Disconnect instead of crashing if a client sends a large Connect
We were rejecting connections if the sha256 didn't match, but generally
when the sha256 doesn't match, someone has changed the size of the
Connect message. This was resulting in the Read call overflowing the
buffer and CHECKing. Instead, trigger an abort in this case, and boot
the client safely.
Change-Id: I565acc61e14c99b3e32fd50bea9972cd195bb28d
Signed-off-by: James Kuszmaul <james.kuszmaul@bluerivertech.com>
diff --git a/aos/network/message_bridge_client_lib.cc b/aos/network/message_bridge_client_lib.cc
index a340553..6db772a 100644
--- a/aos/network/message_bridge_client_lib.cc
+++ b/aos/network/message_bridge_client_lib.cc
@@ -206,6 +206,8 @@
}
} else if (message->message_type == Message::kMessage) {
HandleData(message.get());
+ } else if (message->message_type == Message::kOverflow) {
+ NodeDisconnected();
}
client_.FreeMessage(std::move(message));
}
diff --git a/aos/network/message_bridge_server_lib.cc b/aos/network/message_bridge_server_lib.cc
index a474e88..07ea969 100644
--- a/aos/network/message_bridge_server_lib.cc
+++ b/aos/network/message_bridge_server_lib.cc
@@ -495,6 +495,9 @@
}
} else if (message->message_type == Message::kMessage) {
HandleData(message.get());
+ } else if (message->message_type == Message::kOverflow) {
+ MaybeIncrementInvalidConnectionCount(nullptr);
+ NodeDisconnected(message->header.rcvinfo.rcv_assoc_id);
}
server_.FreeMessage(std::move(message));
}
diff --git a/aos/network/message_bridge_test.cc b/aos/network/message_bridge_test.cc
index 07a56f0..ccb907b 100644
--- a/aos/network/message_bridge_test.cc
+++ b/aos/network/message_bridge_test.cc
@@ -8,6 +8,7 @@
#include "aos/events/pong_generated.h"
#include "aos/ipc_lib/event.h"
#include "aos/network/message_bridge_client_lib.h"
+#include "aos/network/message_bridge_protocol.h"
#include "aos/network/message_bridge_server_lib.h"
#include "aos/network/team_number.h"
#include "aos/sha256.h"
@@ -1354,8 +1355,9 @@
pi1_remote_timestamp_thread.join();
}
-// Test that differing config sha256's result in no connection.
-TEST_P(MessageBridgeParameterizedTest, MismatchedSha256) {
+// Test that a client which connects with too big a message gets disconnected
+// without crashing.
+TEST_P(MessageBridgeParameterizedTest, TooBigConnect) {
// This is rather annoying to set up. We need to start up a client and
// server, on the same node, but get them to think that they are on different
// nodes.
@@ -1403,7 +1405,48 @@
StartPi2Server();
{
- MakePi2Client();
+ // Now, spin up a SctpClient and send a massive hunk of data. This should
+ // trigger a disconnect, but no crash.
+ OnPi2();
+ FLAGS_application_name = "pi2_message_bridge_client";
+ pi2_client_event_loop =
+ std::make_unique<aos::ShmEventLoop>(&config.message());
+ pi2_client_event_loop->SetRuntimeRealtimePriority(1);
+
+ const aos::Node *const remote_node = CHECK_NOTNULL(
+ configuration::GetNode(pi2_client_event_loop->configuration(), "pi1"));
+
+ const aos::FlatbufferDetachedBuffer<aos::message_bridge::Connect>
+ connect_message(MakeConnectMessage(
+ pi2_client_event_loop->configuration(),
+ pi2_client_event_loop->node(), "pi1",
+ pi2_client_event_loop->boot_uuid(), config_sha256));
+
+ SctpClient client(remote_node->hostname()->string_view(),
+ remote_node->port(),
+ connect_message.message().channels_to_transfer()->size() +
+ kControlStreams(),
+ "");
+
+ client.SetMaxReadSize(100000);
+ client.SetMaxWriteSize(100000);
+
+ client.SetPoolSize(2u);
+
+ const std::string big_data(100000, 'a');
+
+ pi2_client_event_loop->epoll()->OnReadable(client.fd(), [&]() {
+ aos::unique_c_ptr<Message> message = client.Read();
+ client.FreeMessage(std::move(message));
+ });
+
+ aos::TimerHandler *const send_big_message = pi2_client_event_loop->AddTimer(
+ [&]() { CHECK(client.Send(kConnectStream(), big_data, 0)); });
+
+ pi2_client_event_loop->OnRun([this, send_big_message]() {
+ send_big_message->Schedule(pi2_client_event_loop->monotonic_now() +
+ chrono::seconds(1));
+ });
RunPi2Client(chrono::milliseconds(3050));
@@ -1411,7 +1454,7 @@
EXPECT_TRUE(pi1_server_statistics_fetcher.Fetch());
EXPECT_TRUE(pi1_client_statistics_fetcher.Fetch());
EXPECT_TRUE(pi2_server_statistics_fetcher.Fetch());
- EXPECT_TRUE(pi2_client_statistics_fetcher.Fetch());
+ EXPECT_FALSE(pi2_client_statistics_fetcher.Fetch());
const ServerConnection *const pi1_connection =
pi1_server_statistics_fetcher->connections()->Get(0);
@@ -1419,17 +1462,12 @@
pi1_client_statistics_fetcher->connections()->Get(0);
const ServerConnection *const pi2_connection =
pi2_server_statistics_fetcher->connections()->Get(0);
- const ClientConnection *const pi2_client_connection =
- pi2_client_statistics_fetcher->connections()->Get(0);
// Make sure one direction is disconnected with a bunch of connection
// attempts and failures.
EXPECT_EQ(pi1_connection->state(), State::DISCONNECTED);
EXPECT_EQ(pi1_connection->connection_count(), 0u);
- EXPECT_GT(pi1_connection->invalid_connection_count(), 10u);
-
- EXPECT_EQ(pi2_client_connection->state(), State::DISCONNECTED);
- EXPECT_GT(pi2_client_connection->connection_count(), 10u);
+ EXPECT_GE(pi1_server_statistics_fetcher->invalid_connection_count(), 1u);
// And the other direction is happy.
EXPECT_EQ(pi2_connection->state(), State::CONNECTED);
@@ -1443,9 +1481,10 @@
VLOG(1) << aos::FlatbufferToJson(pi2_server_statistics_fetcher.get());
VLOG(1) << aos::FlatbufferToJson(pi1_server_statistics_fetcher.get());
- VLOG(1) << aos::FlatbufferToJson(pi2_client_statistics_fetcher.get());
VLOG(1) << aos::FlatbufferToJson(pi1_client_statistics_fetcher.get());
+ pi2_client_event_loop->epoll()->DeleteFd(client.fd());
+
StopPi2Client();
}
diff --git a/aos/network/sctp_lib.cc b/aos/network/sctp_lib.cc
index 3f482c3..d0924d5 100644
--- a/aos/network/sctp_lib.cc
+++ b/aos/network/sctp_lib.cc
@@ -404,11 +404,6 @@
CHECK(!(inmessage.msg_flags & MSG_CTRUNC))
<< ": Control message truncated.";
- CHECK_LE(size, static_cast<ssize_t>(max_read_size_))
- << ": Message overflowed buffer on stream "
- << result->header.rcvinfo.rcv_sid << ".";
-
- result->size = size;
if (MSG_NOTIFICATION & inmessage.msg_flags) {
result->message_type = Message::kNotification;
} else {
@@ -436,6 +431,25 @@
<< ": Failed to find a SCTP_RCVINFO cmsghdr. flags: "
<< inmessage.msg_flags;
}
+
+ // Client just sent too big a block of data. Eat it and signal up the
+ // chain.
+ result->size = size;
+ if (size > static_cast<ssize_t>(max_read_size_)) {
+ Abort(result->header.rcvinfo.rcv_assoc_id);
+ result->message_type = Message::kOverflow;
+
+ VLOG(1) << "Message overflowed buffer on stream "
+ << result->header.rcvinfo.rcv_sid << ", disconnecting."
+ << " Check for config mismatch or rogue device.";
+ return result;
+ }
+
+ CHECK_LE(size, static_cast<ssize_t>(max_read_size_))
+ << ": Message overflowed buffer on stream "
+ << result->header.rcvinfo.rcv_sid << "."
+ << " Check for config mismatch or rogue device.";
+
if (result->message_type == Message::kNotification) {
// Notifications are never fragmented, just return it now.
CHECK(inmessage.msg_flags & MSG_EOR)
diff --git a/aos/network/sctp_lib.h b/aos/network/sctp_lib.h
index 42cecea..14509e4 100644
--- a/aos/network/sctp_lib.h
+++ b/aos/network/sctp_lib.h
@@ -53,7 +53,14 @@
struct sockaddr_storage sin;
// Data type. Is it a block of data, or is it a struct sctp_notification?
- enum MessageType { kMessage, kNotification } message_type;
+ enum MessageType {
+ // Block of data?
+ kMessage,
+ // struct sctp_notification?
+ kNotification,
+ // Client sent too large a message and was disconnected.
+ kOverflow,
+ } message_type;
size_t size = 0u;
uint8_t *mutable_data() {