Skip to content

wsd: protect against thread-affinity violation in StreamSocket dtor #15120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Ashod wants to merge 1 commit into
base: main
Choose a base branch
from
+15 −1
Open

wsd: protect against thread-affinity violation in StreamSocket dtor #15120

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion net/Socket.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1238,7 +1238,21 @@
{
LOG_TRC("StreamSocket dtor called with pending write: " << _outBuffer.size()
<< ", read: " << _inBuffer.size());
ensureDisconnected();
if (!_doneDisconnect)
{
// This dtor could be called from a different thread when we are owned by
// a weak_ptr elevated to a shared_ptr while the real owning shared_ptr
// is destroyed. This can happen when we remove a closed socket from the
// poll while in another thread a weak_ptr on it has temporarily lock()'d
// and got another valid reference to it.
// In that case, the real owner should've called ensureDisconnected()
// and we won't need it again here, hence the conditional, and won't get
// tripped-up by the ASSERT_CORRECT_SOCKET_THREAD check inside it.
// Otherwise, we will invoke it and it's only fair to catch the thread
// affinity violation.
ensureDisconnected();

Check warning

Code scanning / CodeQL

Virtual call from constructor or destructor Warning

Call to function ensureDisconnected that calls virtual function
shutdownConnection
Loading
(overridden in
SslStreamSocket
Loading
).

Copilot Autofix

AI about 16 hours ago

In general, the fix is to ensure that constructors and destructors of base classes do not cause virtual dispatch, directly or indirectly. Here, the destructor of StreamSocket calls ensureDisconnected(), which in turn calls the virtual shutdownConnection(). We should refactor so that ensureDisconnected() performs only non-virtual logic and leaves the actual shutdown call to a context where virtual dispatch is appropriate, or to an explicit base-class call.

The simplest change without altering external behavior is:

  • Keep StreamSocket::~StreamSocket() as-is in terms of visible effects (it still performs disconnect logic and then calls StreamSocket::shutdownConnection() explicitly).
  • Modify ensureDisconnected() so it no longer calls asyncShutdown() and shutdownConnection(). That way, when it is invoked from the destructor we do not indirectly invoke any virtual function. We still maintain its primary contract: emitting onDisconnect() once and logging if the socket remains open.
  • Shutdown during destruction continues to be handled exclusively by the explicit call to StreamSocket::shutdownConnection() in the destructor (StreamSocket::shutdownConnection();), which is a non-virtual qualified call and therefore safe in a destructor.
  • In non-destruction contexts, callers that require both disconnect notification and shutdown can perform ensureDisconnected(); asyncShutdown(); shutdownConnection(); themselves if needed. Since we cannot see other callers in this snippet, we avoid modifying them; we only narrow ensureDisconnected()’s responsibilities, which is less likely to break logic because the destructor already performs shutdown unconditionally.

Concretely, in net/Socket.hpp:

  • In the body of ensureDisconnected(), remove the block that conditionally calls asyncShutdown(); and shutdownConnection(); when !isShutdown(). Leave the rest of the method unchanged.

This eliminates the virtual call from the destructor while preserving existing destruction semantics: onDisconnect() remains called once, and shutdown is still performed via the explicit base call in the destructor.

Suggested changeset 1
net/Socket.hpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/net/Socket.hpp b/net/Socket.hpp
--- a/net/Socket.hpp
+++ b/net/Socket.hpp
@@ -1278,11 +1278,7 @@
                 // The SocketHandler has a weak pointer to us and we could
                 // be getting destroyed at this point, so it won't get a
                 // reference to us from the weak pointer, and so can't disconnect.
-                if (!isShutdown())
-                {
-                    asyncShutdown(); // signal
-                    shutdownConnection(); // real -> setShutdown()
-                }
+                // Any required shutdown should be performed explicitly by the caller.
             }
         }
 
EOF
@@ -1278,11 +1278,7 @@
// The SocketHandler has a weak pointer to us and we could
// be getting destroyed at this point, so it won't get a
// reference to us from the weak pointer, and so can't disconnect.
if (!isShutdown())
{
asyncShutdown(); // signal
shutdownConnection(); // real -> setShutdown()
}
// Any required shutdown should be performed explicitly by the caller.
}
}

Copilot is powered by AI and may make mistakes. Always verify output.
Copy link
Contributor

@mmeeks mmeeks Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh - I worry about this case I guess; are we sure that all of this cleanup happens properly in the other threads ? eg. ChildSession::disconnect called from ~ChildSession - from ~ProtocolHandlerInterface looks problematic as an example. I assume we are still warning of thread affinity in that chain though (?) =)

Anyhow - it seems quite plausible, and simple enough. Not sure we want a novel in the comment there though - should be visible from the commit message.

}

_socketHandler.reset();

if (!_shutdownSignalled)
Expand Down
Loading