Re: File descriptors in exec'd subprocesses
Thomas Munro <thomas.munro@gmail.com>
From: Thomas Munro <thomas.munro@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2023-02-21T03:59:59Z
Lists: pgsql-hackers
Attachments
- v4-0001-Remove-obsolete-coding-for-macOS-10.7.patch (text/x-patch) patch v4-0001
- v4-0002-Don-t-leak-descriptors-into-subprograms.patch (text/x-patch) patch v4-0002
- v4-0003-Use-accept4-to-accept-connections-where-available.patch (text/x-patch) patch v4-0003
- v4-0004-Use-newer-client-socket-options-where-available.patch (text/x-patch) patch v4-0004
- v4-0005-Use-pipe2-for-postmaster-pipe-where-available.patch (text/x-patch) patch v4-0005
Something bothered me about the previous versions: Which layer should add O_CLOEXEC, given that we have md.c -> PathNameOpenXXX() -> BasicOpenFile() -> open()? Previously I had md.c adding it, but on reflection, it makes no sense to open a "File" (virtual file descriptor) that is *not* O_CLOEXEC. The client doesn't really know when the raw descriptor is currently open and shouldn't really access it; it would be strange to want it to survive a call to exec*(). I think the 'highest' level API that we could consider requiring O_CLOEXEC to be passed in explicitly, or not, is BasicOpenFile(). Done like that in this version. This is the version I'm thinking of committing, unless someone wants to argue for another level. Another good choice would be to do it inside BasicOpenFile(), and then the patch would be smaller again (xlog.c wouldn't need to mention it, and there would perhaps be less risk that some long-lived descriptor somewhere else has failed to request it), but perhaps that would be presumptuous. That function returns raw descriptors, and the caller, perhaps an extension, might legitimately want to make an inheritable descriptor for some reason, I guess? Does anyone think I should move it in there instead? I realised that if we're going to use accept4() to cut down on syscalls, we could also do the same for the postmaster pipe with pipe2(). Here also is a tiny archeological cleanup to avoid creating contradictory claims about whether all computers have O_CLOEXEC. I toyed with the idea of a tiny Linux-only regression test using "COPY fds FROM PROGRAM 'ls /proc/self/fd'" expecting 0, 1, 2, 3 (3 being ls's opendir()), but that's probably a little too cute; and also showed me that pg_regress.c leaks its log file, the fix for which is to add "e" to its fdopen(), but that's another POSIX-next feature[1] that seems a little harder to detect, and I gave up on that. [1] https://wiki.freebsd.org/AtomicCloseOnExec
Commits
-
libpq: Use modern socket flags, if available.
- bfc9497ece01 16.0 landed
-
Don't leak descriptors into subprograms.
- 1da569ca1f1f 16.0 landed
-
Remove obsolete coding for early macOS.
- a1f45f69bbfc 16.0 landed
-
Adjust commit 2dbe8905 for ancient macOS.
- 04cad8f7bcd6 15.0 cited