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.

Type	Reviewers
ACK	ryanofsky

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #336 (proxy-io: Reference-count Connection objects by ryanofsky)
• #304 (proxy: fix BuildList to use non-const iteration for interface types by ryanofsky)
• #231 (Add windows 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]

Rebased on master which now includes #274. Adapted the tests to the stream API (MakeStream) and TestSetup now holds an EventLoopRef because it needs it to keep loop() running.

[ryanofsky]

Code review f3355b5. I need to take more time to understand the tests but the bugfix looks right and useful for improving the stability of the C++ IPC client if it connects to a server that disconnects right away.

I left a minor suggestion below. Also would note that this conflicts with #231 and while I think conflicts mostly just come from moved code, it could be useful to try merging the two PRs and making sure the new tests do not introduce any unix-isms.

[ryanofsky]

In commit "Add test coverage for ConnectStream" (31c1ac2)

Am curious why this new FooInit interface is needed and existing Foo interface isn't used.

It also seems like a potentially complicating factor that could make the tests harder to debug & understand for this to have a construct method. I wonder if it could be dropped or at least the Thread map parameters could be dropped since it doesn't look like anything in these tests requires threadmaps

EDIT: Oh, I see in next commit it looks like there are new tests that rely on the construct call failing. I think it would be to only use the FooInit type for the tests which actually need the construct method, and use FooInterface for other tests. Also would be good to drop ThreadMap parameters as I believe they should not be needed.

[xyzconstant]

Nice suggestion!

I addressed it in 44d1914 by removing the ThreadMap parameters.

[xyzconstant]

Regarding the FooInit/FooInterface split, the tests already follow this. Only the 4 tests that require the construct() call use FooInit.

Also, I've replaced the initThreadMap call with a simple add(1, 2) in the "ConnectStream defers disconnect failure to the first IPC request for interfaces without construct()" test case.

[xyzconstant]

Rebased with master and fixed surfaced IWYU issues.

Additionally, addressed @ryanofsky's feedback by removing the ThreadMap parameter from the (new) FooInit's construct method. Thanks for the review!

[ryanofsky]

Code review ACK fae9a63. I left a lot of comments here about the tests, but the fix itself looks very good and this could be merged as-is.

[ryanofsky]

In commit "Add test coverage for ConnectStream" (44d1914)

All these class members should have m_ prefixes so it is clear when they are accessed from methods that they are class members and not local variables. Lack of prefixes also makes code confusing below because there are two different variables called loop

[xyzconstant]

Addressed this in 70467c5 (follow-up PR).

[ryanofsky]

In commit "Add test coverage for ConnectStream" (44d1914)

This seems to be slightly different than the DefaultLogHandler defined in listen_tests.cpp. Would be nice to define a shared on, maybe in a test.h file.

[xyzconstant]

Nice suggestion! I moved this logger to a common.h file instead at ae5c6bc (follow-up PR).

[ryanofsky]

In commit "Add test coverage for ConnectStream" (44d1914)

Would probably drop mp:: prefix throughout this file since tests are in the mp namespace.

[xyzconstant]

Nice, addressed at 5790bec (follow-up PR).

[ryanofsky]

In commit "Add test coverage for ConnectStream" (44d1914)

Having the init_sockets callback and the client_fd and server_fd members seems unnecessarily complicated given that nothing else in the test setup class uses them. Would seem simpler to drop these

diff

--- a/test/mp/test/connect_tests.cpp
+++ b/test/mp/test/connect_tests.cpp
@@ -9,16 +9,15 @@
 #include <kj/test.h>
 #include <mp/proxy.h>
 #include <mp/proxy-io.h>
+#include <mp/util.h>
 #include <mp/test/foo.capnp.h>
 #include <mp/test/foo.capnp.proxy.h>
 #include <sys/socket.h>
-#include <sys/types.h>
 #include <unistd.h>
 
 #include <chrono>
 #include <condition_variable>
 #include <cstring> // IWYU pragma: keep
-#include <functional>
 #include <future>
 #include <memory>
 #include <mutex>
