Thread

Re: [PATCH] Fix socket handle inheritance on Windows

Bryan Green <dbryan.green@gmail.com> — 2025-11-07T01:35:29Z

On 11/6/25 19:03, Thomas Munro wrote:
> On Fri, Nov 7, 2025 at 7:53 AM Bryan Green <dbryan.green@gmail.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