Fix error handling when creating clients (mp::ConnectStream)

[xyzconstant]

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:

1. Connecting to a socket serving a valid init interface
2. Passing a disconnected socket (ConnectStream throws during the construct() call)
3. Passing a disconnected socket to an interface without construct() (the failure is deferred to the first IPC request)
4. Passing a disconnected socket and making no calls (the disconnect is still handled and the connection cleaned up)
5. Passing a live socket that disconnects after some data is received
6. 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]

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.

[ryanofsky]

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]

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]

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

[xyzconstant]

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]

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]

Code review 96c32f3

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 #" 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 91f015e 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 91f015e 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]

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.

[DrahtBot]

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