Content-Type: multipart/mixed; boundary="------------oYcrfAsCtrwobKP90usSTPHo"
Message-ID: <e2b16375-7430-4053-bda3-5d2194ff1880@gmail.com>
Date: Fri, 31 Oct 2025 11:16:04 -0600
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
From: Bryan Green <dbryan.green@gmail.com>
Subject: [PATCH] O_CLOEXEC not honored on Windows - handle inheritance chain
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/e2b16375-7430-4053-bda3-5d2194ff1880%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------oYcrfAsCtrwobKP90usSTPHo
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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/TYAPR01MB586654E2D74B838021BE77CAF5EEA@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
--------------oYcrfAsCtrwobKP90usSTPHo
Content-Type: text/plain; charset=UTF-8;
 name="0001-Fix-O_CLOEXEC-flag-handling-in-Windows-port.patch"
Content-Disposition: attachment;
 filename="0001-Fix-O_CLOEXEC-flag-handling-in-Windows-port.patch"
Content-Transfer-Encoding: base64

RnJvbSAwOTVkZDdhZjQzYzgxZjU1MjcwZGI3MTc5OGVlMmYwMjQ4ZjEwODZlIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBCcnlhbiBHcmVlbiA8ZGJyeWFuLmdyZWVuQGdtYWls
LmNvbT4KRGF0ZTogRnJpLCAzMSBPY3QgMjAyNSAxMDoxMTozMCAtMDYwMApTdWJqZWN0OiBb
UEFUQ0hdIEZpeCBPX0NMT0VYRUMgZmxhZyBoYW5kbGluZyBpbiBXaW5kb3dzIHBvcnQuCgpQ
b3N0Z3JlU1FMJ3Mgc3JjL3BvcnQvb3Blbi5jIGhhcyBhbHdheXMgc2V0IGJJbmhlcml0SGFu
ZGxlID0gVFJVRQp3aGVuIG9wZW5pbmcgZmlsZXMgb24gV2luZG93cywgbWFraW5nIGFsbCBm
aWxlIGRlc2NyaXB0b3JzIGluaGVyaXRhYmxlCmJ5IGNoaWxkIHByb2Nlc3Nlcy4gIFRoaXMg
bWVhbnQgdGhlIE9fQ0xPRVhFQyBmbGFnLCBhZGRlZCB0byBtYW55IGNhbGwKc2l0ZXMgYnkg
Y29tbWl0IDFkYTU2OWNhMWYsIHdhcyBzaWxlbnRseSBpZ25vcmVkLgoKVGhlIG9yaWdpbmFs
IGNvbW1pdCBpbmNsdWRlZCBhIGNvbW1lbnQgc3VnZ2VzdGluZyB0aGF0IG91ciBvcGVuKCkK
cmVwbGFjZW1lbnQgZG9lc24ndCBjcmVhdGUgaW5oZXJpdGFibGUgaGFuZGxlcywgYnV0IGl0
IGFwcGVhcnMgdGhpcwptYXkgaGF2ZSBiZWVuIGJhc2VkIG9uIGEgbWlzdW5kZXJzdGFuZGlu
ZyBvZiB0aGUgY29kZSBwYXRoLiAgSW4KcHJhY3RpY2UsIHRoZSBjb2RlIHdhcyBjcmVhdGlu
ZyBpbmhlcml0YWJsZSBoYW5kbGVzIGluIGFsbCBjYXNlcy4KClRoaXMgaGFzbid0IGNhdXNl
ZCB3aWRlc3ByZWFkIHByb2JsZW1zIGJlY2F1c2UgbW9zdCBjaGlsZCBwcm9jZXNzZXMKKGFy
Y2hpdmVfY29tbWFuZCwgQ09QWSBQUk9HUkFNLCBldGMuKSBvcGVyYXRlIG9uIGZpbGUgcGF0
aHMgcGFzc2VkIGFzCmFyZ3VtZW50cyByYXRoZXIgdGhhbiBpbmhlcml0ZWQgZmlsZSBkZXNj
cmlwdG9ycy4gIEV2ZW4gaWYgYSBjaGlsZAp3YW50ZWQgdG8gdXNlIGFuIGluaGVyaXRlZCBo
YW5kbGUsIGl0IHdvdWxkIG5lZWQgdG8gbGVhcm4gdGhlIG51bWVyaWMKaGFuZGxlIHZhbHVl
LCB3aGljaCBpc24ndCBwYXNzZWQgdGhyb3VnaCBvdXIgSVBDIG1lY2hhbmlzbXMuCgpOb25l
dGhlbGVzcywgdGhlIGN1cnJlbnQgYmVoYXZpb3IgaXMgd3JvbmcuICBJdCB2aW9sYXRlcyBk
b2N1bWVudGVkCk9fQ0xPRVhFQyBzZW1hbnRpY3MsIGNvbnRyYWRpY3RzIG91ciBvd24gY29k
ZSBjb21tZW50cywgYW5kIG1ha2VzClBvc3RncmVTUUwgYmVoYXZlIGRpZmZlcmVudGx5IG9u
IFdpbmRvd3MgdGhhbiBvbiBVbml4LiAgSXQgYWxzbyBjcmVhdGVzCnBvdGVudGlhbCBpc3N1
ZXMgd2l0aCBmdXR1cmUgY29kZSBvciBzZWN1cml0eSBhdWRpdGluZyB0b29scy4KClRvIGZp
eCwgZGVmaW5lIE9fQ0xPRVhFQyB0byBhIG5vbnplcm8gdmFsdWUgKDB4ODAwMDAsIGluIHRo
ZSBwcml2YXRlCnVzZSByYW5nZSBmb3Igb3BlbigpIGZsYWdzKS4gIFRoZW4gaG9ub3IgaXQg
aW4gcGd3aW4zMl9vcGVuX2hhbmRsZSgpCmJ5IHNldHRpbmcgYkluaGVyaXRIYW5kbGUgYWNj
b3JkaW5nIHRvIHdoZXRoZXIgT19DTE9FWEVDIGlzIHNwZWNpZmllZC4KV2Ugc2V0IGJJbmhl
cml0SGFuZGxlIGRpcmVjdGx5IGluIFNFQ1VSSVRZX0FUVFJJQlVURVMgcmF0aGVyIHRoYW4g
dXNpbmcKU2V0SGFuZGxlSW5mb3JtYXRpb24oKSBhZnRlcndhcmRzLCBhcyB0aGUgZm9ybWVy
IGlzIHNpbXBsZXIuCgpCYWNrLXBhdGNoIHRvIHYxNiB3aGVyZSBjb21taXQgMWRhNTY5Y2Ex
ZiBpbnRyb2R1Y2VkIHdpZGVzcHJlYWQgdXNlIG9mCk9fQ0xPRVhFQy4gIFdoaWxlIHRoZSBw
cmFjdGljYWwgcmlzayBpcyBsb3csIGhhdmluZyBjb2RlIHdvcmsgYXMKZG9jdW1lbnRlZCBh
bmQgY29uc2lzdGVudGx5IGFjcm9zcyBwbGF0Zm9ybXMgaXMgd29ydGggdGhlIG1pbmltYWwg
cmlzay4KCkJ1ZyBmb3VuZCB3aGlsZSByZXZpZXdpbmcgYW4gdW5yZWxhdGVkIHBhdGNoIHRv
IHBnX2N0bCdzIHByb2Nlc3MKc3Bhd25pbmcgbG9naWMuCgpBdXRob3I6IEJyeWFuIEdyZWVu
IDxkYnJ5YW4uZ3JlZW5AZ21haWwuY29tPgotLS0KIHNyYy9pbmNsdWRlL3BvcnQvd2luMzJf
cG9ydC5oIHwgIDIgKy0KIHNyYy9wb3J0L29wZW4uYyAgICAgICAgICAgICAgIHwgMTMgKysr
KysrKysrKystLQogMiBmaWxlcyBjaGFuZ2VkLCAxMiBpbnNlcnRpb25zKCspLCAzIGRlbGV0
aW9ucygtKQoKZGlmZiAtLWdpdCBhL3NyYy9pbmNsdWRlL3BvcnQvd2luMzJfcG9ydC5oIGIv
c3JjL2luY2x1ZGUvcG9ydC93aW4zMl9wb3J0LmgKaW5kZXggZmY3MDI4YmRjOC4uMDdmZWNl
NmM0NiAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUvcG9ydC93aW4zMl9wb3J0LmgKKysrIGIv
c3JjL2luY2x1ZGUvcG9ydC93aW4zMl9wb3J0LmgKQEAgLTM0Niw3ICszNDYsNyBAQCBleHRl
cm4gaW50CV9wZ2xzdGF0NjQoY29uc3QgY2hhciAqbmFtZSwgc3RydWN0IHN0YXQgKmJ1Zik7
CiAgKiBpZ25vcmUgT19DTE9FWEVDLiAgKElmIHdlIHdlcmUgdXNpbmcgV2luZG93cycgb3du
IG9wZW4oKSwgaXQgbWlnaHQgYmUKICAqIG5lY2Vzc2FyeSB0byBjb252ZXJ0IHRoaXMgdG8g
X09fTk9JTkhFUklULikKICAqLwotI2RlZmluZSBPX0NMT0VYRUMgMAorI2RlZmluZSBPX0NM
T0VYRUMgMHg4MDAwMAogCiAvKgogICogU3VwcGxlbWVudCB0byA8ZXJybm8uaD4uCmRpZmYg
LS1naXQgYS9zcmMvcG9ydC9vcGVuLmMgYi9zcmMvcG9ydC9vcGVuLmMKaW5kZXggNGEzMWM1
ZDdiNy4uNzEzNDRiOTkxNyAxMDA2NDQKLS0tIGEvc3JjL3BvcnQvb3Blbi5jCisrKyBiL3Ny
Yy9wb3J0L29wZW4uYwpAQCAtNzQsMTMgKzc0LDIyIEBAIHBnd2luMzJfb3Blbl9oYW5kbGUo
Y29uc3QgY2hhciAqZmlsZU5hbWUsIGludCBmaWxlRmxhZ3MsIGJvb2wgYmFja3VwX3NlbWFu
dGljcykKIAkvKiBDaGVjayB0aGF0IHdlIGNhbiBoYW5kbGUgdGhlIHJlcXVlc3QgKi8KIAlh
c3NlcnQoKGZpbGVGbGFncyAmICgoT19SRE9OTFkgfCBPX1dST05MWSB8IE9fUkRXUikgfCBP
X0FQUEVORCB8CiAJCQkJCQkgKE9fUkFORE9NIHwgT19TRVFVRU5USUFMIHwgT19URU1QT1JB
UlkpIHwKLQkJCQkJCSBfT19TSE9SVF9MSVZFRCB8IE9fRFNZTkMgfCBPX0RJUkVDVCB8CisJ
CQkJCQkgX09fU0hPUlRfTElWRUQgfCBPX0RTWU5DIHwgT19ESVJFQ1QgfCBPX0NMT0VYRUMg
fAogCQkJCQkJIChPX0NSRUFUIHwgT19UUlVOQyB8IE9fRVhDTCkgfCAoT19URVhUIHwgT19C
SU5BUlkpKSkgPT0gZmlsZUZsYWdzKTsKIAogCXNhLm5MZW5ndGggPSBzaXplb2Yoc2EpOwot
CXNhLmJJbmhlcml0SGFuZGxlID0gVFJVRTsKIAlzYS5scFNlY3VyaXR5RGVzY3JpcHRvciA9
IE5VTEw7CiAKKwkvKgorCSAqIElmIE9fQ0xPRVhFQyBpcyBzcGVjaWZpZWQsIGNyZWF0ZSBh
IG5vbi1pbmhlcml0YWJsZSBoYW5kbGUuICBPdGhlcndpc2UsCisJICogY3JlYXRlIGFuIGlu
aGVyaXRhYmxlIGhhbmRsZSAodGhlIGRlZmF1bHQgV2luZG93cyBiZWhhdmlvcikuCisJICoK
KwkgKiBOb3RlOiBXZSBjb3VsZCBpbnN0ZWFkIHVzZSBTZXRIYW5kbGVJbmZvcm1hdGlvbigp
IGFmdGVyIENyZWF0ZUZpbGUoKSB0bworCSAqIGNsZWFyIEhBTkRMRV9GTEFHX0lOSEVSSVQs
IGJ1dCBzZXR0aW5nIGJJbmhlcml0SGFuZGxlPUZBTFNFIGlzIHNpbXBsZXIKKwkgKiBhbmQg
YWNoaWV2ZXMgdGhlIHNhbWUgcmVzdWx0LgorCSAqLworCXNhLmJJbmhlcml0SGFuZGxlID0g
KGZpbGVGbGFncyAmIE9fQ0xPRVhFQykgPyBGQUxTRSA6IFRSVUU7CisKIAl3aGlsZSAoKGgg
PSBDcmVhdGVGaWxlKGZpbGVOYW1lLAogCS8qIGNhbm5vdCB1c2UgT19SRE9OTFksIGFzIGl0
ID09IDAgKi8KIAkJCQkJCSAgIChmaWxlRmxhZ3MgJiBPX1JEV1IpID8gKEdFTkVSSUNfV1JJ
VEUgfCBHRU5FUklDX1JFQUQpIDoKLS0gCjIuNDYuMC53aW5kb3dzLjEKCg==

--------------oYcrfAsCtrwobKP90usSTPHo--