Add nonunix platform support #274

Is this declaration proposital? It has no definition anywhere

re: #274 (review)

In commit "util, refactor: Fix PtrOrValue constructor for move-only types on MSVC" (1060a95de217025f4ea0a7f56a3936aec57834c1)

Is this declaration proposital? It has no definition anywhere

Good catch. No this declaration is just a mistake accidentally to this commit. Will remove

In 1060a95de217025f4ea0a7f56a3936aec57834c1 util, refactor: Fix PtrOrValue constructor for move-only types on MSVC: this seems unrelated?

re: #274 (review)

Removed now

re: #274 (review)

In 1060a95 util, refactor: Fix PtrOrValue constructor for move-only types on MSVC: this seems unrelated?

Thanks, dropped this

Now that this function does fork() + execvp() rather than just execvp, is ExecProcess still the intended name?

re: #274 (review)

In commit "util, refactor: Handle forking inside ExecProcess" (b16f8c4b47ba80088a26e62c5251d0a88b0d45cb)

Now that this function does fork() + execvp() rather than just execvp, is ExecProcess still the intended name?

Good point, I don't think an ExecProcess is a bad name but it could be misleading, and something like RunProcess could be better.

Looking at this again, it was a pretty minor nit. ExecProcess is a fine name

re: #274 (review)

Looking at this again, it was a pretty minor nit. ExecProcess is a fine name

Wound up renaming to StartProcess, which I think should be an improvement

StartSpawned reads as imperative, but the body just parses an int out of the connect-info string (and the header comment seems to describe more work than the function actually does). Would something like ParseConnectInfo fit better?

I might be missing the full picture here.

Yes, I was missing the full picture. Just looked PR 231

re: #274 (review)

the header comment seems to describe more work than the function actually does

Yes, you linked to the relevant change, but just to answer the question, while on unix this only needs to parse the descriptor number, on windows it needs to do more and read a socket descriptor over a pipe.

In 36c91a0c73d5fc5ac616c43ed2a0fdfdea115b81 util, refactor: Add ProcessId type alias and use it: the commit message could mention that Windows uses a different underlying type.

re: #274 (review)

In 36c91a0 util, refactor: Add ProcessId type alias and use it: the commit message could mention that Windows uses a different underlying type.

Thanks, added this to commit message

In 94af41bb55b773787ad1cf195c3b65a68568a1f6 util, refactor: Add SocketId type alias and use it: SocketError is unused in this commit. Maybe do:

diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h
index d7b9f0e546..cae4ac08e9 100644
--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -310,8 +310,8 @@ public:

     //! Pipe read handle used to wake up the event loop thread.
-    int m_wait_fd = -1;
+    SocketId m_wait_fd = SocketError;

     //! Pipe write handle used to wake up the event loop thread.
-    int m_post_fd = -1;
+    SocketId m_post_fd = SocketError;

     //! Number of clients holding references to ProxyServerBase objects that
diff --git a/src/mp/proxy.cpp b/src/mp/proxy.cpp
index eb0e841291..4783e1f110 100644
--- a/src/mp/proxy.cpp
+++ b/src/mp/proxy.cpp
@@ -217,6 +217,6 @@ EventLoop::~EventLoop()
     KJ_ASSERT(m_post_fn == nullptr);
     KJ_ASSERT(!m_async_fns);
-    KJ_ASSERT(m_wait_fd == -1);
-    KJ_ASSERT(m_post_fd == -1);
+    KJ_ASSERT(m_wait_fd == SocketError);
+    KJ_ASSERT(m_post_fd == SocketError);
     KJ_ASSERT(m_num_clients == 0);

@@ -269,6 +269,6 @@ void EventLoop::loop()
     KJ_SYSCALL(::close(post_fd));
     const Lock lock(m_mutex);
-    m_wait_fd = -1;
-    m_post_fd = -1;
+    m_wait_fd = SocketError;
+    m_post_fd = SocketError;
     m_async_fns.reset();
     m_cv.notify_all();

