Re: [PATCH] Fix socket handle inheritance on Windows

On 11/6/25 19:03, Thomas Munro wrote:
> On Fri, Nov 7, 2025 at 7:53 AM Bryan Green <dbryan(dot)green(at)gmail(dot)com> wrote:
>>> The socket fix adds WSA_FLAG_NO_HANDLE_INHERIT to WSASocket() in
>>> pgwin32_socket(), and calls SetHandleInformation() in
>>> BackendInitialize() to mark the inherited client socket non-inheritable.
>>> The latter is needed because handles passed to children become
>>> inheritable again on Windows.
>
> Right, this is a problem too and your analysis makes sense.
>
> - s = WSASocket(af, type, protocol, NULL, 0, WSA_FLAG_OVERLAPPED);
> + s = WSASocket(af, type, protocol, NULL, 0,
> + WSA_FLAG_OVERLAPPED | WSA_FLAG_NO_HANDLE_INHERIT);
>
> I wonder if we should control this with a new be-more-like-Unix
> SOCK_CLOEXEC flag. In practice we might never want sockets created
> this way to be inherited across CreateProcess, so we might always pass
> SOCK_CLOEXEC in places like libpq (we already do that). But in theory
> at least, someone might have a reason to want an inheritable socket
> (no doubt with some complications), and it seems attractive to work
> the same way cross-platform and also mirror the O_CLOEXEC behaviour
> your other thread fixes, and I'm also looking further ahead to the
> case of accepted sockets that have some additional needs (see below),
> so it might be good to be explicit and consistent about the flags.
>
> +#ifdef WIN32
> + /*
> + * On Windows, the client socket inherited from the postmaster becomes
> + * inheritable again in this process. Prevent child processes spawned
> + * by this backend from inheriting it.
> + */
> + if (!SetHandleInformation((HANDLE) port->sock, HANDLE_FLAG_INHERIT, 0))
> + ereport(WARNING,
> + (errmsg_internal("could not disable socket handle
> inheritance: error code %lu",
> + GetLastError())));
> +#endif
>
> On Unix we also need the equivalent, but it's in pq_init():
>
> #ifndef WIN32
>
> /* Don't give the socket to any subprograms we execute. */
> if (fcntl(port->sock, F_SETFD, FD_CLOEXEC) < 0)
> elog(FATAL, "fcntl(F_SETFD) failed on socket: %m");
> #endif
>
> Would it make sense to have a socket_set_cloexec() function, following
> the example of socket_set_nonblocking(), so we could harmonise the
> calling code?
>

Yes, I think it would.

> I think we'll also need an accept4() function for Windows that accepts
> SOCK_CLOEXEC and converts it to WSA_FLAG_NO_HANDLE_INHERIT, now that
> you've pointed this out. It's not needed for your problem report
> here, and doesn't even make sense for EXEC_BACKEND by definition, but
> I think we'll probably want it for multithreaded PostgreSQL on
> Windows. With backends as threads, at least in one experimental
> architecture with listener in the main/only sub-postmaster process,
> there is a race window between ye olde accept() and
> socket_set_cloexec() that leaks handles if a subprocess is created
> concurrently by any thread. Adopting accept4() is pretty trivial
> (something like v5-0001[1]), it just wasn't quite compelling enough to
> commit before multithreading started heating up as a topic. I hadn't
> previously grokked that Windows will need to be able to reach its
> equivalent flag too for the reason you've pointed out.
>

I will look over [1].

> I mention that as context for my suggestion that we might want an
> explicit SOCK_CLOEXEC flag, because it finishes up being conditional
> in the accept4() case assuming that design: multi-process mode needs
> it except in EXEC_BACKEND builds which have to call
> socket_set_cloexit() in the exec'd child by definition, while
> in-development multi-threaded mode needs it on all platforms. (The
> fact that macOS alone has so far refused to implement it is a bit
> annoying, and potential workarounds are expensive, but that's a topic
> for another day.)
>

If you agree, and I think you do, I will implement the SOCK_CLOEXEC
abstraction and the socket_set_cloexec helper for this. My bug fix will
be the first user.

I also need to handle the handles for shared memory and pipes. I will
probably just write those up tonight and share for review and discussion.

> [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGKPNFcfBQduqof4-7C%3DavjcSfdkKBGvQoRuAvfocnvY0A%40mail.gmail.com#abe1bf9def93e897827e878aac8a6945

Thanks for your excellent review and suggestions.

--
Bryan Green
EDB: https://www.enterprisedb.com