@@ -45,9 +44,6 @@ void DefaultLogHandler(mp::LogMessage log)
 class TestSetup
 {
 public:
-    int client_fd;
-    int server_fd;
-
     mp::EventLoop* loop;
     std::optional<mp::EventLoopRef> loop_ref;
     //! Thread variable should be after other struct members so the thread does
@@ -55,14 +51,6 @@ public:
     std::thread loop_thread;
 
     TestSetup(mp::LogFn log_handler = DefaultLogHandler)
-        : TestSetup(
-              [](int fds[2]) {
-                  KJ_REQUIRE(socketpair(AF_UNIX, SOCK_STREAM, 0, fds) != -1);
-              },
-              log_handler) {}
-
-    TestSetup(const std::function<void(int[2])>& init_sockets,
-              mp::LogFn log_handler = DefaultLogHandler)
     {
         std::promise<mp::EventLoop*> loop_promise;
         loop_thread = std::thread([&, log_handler] {
@@ -72,13 +60,6 @@ public:
         });
         loop = loop_promise.get_future().get();
         loop_ref.emplace(*loop);
-
-        // Initialize and store sockets
-        int fds[2] = {-1, -1};
-        init_sockets(fds);
-
-        client_fd = fds[0];
-        server_fd = fds[1];
     }
 
     ~TestSetup()
@@ -91,15 +72,16 @@ public:
 KJ_TEST("ConnectStream connects to a socket serving a valid init interface")
 {
     TestSetup setup;
+    auto [client_fd, server_fd] = SocketPair();
 
-    std::thread server_thread([&setup]() {
+    std::thread server_thread([&]() {
         mp::EventLoop server_loop("mptest-valid-server", DefaultLogHandler);
         std::unique_ptr<FooInit> init = std::make_unique<FooInit>();
-        ServeStream<messages::FooInit>(server_loop, MakeStream(server_loop, setup.server_fd), *init);
+        ServeStream<messages::FooInit>(server_loop, MakeStream(server_loop, server_fd), *init);
         server_loop.loop();
     });
 
-    auto init = ConnectStream<messages::FooInit>(*setup.loop, MakeStream(*setup.loop, setup.client_fd));
+    auto init = ConnectStream<messages::FooInit>(*setup.loop, MakeStream(*setup.loop, client_fd));
 
     init.reset();
     server_thread.join();
@@ -109,11 +91,12 @@ KJ_TEST("ConnectStream connects to a socket serving a valid init interface")
 KJ_TEST("ConnectStream throws when the socket is already disconnected")
 {
     TestSetup setup;
+    auto [client_fd, server_fd] = SocketPair();
 
-    close(setup.server_fd);
+    close(server_fd);
 
     try {
-        auto init = ConnectStream<messages::FooInit>(*setup.loop, MakeStream(*setup.loop, setup.client_fd));
+        auto init = ConnectStream<messages::FooInit>(*setup.loop, MakeStream(*setup.loop, client_fd));
 
         KJ_EXPECT(false);
     } catch (const std::runtime_error& e) {
@@ -126,12 +109,13 @@ KJ_TEST("ConnectStream throws when the socket is already disconnected")
 KJ_TEST("ConnectStream defers disconnect failure to the first IPC request for interfaces without construct()")
 {
     TestSetup setup;
+    auto [client_fd, server_fd] = SocketPair();
 
-    close(setup.server_fd);
+    close(server_fd);
 
     // Without a construct() method no IPC call is made during client
     // creation, so ConnectStream succeeds even though the peer is gone.
-    auto foo = ConnectStream<messages::FooInterface>(*setup.loop, MakeStream(*setup.loop, setup.client_fd));
+    auto foo = ConnectStream<messages::FooInterface>(*setup.loop, MakeStream(*setup.loop, client_fd));
 
     try {
         foo->add(1, 2);
@@ -157,10 +141,11 @@ KJ_TEST("ConnectStream handles a disconnect when no client calls are made")
         }
         DefaultLogHandler(log);
     });
+    auto [client_fd, server_fd] = SocketPair();
 
-    close(setup.server_fd);
+    close(server_fd);
 
-    auto foo = ConnectStream<messages::FooInterface>(*setup.loop, MakeStream(*setup.loop, setup.client_fd));
+    auto foo = ConnectStream<messages::FooInterface>(*setup.loop, MakeStream(*setup.loop, client_fd));
 
     // The disconnect handler registered by ProxyClientBase should run and
     // delete the connection even when no calls are ever made.
@@ -171,20 +156,21 @@ KJ_TEST("ConnectStream handles a disconnect when no client calls are made")
 KJ_TEST("ConnectStream throws when the socket disconnects after receiving data")
 {
     TestSetup setup;
+    auto [client_fd, server_fd] = SocketPair();
 
-    std::thread server_thread([&setup]() {
+    std::thread server_thread([&]() {
         char buf[128];
 
         ssize_t bytes_received =
-            recv(setup.server_fd, buf, sizeof(buf), 0);
+            recv(server_fd, buf, sizeof(buf), 0);
 
         if (bytes_received > 0) {
-            close(setup.server_fd);
+            close(server_fd);
         }
     });
 
     try {
-        auto init = ConnectStream<messages::FooInit>(*setup.loop, MakeStream(*setup.loop, setup.client_fd));
+        auto init = ConnectStream<messages::FooInit>(*setup.loop, MakeStream(*setup.loop, client_fd));
 
         if (server_thread.joinable()) server_thread.join();
         KJ_EXPECT(false);
@@ -199,16 +185,14 @@ KJ_TEST("ConnectStream throws when the socket disconnects after receiving data")
 KJ_TEST("ConnectStream throws when a connection accepted from a listener disconnects after receiving data")
 {
     UnixListener listener;
+    TestSetup setup;
+    int client_fd = listener.MakeConnectedSocket();
+    int server_fd = listener.release();
 
-    TestSetup setup([&listener](int fds[2]) {
-        fds[0] = listener.MakeConnectedSocket(); // client_fd
-        fds[1] = listener.release();             // server_fd
-    });
-
-    std::thread server_thread([&setup]() {
+    std::thread server_thread([&]() {
         char buf[128];
 
-        int connection_fd = accept(setup.server_fd, nullptr, nullptr);
+        int connection_fd = accept(server_fd, nullptr, nullptr);
 
         if (connection_fd >= 0) {
             ssize_t bytes_received =
@@ -218,11 +202,11 @@ KJ_TEST("ConnectStream throws when a connection accepted from a listener disconn
                 close(connection_fd);
             }
         }
-        close(setup.server_fd);
+        close(server_fd);
     });
 
     try {
-        auto init = ConnectStream<messages::FooInit>(*setup.loop, MakeStream(*setup.loop, setup.client_fd));
+        auto init = ConnectStream<messages::FooInit>(*setup.loop, MakeStream(*setup.loop, client_fd));
 
         if (server_thread.joinable()) server_thread.join();
         KJ_EXPECT(false);

[xyzconstant]

Yeah, you're right, having them was indeed unnecessary. Addressed this at 466afd0 (follow-up PR).

Thanks for the diff!

[ryanofsky]

In commit "Add test coverage for ConnectStream" (44d1914)

I don't think it makes sense to call joinable here and to repeat this same line. It would make more sense to simply call server_thread.join() unconditionally at the end of this function after the try/catch.

Same comment also applies to test below

[xyzconstant]

Nice catch. Addressed at 24f17bd (follow-up PR).

[ryanofsky]

In commit "Add test coverage for ConnectStream" (44d1914)

Might be good to note in a comment that FooInit capnproto interface has a construct method, so this is not just connecting to the IPC server, but also testing that the construct IPC request completes successfully.

[xyzconstant]

Nice, done at 76ab72f (follow-up PR).

[ryanofsky]

In commit "Fix error handling when creating clients" (bb47369)

Would be good to use KJ_SYSCALL here and below to check close return value

[xyzconstant]

Done at 2977072 (follow-up PR).

[ryanofsky]

In commit "Fix error handling when creating clients" (bb47369)

It would be helpful if commit message noted the reason existing tests in this commit are changing, that because the onDisconnect call now happens later, client code might detect disconnects happening during IPC calls instead of before them.

[xyzconstant]

Sorry, couldn't reword the commit in time. Thanks for the feedback, though.

[ryanofsky]

In commit "Fix error handling when creating clients" (bb47369)

I'm confused about this change. I don't understand if disconnects are detected later now, why the "interrupted by disconnect" message would change to an "interrupted after connect". Would be good to have a more specific comment explaining this, or possibly reverting the change here if not necessary.

EDIT: Reason for this changes seems to be that test.cpp does not use ConnectStream so no disconnect handler was ever registered previously, and now one is registed since the disconnect handler was moved to ProxyClientBase. Would be helpful if commit message mentioned that as reason this test is changing.

[xyzconstant]

Thanks for the review @ryanofsky!

Unfortunately, I couldn't address your feedback in time before this PR was merged. Opened a follow-up PR to address your suggestions: #344.

I replied to some of your inline comments with links to the commit in the follow-up PR that tackles it.