Although IIUC this will go away in a later commit.

re: #274 (review)

In 94af41b util, refactor: Add SocketId type alias and use it: SocketError is unused in this commit. Maybe do:

Thanks, applied patch!

Although IIUC this will go away in a later commit.

Also true, but seems nice to be complete here

In beaa50a046190277a91cdf9ca87c169fd2516c55 util, refactor: Add ConnectInfo type alias and use it: maybe call it SpawnConnectInfo?

re: #274 (review)

In beaa50a util, refactor: Add ConnectInfo type alias and use it: maybe call it SpawnConnectInfo?

Makes sense, renamed

In b16f8c4b47ba80088a26e62c5251d0a88b0d45cb util, refactor: Handle forking inside ExecProcess: I think you still need some sort of warning because the execvp failed branch in the child is not async-signal-safe.

re: #274 (review)

In b16f8c4 util, refactor: Handle forking inside ExecProcess: I think you still need some sort of warning because the execvp failed branch in the child is not async-signal-safe.

Agree there is an issue here, although the warning doesn't really help because it is telling the caller to do something it has no control over, and the function itself is breaking the safety rule internally. Fixed by switching to posix_spawn.

In b16f8c4 util, refactor: Handle forking inside ExecProcess: this comment shouldn't be dropped, as it's a warning against using this in run time without dealing get signal-safety issue above first.

re: #274 (review)

In b16f8c4 util, refactor: Handle forking inside ExecProcess: this comment shouldn't be dropped, as it's a warning against using this in run time without dealing get signal-safety issue above first.

Note this review comment seems to be on same lines of code as #274 (review), so I think this is a duplicate suggestion

In 022b29b776759a246296cad578fbde97d47149a3 util, refactor: Add SocketPair() and use it in SpawnProcess: changing throw std::system_error and != 0 check, to KJ_SYSCALL seems fine, but still a slight behavior change?

re: #274 (review)

In 022b29b util, refactor: Add SocketPair() and use it in SpawnProcess: changing throw std::system_error and != 0 check, to KJ_SYSCALL seems fine, but still a slight behavior change?

Thanks, noted in commit message

In 24c5e57fddec43c3e46fff36f93fe5f9f35d05bd util: Clear FD_CLOEXEC on child socket before exec: to not make things more non-fork-safe:

diff --git a/src/mp/util.cpp b/src/mp/util.cpp
index 15400215e4..b2a545ef5f 100644
--- a/src/mp/util.cpp
+++ b/src/mp/util.cpp
@@ -62,4 +62,12 @@ size_t MaxFd()
 }

+template <size_t N>
+[[noreturn]] void ChildProcessError(const char (&msg)[N])
+{
+    const ssize_t writeResult = ::write(STDERR_FILENO, msg, N - 1);
+    (void)writeResult;
+    _exit(126);
+}
+
 } // namespace

@@ -144,8 +152,5 @@ std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_
             throw std::system_error(errno, std::system_category(), "close");
         }
-        static constexpr char msg[] = "SpawnProcess(child): close(fds[1]) failed\n";
-        const ssize_t writeResult = ::write(STDERR_FILENO, msg, sizeof(msg) - 1);
-        (void)writeResult;
-        _exit(126);
+        ChildProcessError("SpawnProcess(child): close(fds[1]) failed\n");
     }

@@ -164,8 +169,12 @@ std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_
         // the socket was created.
         int flags = fcntl(fds[0], F_GETFD);
-        if (flags == -1) throw std::system_error(errno, std::system_category(), "fcntl F_GETFD");
+        if (flags == -1) {
+            ChildProcessError("SpawnProcess(child): fcntl(fds[0], F_GETFD) failed\n");
+        }
         if (flags & FD_CLOEXEC) {
             flags &= ~FD_CLOEXEC;
-            if (fcntl(fds[0], F_SETFD, flags) == -1) throw std::system_error(errno, std::system_category(), "fcntl F_SETFD");
+            if (fcntl(fds[0], F_SETFD, flags) == -1) {
+                ChildProcessError("SpawnProcess(child): fcntl(fds[0], F_SETFD) failed\n");
+            }
         }

