From a9258e15d2f46f7b5b7d6c869de1b16aa3be6335 Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Wed, 18 Oct 2023 23:06:54 -0700 Subject: [PATCH 1/3] [ntcore] Fix crash on disconnect The object was being destroyed due to the error handling path. Instead, defer to the next loop cycle using a one-shot timer. Properly handle error return values from Send functions. Fix UB in accessing one past the end of a vector. --- ntcore/src/main/native/cpp/NetworkClient.cpp | 11 +++++++++-- ntcore/src/main/native/cpp/NetworkServer.cpp | 10 ++++++++-- ntcore/src/main/native/cpp/net/NetworkOutgoingQueue.h | 6 ++++++ ntcore/src/main/native/cpp/net/ServerImpl.cpp | 9 +++++++++ .../src/main/native/cpp/net/WebSocketConnection.cpp | 3 ++- 5 files changed, 34 insertions(+), 5 deletions(-) diff --git a/ntcore/src/main/native/cpp/NetworkClient.cpp b/ntcore/src/main/native/cpp/NetworkClient.cpp index fa849a471eb..9ea31a89026 100644 --- a/ntcore/src/main/native/cpp/NetworkClient.cpp +++ b/ntcore/src/main/native/cpp/NetworkClient.cpp @@ -242,7 +242,11 @@ void NetworkClient3::TcpConnected(uv::Tcp& tcp) { tcp.error.connect([this, &tcp](uv::Error err) { DEBUG3("NT3 TCP error {}", err.str()); if (!tcp.IsLoopClosing()) { - DoDisconnect(err.str()); + // we could be in the middle of sending data, so defer disconnect + uv::Timer::SingleShot(m_loop, uv::Timer::Time{0}, + [this, reason = std::string{err.str()}] { + DoDisconnect(reason); + }); } }); tcp.end.connect([this, &tcp] { @@ -412,7 +416,10 @@ void NetworkClient::WsConnected(wpi::WebSocket& ws, uv::Tcp& tcp, m_clientImpl->SendInitial(); ws.closed.connect([this, &ws](uint16_t, std::string_view reason) { if (!ws.GetStream().IsLoopClosing()) { - DoDisconnect(reason); + // we could be in the middle of sending data, so defer disconnect + uv::Timer::SingleShot( + m_loop, uv::Timer::Time{0}, + [this, reason = std::string{reason}] { DoDisconnect(reason); }); } }); ws.text.connect([this](std::string_view data, bool) { diff --git a/ntcore/src/main/native/cpp/NetworkServer.cpp b/ntcore/src/main/native/cpp/NetworkServer.cpp index 21eebd91c1b..f4e9325434e 100644 --- a/ntcore/src/main/native/cpp/NetworkServer.cpp +++ b/ntcore/src/main/native/cpp/NetworkServer.cpp @@ -150,7 +150,11 @@ NetworkServer::ServerConnection3::ServerConnection3( if (!m_wire->GetDisconnectReason().empty()) { return; } - m_wire->Disconnect(fmt::format("stream error: {}", err.name())); + // we could be in the middle of sending data, so defer disconnect + uv::Timer::SingleShot( + m_wire->GetStream().GetLoop(), uv::Timer::Time{0}, [this, err] { + m_wire->Disconnect(fmt::format("stream error: {}", err.name())); + }); m_wire->GetStream().Shutdown([this] { m_wire->GetStream().Close(); }); }); stream->end.connect([this] { @@ -284,7 +288,9 @@ void NetworkServer::ServerConnection4::ProcessWsUpgrade() { auto realReason = m_wire->GetDisconnectReason(); INFO("DISCONNECTED NT4 client '{}' (from {}): {}", m_info.remote_id, m_connInfo, realReason.empty() ? reason : realReason); - ConnectionClosed(); + // we could be in the middle of sending data, so defer disconnect + uv::Timer::SingleShot(m_websocket->GetStream().GetLoop(), + uv::Timer::Time{0}, [this] { ConnectionClosed(); }); }); m_websocket->text.connect([this](std::string_view data, bool) { m_server.m_serverImpl.ProcessIncomingText(m_clientId, data); diff --git a/ntcore/src/main/native/cpp/net/NetworkOutgoingQueue.h b/ntcore/src/main/native/cpp/net/NetworkOutgoingQueue.h index f5670ae98ea..81c2b2efad2 100644 --- a/ntcore/src/main/native/cpp/net/NetworkOutgoingQueue.h +++ b/ntcore/src/main/native/cpp/net/NetworkOutgoingQueue.h @@ -280,9 +280,15 @@ void NetworkOutgoingQueue::SendOutgoing(uint64_t curTimeMs, }); } } + if (unsent < 0) { + return; // error + } if (unsent == 0) { // finish writing any partial buffers unsent = m_wire.Flush(); + if (unsent < 0) { + return; // error + } } int delta = it - msgs.begin() - unsent; for (auto&& msg : std::span{msgs}.subspan(0, delta)) { diff --git a/ntcore/src/main/native/cpp/net/ServerImpl.cpp b/ntcore/src/main/native/cpp/net/ServerImpl.cpp index 66856cfd9db..5f3a2984282 100644 --- a/ntcore/src/main/native/cpp/net/ServerImpl.cpp +++ b/ntcore/src/main/native/cpp/net/ServerImpl.cpp @@ -524,6 +524,9 @@ void ServerImpl::ClientData4::SendAnnounce(TopicData* topic, WireEncodeAnnounce(os, topic->name, topic->id, topic->typeStr, topic->properties, pubuid); }); + if (unsent < 0) { + return; // error + } if (unsent == 0 && m_wire.Flush() == 0) { return; } @@ -544,6 +547,9 @@ void ServerImpl::ClientData4::SendUnannounce(TopicData* topic) { if (m_local) { int unsent = m_wire.WriteText( [&](auto& os) { WireEncodeUnannounce(os, topic->name, topic->id); }); + if (unsent < 0) { + return; // error + } if (unsent == 0 && m_wire.Flush() == 0) { return; } @@ -565,6 +571,9 @@ void ServerImpl::ClientData4::SendPropertiesUpdate(TopicData* topic, int unsent = m_wire.WriteText([&](auto& os) { WireEncodePropertiesUpdate(os, topic->name, update, ack); }); + if (unsent < 0) { + return; // error + } if (unsent == 0 && m_wire.Flush() == 0) { return; } diff --git a/ntcore/src/main/native/cpp/net/WebSocketConnection.cpp b/ntcore/src/main/native/cpp/net/WebSocketConnection.cpp index 986156cdbd0..e8ac9142b17 100644 --- a/ntcore/src/main/native/cpp/net/WebSocketConnection.cpp +++ b/ntcore/src/main/native/cpp/net/WebSocketConnection.cpp @@ -186,7 +186,8 @@ int WebSocketConnection::Flush() { m_ws_frames.reserve(m_frames.size()); for (auto&& frame : m_frames) { m_ws_frames.emplace_back( - frame.opcode, std::span{&m_bufs[frame.start], &m_bufs[frame.end]}); + frame.opcode, + std::span{m_bufs}.subspan(frame.start, frame.end - frame.start)); } auto unsentFrames = m_ws.TrySendFrames( From d1baef31263b4cfc3b354126ae9e6dbf5f9e7135 Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Thu, 19 Oct 2023 00:24:52 -0700 Subject: [PATCH 2/3] Use weak pointers --- ntcore/src/main/native/cpp/NetworkServer.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/ntcore/src/main/native/cpp/NetworkServer.cpp b/ntcore/src/main/native/cpp/NetworkServer.cpp index f4e9325434e..7cf37899476 100644 --- a/ntcore/src/main/native/cpp/NetworkServer.cpp +++ b/ntcore/src/main/native/cpp/NetworkServer.cpp @@ -152,8 +152,11 @@ NetworkServer::ServerConnection3::ServerConnection3( } // we could be in the middle of sending data, so defer disconnect uv::Timer::SingleShot( - m_wire->GetStream().GetLoop(), uv::Timer::Time{0}, [this, err] { - m_wire->Disconnect(fmt::format("stream error: {}", err.name())); + m_wire->GetStream().GetLoop(), uv::Timer::Time{0}, + [wire = m_wire->weak_from_this(), err] { + if (auto w = wire.lock()) { + w->Disconnect(fmt::format("stream error: {}", err.name())); + } }); m_wire->GetStream().Shutdown([this] { m_wire->GetStream().Close(); }); }); @@ -290,7 +293,11 @@ void NetworkServer::ServerConnection4::ProcessWsUpgrade() { m_connInfo, realReason.empty() ? reason : realReason); // we could be in the middle of sending data, so defer disconnect uv::Timer::SingleShot(m_websocket->GetStream().GetLoop(), - uv::Timer::Time{0}, [this] { ConnectionClosed(); }); + uv::Timer::Time{0}, [self = weak_from_this()] { + if (auto s = self.lock()) { + s->ConnectionClosed(); + } + }); }); m_websocket->text.connect([this](std::string_view data, bool) { m_server.m_serverImpl.ProcessIncomingText(m_clientId, data); From 138c9558a9c0a167fb92e5de356e41f44cb363ae Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Thu, 19 Oct 2023 09:00:50 -0700 Subject: [PATCH 3/3] Revert NetworkServer changes --- ntcore/src/main/native/cpp/NetworkServer.cpp | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/ntcore/src/main/native/cpp/NetworkServer.cpp b/ntcore/src/main/native/cpp/NetworkServer.cpp index 7cf37899476..21eebd91c1b 100644 --- a/ntcore/src/main/native/cpp/NetworkServer.cpp +++ b/ntcore/src/main/native/cpp/NetworkServer.cpp @@ -150,14 +150,7 @@ NetworkServer::ServerConnection3::ServerConnection3( if (!m_wire->GetDisconnectReason().empty()) { return; } - // we could be in the middle of sending data, so defer disconnect - uv::Timer::SingleShot( - m_wire->GetStream().GetLoop(), uv::Timer::Time{0}, - [wire = m_wire->weak_from_this(), err] { - if (auto w = wire.lock()) { - w->Disconnect(fmt::format("stream error: {}", err.name())); - } - }); + m_wire->Disconnect(fmt::format("stream error: {}", err.name())); m_wire->GetStream().Shutdown([this] { m_wire->GetStream().Close(); }); }); stream->end.connect([this] { @@ -291,13 +284,7 @@ void NetworkServer::ServerConnection4::ProcessWsUpgrade() { auto realReason = m_wire->GetDisconnectReason(); INFO("DISCONNECTED NT4 client '{}' (from {}): {}", m_info.remote_id, m_connInfo, realReason.empty() ? reason : realReason); - // we could be in the middle of sending data, so defer disconnect - uv::Timer::SingleShot(m_websocket->GetStream().GetLoop(), - uv::Timer::Time{0}, [self = weak_from_this()] { - if (auto s = self.lock()) { - s->ConnectionClosed(); - } - }); + ConnectionClosed(); }); m_websocket->text.connect([this](std::string_view data, bool) { m_server.m_serverImpl.ProcessIncomingText(m_clientId, data);