Skip to content

Commit

Permalink
net: keep the socket handler instance alive a little longer
Browse files Browse the repository at this point in the history
`make -C cypress_test check-desktop spec=calc/annotation_spec.js`
sometimes failed due to coolwsd crashing in the background.

Checking with sanitizers, a memory corruption always happens on the
websrv_poll thread. First we call handleIncomingMessage() on a socket
handler:
    Klomgor#13 0x5632a3dbf888 in StreamSocket::setHandler(std::shared_ptr<ProtocolHandlerInterface>) /home/vmiklos/git/collaboraonline/online-san/./net/Socket.hpp:1335:24
    Klomgor#14 0x5632a3d6d44e in AdminSocketHandler::handleInitialRequest(std::weak_ptr<StreamSocket> const&, Poco::Net::HTTPRequest const&) /home/vmiklos/git/collaboraonline/online-san/wsd/Admin.cpp:499:17
    Klomgor#15 0x5632a4291333 in ClientRequestDispatcher::handleIncomingMessage(SocketDisposition&) /home/vmiklos/git/collaboraonline/online-san/wsd/ClientRequestDispatcher.cpp:797:17
    Klomgor#16 0x5632a4c9da3e in StreamSocket::handlePoll(SocketDisposition&, std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l>>>, int) /home/vmiklos/git/collaboraonline/online-san/./net/Socket.hpp:1501:33
Note how StreamSocket::handlePoll() invokes a member function of
_socketHandler, which calls back and resets _socketHandler. Then a
little bit later, still in
ClientRequestDispatcher::handleIncomingMessage() we try to continue
using `this`:
    #0 0x5632a3db1485 in ProtocolHandlerInterface::logPrefix(std::ostream&) const /home/vmiklos/git/collaboraonline/online-san/./net/Socket.hpp:517:66
    Klomgor#1 0x5632a4298f32 in ClientRequestDispatcher::handleIncomingMessage(SocketDisposition&) /home/vmiklos/git/collaboraonline/online-san/wsd/ClientRequestDispatcher.cpp:969:9
    Klomgor#2 0x5632a4c9da3e in StreamSocket::handlePoll(SocketDisposition&, std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l>>>, int) /home/vmiklos/git/collaboraonline/online-san/./net/Socket.hpp:1501:33
And that's a problem, since setHandler() already deleted the
ClientRequestDispatcher by that time.

Fix the problem by keeping the socket handler alive for the duration of
the handleIncomingMessage() call.

All tests in the `make -C cypress_test check-desktop
spec=calc/annotation_spec.js` suite passes with sanitizers after this.

Signed-off-by: Miklos Vajna <[email protected]>
Change-Id: I2360e71f7420ae96486c0a95fff77640c4bef29e
  • Loading branch information
vmiklos authored and caolanm committed Nov 5, 2024
1 parent 6f8c509 commit 1ed0823
Showing 1 changed file with 3 additions and 0 deletions.
3 changes: 3 additions & 0 deletions net/Socket.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1512,6 +1512,9 @@ class StreamSocket : public Socket,

try
{
// Keep the current handler alive, while the incoming message is handled.
std::shared_ptr<ProtocolHandlerInterface> socketHandler(_socketHandler);

_socketHandler->handleIncomingMessage(disposition);
}
catch (const std::exception& exception)
Expand Down

0 comments on commit 1ed0823

Please sign in to comment.