Was there something in particular that led you this (hardening) commit?

re: #274 (review)

In 24c5e57 util: Clear FD_CLOEXEC on child socket before exec: to not make things more non-fork-safe:

Good catch. An easier to way to make it safe was just to move the CLOEXEC code before the fork() call so now doing that

re: #274 (review)

Was there something in particular that led you this (hardening) commit?

I don't remember exactly. It was definitely a bug of some kind. I think an earlier version of the code was using newTwoWayPipe which wraps a socket pair and sets CLOEXEC. Also originally SpawnProcess required callers to construct the socket pair, taking the child socket as an argument instead of both sockets. But this made calling code more complicated, so now SpawnProcess returns a parent socket instead of accepting a child socket.

Now since SpawnProcess is creating the socket pair it knows whether CLOEXEC is set so this code is not necessary. But I think it's good to use CLOEXEC to prevent accidental leaks so I did keep some of this and integrated it better with the earlier commit introducing SocketPair.

In 091f5e16dcd9f08d92aafbae4feee03566d0041e proxy, refactor: Change ConnectStream and ServeStream to accept stream objects: would it make sense to introduce this earlier, in 3c81cf27ead7add93c69a86ffb97bb51b9fe799a?

re: #274 (review)

In 091f5e1 proxy, refactor: Change ConnectStream and ServeStream to accept stream objects: would it make sense to introduce this earlier, in 3c81cf2?

It could make sense but I feel it's a little clearer if the EventLoop code is using kj::Own<kj::AsyncIoStream> and kj::Own<kj::OutputStream> types directly and not tied to the mp::Stream type for external callers and meant to be more opaque.

But I did extend this commit to use Stream stream type in ConnectStream and ServeStream functions since these are external functions.

In 7cb83a5d53476585c12dccabf84cc56294f64c5e cmake: Fix CapnProto tool paths broken by Ubuntu Noble packaging bug: StreamSocketId is unused and stays that way? Given that it's potentially unsafe to throw, maybe it's better to drop this?

re: #274 (review)

In 7cb83a5 cmake: Fix CapnProto tool paths broken by Ubuntu Noble packaging bug: StreamSocketId is unused and stays that way? Given that it's potentially unsafe to throw, maybe it's better to drop this?

Now removed. I think this was used previously when code was using newTwoWayPipe to create streams and using StreamSocketId to turn them into socket ids. Now it does the opposite calling SocketPair starting from socket ids, and calling MakeStream to turn them into streams.

Probably will not go back to the previous approach since it made SpawnProcess more complicated as described #274 (review), so ok to drop StreamSocketId.

There are some reasons why StreamSocketId might be useful: it is an inverse to the MakeStream function, and could be used in the EventLoop constructor to assign m_post_writer. But there isn't a need for it now.

In bfc2db7b517e59e2413b8920341d320bb1228dd2 proxy: Call shutdownWrite() in Connection destructor: maybe squash 28e4c7fd2eb992d632a3b394eab30d3e236208ca into this?

re: #274 (review)

In bfc2db7 proxy: Call shutdownWrite() in Connection destructor: maybe squash 28e4c7f into this?

Didn't squash these because I think 28e4c7fd2eb992d632a3b394eab30d3e236208ca is good documentation of the macos REDUCE_EXPORTS bug and will be probably be useful to reference in the future (the bug can happen very easily just by throwing and catching exceptions.)

In ddd3945be422d3b2addd2b25778eaff01ba41923 util, refactor: Do not fork() and exec() separately: not posix_spawnp?

In d9a5a68872167dd1bb0412d1174dcee694182527 util, refactor: Add SocketPair() and use it in SpawnProcess: it seems a bit odd to set FD_CLOEXEC and then unset it again for the child. The unset code also assume it was the only flag, since it sets 0. Maybe it's better to give this function a boolean argument for whether to set this flag on the child?