Fix error handling when creating clients (`mp::ConnectStream`) #298

xyzconstant commented at 3:53 AM on June 24, 2026: contributor

Avoid use-after-free if the socket is disconnected before ConnectStream connects (#308), and avoid leaks and hangs if client construct() calls throw (#309). Also add tests to cover these and other client connection errors, as suggested by @ryanofsky.

The following cases are tested:

Connecting to a socket serving a valid init interface
Passing a disconnected socket (ConnectStream throws during the construct() call)
Passing a disconnected socket to an interface without construct() (the failure is deferred to the first IPC request)
Passing a disconnected socket and making no calls (the disconnect is still handled and the connection cleaned up)
Passing a live socket that disconnects after some data is received
Passing a socket from a listening socket (accept()) that disconnects after some data arrives

Additionally, a new FooInit test interface is added, and the UnixListener class introduced in #269 is extracted to a shared file so the new connect_tests.cpp file can use it.

Note: Clients that own their connection now delete it on unexpected disconnects, so calls after a server disconnect fail with "called after disconnect" instead of "interrupted by disconnect" (one test.cpp assertion updated accordingly).

DrahtBot commented at 3:54 AM on June 24, 2026: none

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#274 (Add nonunix platform support by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

xyzconstant renamed this:
~~WIP: Add test coverage for ConnectStream~~
WIP: Add test coverage for `ConnectStream`
on Jun 24, 2026

ryanofsky commented at 8:21 PM on July 5, 2026: collaborator

Thanks for following up to #183 with these tests. They do seem potentially useful. Here is feedback I'd have:

The first 3 tests seems like they are mostly in good shape. You would just want to wrap these in try/catch to and assert expected exceptions are thrown. For the second test it would seem fine to accept both "called after disconnect" and "interrupted by disconnect" exceptions since we already have existing tests testing for each of these errors more specifically.
The first 3 tests do overlap a lot with disconnect tests already in test.cpp, and it's a little questionable if having all 3 tests adds much value. The first test could be is nice because it directly tests connecting to a non-capnp server that disconnects ignoring whatever is sent. But the second and third tests just sending the same disconnect at later points in time and needing MSG_PEEK complexity would not seem to add as much value.
For the 4th test having the client hang as long as server hangs is probably expected behavior. It would be good to make sure that client can still disconnect or cancel the calls if the the server hangs. Or that client calls are able to time out correctly.
It could make sense to rebase this on #269 or rebase if that could help with the "Add the case with an actual mkdtemp/socket/bind/listen setup" follow up comment.
Maybe (not sure) it could be interesting to have tests working in opposite direction with server providing dummy interface and clients connecting and disconnecting suddenly. Maybe it could also be useful to have tests sending garbage bytes and making sure clients and servers handle them cleanly. It might even be useful to use fuzzing for this though it might not be a good use of fuzzing resources since it would mostly only be fuzz testing capnproto code and only a little bit of libmultiprocess exception/cleanup handling code.

Overall the tests here seem reasonable to add. It seems good to have at least 1-2 tests verifying disconnects are processed when a capnproto client connects to a non-capnproto server.

(Relatedly, there are also other disconnect tests that could be added at different points during capnproto connections, which I started to write in #201 (comment) and https://github.com/ryanofsky/libmultiprocess/commits/pr/distest.2 but was never able to really finish due to complexity of trying to set up and cover all of the relevant cases. Just mentioning this for completeness, though. There's probably not an obviously place to follow up with this at the moment.)

xyzconstant force-pushed on Jul 8, 2026

DrahtBot added the label Needs rebase on Jul 8, 2026

xyzconstant force-pushed on Jul 8, 2026

DrahtBot removed the label Needs rebase on Jul 8, 2026

xyzconstant force-pushed on Jul 8, 2026

xyzconstant renamed this:
~~WIP: Add test coverage for `ConnectStream`~~
Add test coverage for `ConnectStream`
on Jul 8, 2026

xyzconstant marked this as ready for review on Jul 8, 2026

xyzconstant force-pushed on Jul 8, 2026

xyzconstant commented at 9:54 PM on July 8, 2026: contributor

Thanks for the feedback @ryanofsky!

I've just rebased to master now that #269 has been merged and added a new commit to move UnixListener to its own dedicated file.

Also, I've made changes to the tests (please check the description), keeping the first 2 tests (Notice that I've dropped the MSG_PEEK setup as you suggested.) now catching errors in a try/catch block, plus a Unix domain socket that disconnects after some data arrives and a successful case connecting to a valid libmultiprocess server.

It would be good to make sure that client can still disconnect or cancel the calls if the the server hangs. Or that client calls are able to time out correctly.

This is my current focus. I'm looking more into it, but I think this work might need its own branch so we keep test coverage scoped to this PR.

Maybe (not sure) it could be interesting to have tests working in opposite direction with server providing dummy interface and clients connecting and disconnecting suddenly. Maybe it could also be useful to have tests sending garbage bytes and making sure clients and servers handle them cleanly. It might even be useful to use fuzzing for this though it might not be a good use of fuzzing resources since it would mostly only be fuzz testing capnproto code and only a little bit of libmultiprocess exception/cleanup handling code.

Good, these ideas are great and definitely worth exploring. Happy to tackle them after this.

xyzconstant commented at 9:55 PM on July 8, 2026: contributor

This PR is now ready for review. I've updated the description as well!

xyzconstant force-pushed on Jul 8, 2026

xyzconstant force-pushed on Jul 9, 2026

xyzconstant force-pushed on Jul 10, 2026

xyzconstant commented at 5:44 AM on July 10, 2026: contributor

The netbsd 9.4 job exposed a use-after-free race condition in ConnectStream on early disconnects, included a new commit in this PR that fixes it.

in test/mp/test/connect_tests.cpp:58 in a648c5120f

  53 | +            loop_promise.set_value(&loop);
  54 | +            loop.loop();
  55 | +        });
  56 | +        loop = loop_promise.get_future().get();
  57 | +
  58 | +        // Initalize and store sockets

