Hello,
While reviewing the pg_ctl CreateProcess patch [1], I started looking
into handle inheritance on Windows and found something that puzzles me.
I think there's an issue with O_CLOEXEC, but wanted to walk through my
reasoning to make sure I'm not missing something obvious.
[1]
https://www.postgresql.org/message-id/flat/tyapr01mb586654e2d74b838021be77caf5...@tyapr01mb5866.jpnprd01.prod.outlook.com
The issue appears to be that O_CLOEXEC doesn't actually do anything on
Windows. When PostgreSQL opens a WAL file in xlog.c, it specifies
O_CLOEXEC in the OpenTransientFile() call, expecting the file handle to
be non-inheritable. However, O_CLOEXEC is defined as 0 in win32_port.h,
and our pgwin32_open() function in src/port/open.c unconditionally sets
sa.bInheritHandle = TRUE at line 80. So the flag is simply ignored, and
all file handles are created inheritable.
Now, for a handle to actually be inherited by a child process on
Windows, two conditions must both be true. First, the handle itself must
have been created with bInheritHandle = TRUE (which we do for
everything). Second, the parent must call CreateProcess with
bInheritHandles = TRUE. So the question becomes: does that second
condition ever happen?
It does. When archive_command runs, PostgreSQL calls pgwin32_system(),
which wraps the command in quotes and passes it to the Microsoft C
runtime's system() function. That function needs to make stdio work for
the child process, so it has no choice but to call CreateProcess with
bInheritHandles = TRUE. This means cmd.exe inherits all our file
handles, including any open database or WAL files, and cmd.exe passes
them along to copy.exe or whatever command is being run.
I wrote a test program that links against libpgport.a to verify this
behavior. It opens files with and without O_CLOEXEC using the actual
pgwin32_open() function, then spawns a child process with
bInheritHandles = TRUE (mimicking what system() does) and tries to
access the handles from the child. Both files were accessible from the
child process regardless of whether O_CLOEXEC was specified. The flag
has no effect.
Commit 1da569ca1f (March 2023) added O_CLOEXEC to many call sites
throughout the backend with a comment saying "Our open() replacement
does not create inheritable handles, so it is safe to ignore
O_CLOEXEC." But that doesn't appear to match what the code actually
does. I'm wondering if I've misunderstood something about how handle
inheritance works on Windows, or if the comment was based on a
misunderstanding of the code path.
The practical impact of this seems low. Child processes spawned by
archive_command or COPY PROGRAM operate on file paths passed as
arguments, not on inherited file descriptors, so they don't actually
make use of the handles even though they have them. Even if a child
wanted to use an inherited handle, it would need to somehow learn the
numeric handle value, which isn't passed through our IPC mechanisms.
And Windows users probably employ archive_command less frequently than
Unix users anyway.
Nonetheless, if this is really a bug, it's a correctness issue. It
violates the documented semantics of O_CLOEXEC, contradicts what our
own comments claim, and means PostgreSQL behaves differently on Windows
than on Unix. It also creates unnecessary handle leaks where files
can't be deleted or renamed while child processes are running. While
reviewing my pg_ctl patch, I realized it would make handle inheritance
more explicit and direct, which made me want to understand whether
O_CLOEXEC actually works.
The fix would be straightforward if this is indeed wrong. Define
O_CLOEXEC to a non-zero value like 0x80000 (in the private use range
for open() flags), and then honor it in pgwin32_open() by setting
sa.bInheritHandle based on whether the flag is present:
sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE;
We also have to add the O_CLOEXEC to the assertion in open.c that
validates that fileFlags only contains known flags.
I've tested this change locally and it works as expected. Files opened
with O_CLOEXEC are not accessible from child processes, while files
opened without it are accessible.
So my questions are: Am I correct that both conditions for handle
inheritance are met, meaning handles really are being inherited by
archive_command children? Is there something in Windows that prevents
inheritance that I don't know about? If this is a real bug, would it
make sense to backpatch to v16 where O_CLOEXEC was added? I'm happy to
provide my test code or do additional testing if that would help.
For reference, the Microsoft documentation on handle inheritance is at:
https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa
And I confirmed through research that UCRT's system() does use
bInheritHandles = TRUE, which makes sense since it needs stdio to work.
Bryan Green
From 095dd7af43c81f55270db71798ee2f0248f1086e Mon Sep 17 00:00:00 2001
From: Bryan Green <[email protected]>
Date: Fri, 31 Oct 2025 10:11:30 -0600
Subject: [PATCH] Fix O_CLOEXEC flag handling in Windows port.
PostgreSQL's src/port/open.c has always set bInheritHandle = TRUE
when opening files on Windows, making all file descriptors inheritable
by child processes. This meant the O_CLOEXEC flag, added to many call
sites by commit 1da569ca1f, was silently ignored.
The original commit included a comment suggesting that our open()
replacement doesn't create inheritable handles, but it appears this
may have been based on a misunderstanding of the code path. In
practice, the code was creating inheritable handles in all cases.
This hasn't caused widespread problems because most child processes
(archive_command, COPY PROGRAM, etc.) operate on file paths passed as
arguments rather than inherited file descriptors. Even if a child
wanted to use an inherited handle, it would need to learn the numeric
handle value, which isn't passed through our IPC mechanisms.
Nonetheless, the current behavior is wrong. It violates documented
O_CLOEXEC semantics, contradicts our own code comments, and makes
PostgreSQL behave differently on Windows than on Unix. It also creates
potential issues with future code or security auditing tools.
To fix, define O_CLOEXEC to a nonzero value (0x80000, in the private
use range for open() flags). Then honor it in pgwin32_open_handle()
by setting bInheritHandle according to whether O_CLOEXEC is specified.
We set bInheritHandle directly in SECURITY_ATTRIBUTES rather than using
SetHandleInformation() afterwards, as the former is simpler.
Back-patch to v16 where commit 1da569ca1f introduced widespread use of
O_CLOEXEC. While the practical risk is low, having code work as
documented and consistently across platforms is worth the minimal risk.
Bug found while reviewing an unrelated patch to pg_ctl's process
spawning logic.
Author: Bryan Green <[email protected]>
---
src/include/port/win32_port.h | 2 +-
src/port/open.c | 13 +++++++++++--
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index ff7028bdc8..07fece6c46 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -346,7 +346,7 @@ extern int _pglstat64(const char *name, struct stat *buf);
* ignore O_CLOEXEC. (If we were using Windows' own open(), it might be
* necessary to convert this to _O_NOINHERIT.)
*/
-#define O_CLOEXEC 0
+#define O_CLOEXEC 0x80000
/*
* Supplement to <errno.h>.
diff --git a/src/port/open.c b/src/port/open.c
index 4a31c5d7b7..71344b9917 100644
--- a/src/port/open.c
+++ b/src/port/open.c
@@ -74,13 +74,22 @@ pgwin32_open_handle(const char *fileName, int fileFlags,
bool backup_semantics)
/* Check that we can handle the request */
assert((fileFlags & ((O_RDONLY | O_WRONLY | O_RDWR) | O_APPEND |
(O_RANDOM | O_SEQUENTIAL |
O_TEMPORARY) |
- _O_SHORT_LIVED | O_DSYNC |
O_DIRECT |
+ _O_SHORT_LIVED | O_DSYNC |
O_DIRECT | O_CLOEXEC |
(O_CREAT | O_TRUNC | O_EXCL) |
(O_TEXT | O_BINARY))) == fileFlags);
sa.nLength = sizeof(sa);
- sa.bInheritHandle = TRUE;
sa.lpSecurityDescriptor = NULL;
+ /*
+ * If O_CLOEXEC is specified, create a non-inheritable handle.
Otherwise,
+ * create an inheritable handle (the default Windows behavior).
+ *
+ * Note: We could instead use SetHandleInformation() after CreateFile()
to
+ * clear HANDLE_FLAG_INHERIT, but setting bInheritHandle=FALSE is
simpler
+ * and achieves the same result.
+ */
+ sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE;
+
while ((h = CreateFile(fileName,
/* cannot use O_RDONLY, as it == 0 */
(fileFlags & O_RDWR) ?
(GENERIC_WRITE | GENERIC_READ) :
--
2.46.0.windows.1