maflcko commented at 6:20 AM on July 10, 2026:

LLM Linter (✨ experimental)

Possible typos and grammar issues:

Initalize -> Initialize [misspelling in the socket setup comment; intended meaning is clear but the word is misspelled]

2026-07-10 05:29:42

xyzconstant commented at 2:50 PM on July 10, 2026:

Thanks!

xyzconstant force-pushed on Jul 10, 2026

xyzconstant commented at 4:09 PM on July 10, 2026: contributor

The netbsd 9.4 job exposed a use-after-free race condition in ConnectStream on early disconnects, included a new commit in this PR that fixes it.

Updated the description expanding more on this issue.

xyzconstant force-pushed on Jul 13, 2026

xyzconstant commented at 6:38 PM on July 13, 2026: contributor

Added the FooInit test interface which declares construct() so we could trigger that call against a disconnected socket in tests. It exposed a Connection leak, which, for instance, prevented the EventLoop::loop() from ever exiting.

Updated the description with more details about it and included a fix in the second commit.

ryanofsky commented at 10:48 PM on July 13, 2026: collaborator

Code review 96c32f39b8d9fac2c6b36ffea98d8ff2235112e6

Nice tests and fixes! The changes here look pretty good. I would just suggest a number of things to make this easier to understand and review:

Since this PR is not only adding test coverage, would change title to something like "Fix error handling when creating clients" and give a description like "Avoid use-after-free if socket is disconnected before ConnectStream connects, and avoid leaks and hangs if client construct calls throw. Also add tests to cover these and other client connection errors"
Would suggest adding the tests before making the bugfixes, otherwise it's not clear how the tests and bugfixes relate. It looks like both bugs are caught by the "Passing a disconnected socket will throw" test which is a very short test, and none of the other tests depend on fixes. So it would be clearer to add all the new tests except "Passing a disconnected socket will throw" initially, to be clear they are unrelated to the bugs.
PR description is very long because it goes into detail about the history of the bugs and you how debugged them. I think this information is useful and interesting but it makes it harder to get a quick understanding, so I'd suggest moving this detail into separate github issues describing each bug and adding "Fixes #<issue number>" to the relevant fix commits so they will be closed when this PR is merged.
It would be good if description of the bugs mentioned they are not new and have always been present. I found current description of the bug in 91f015eb71fbc03773f02d6177cb2d099a4bef4b that begins with "Previously," confusing because I thought it was referring to something that was previously working but now broken, when actually it's fixing something that has always been broken.
I think it might be clearer to fix both bugs and add the short "Passing a disconnected socket will throw" all in a single commit. The first fix 91f015eb71fbc03773f02d6177cb2d099a4bef4b looks correct but it would seem nicer if it just let ProxyClientBase destructor be fully responsible for destroying the connection when destroy_connection is true instead of requiring the caller to destroy it. So could add something like if (destroy_connection) { sync([&]{ connection->onDisconnect(...); } } the end of the ProxyClientBase constructor to make ConnectStream simpler than it was before, instead of more complicated.
I think it would be good to add a comment to ConnectStream that it calls the InitInterface.construct method if one is present, so it may block or throw.

xyzconstant force-pushed on Jul 14, 2026

xyzconstant renamed this:
~~Add test coverage for `ConnectStream`~~
Fix error handling when creating clients
on Jul 15, 2026

xyzconstant renamed this:
~~Fix error handling when creating clients~~
Fix error handling when creating clients (`mp::ConnectStream`)
on Jul 15, 2026

xyzconstant force-pushed on Jul 15, 2026

xyzconstant commented at 5:50 PM on July 15, 2026: contributor

re: #298 (comment)

Thanks for the review @ryanofsky!

Addressed your feedback and force-pushed, I hope the commit history is cleaner now.

Opened issues (#308 and #309) and updated the PR title and description as well.

Also, please note that I've added 2 new tests (I just noticed I pushed them right after your latest review, so you might have missed them), and I think they're valuable for showcasing the difference with an Init interface without construct(). They're included in the latest commit along with the single "disconnected socket" case.

xyzconstant force-pushed on Jul 15, 2026

DrahtBot added the label Needs rebase on Jul 17, 2026

DrahtBot commented at 11:12 AM on July 17, 2026: none

🐙 This pull request conflicts with the target branch and needs rebase.

Extract `UnixListener` class to a dedicated file

Next commit will consume the `UnixListener` class in another test file,
so move it to a shared one.

59f202f46a

Correct stale UnixListener doc comment 13faaf27da

Add test coverage for ConnectStream

This commit introduces a new test file `connect_tests.cpp`
for testing the `ConnectStream` client method.

96b9905d07

Fix error handling when creating clients

Two bugs that have always been present:

1. `ConnectStream` registered the `onDisconnect` handler that deletes the
`Connection` before the `ProxyClient` object owning it was created, so an
early disconnect could delete the `Connection` while the client constructor
was reading it (#308).

2. A `construct()` method failing during client construction caused a
`Connection` leak. Normally, cleanup happens in the destructor but a
constructor that throws leaves no object behind, so it never runs, resulting
in an event loop ref preventing `EventLoop::loop()` from exiting (#309).

Fix the first by making `ProxyClientBase` responsible for the connection
when `destroy_connection` is true, registering the delete-on-disconnect
handler at the end of its constructor. Fix the second by running the cleanup
functions before rethrowing.

Fixes #308
Fixes #309

a2e00075e5

xyzconstant force-pushed on Jul 21, 2026