Thread

Re: [PATCH] O_CLOEXEC not honored on Windows - handle inheritance chain

Bryan Green <dbryan.green@gmail.com> — 2025-11-07T17:28:48Z

On 11/6/2025 8:42 AM, Bryan Green wrote:
> On 11/6/2025 7:43 AM, Thomas Munro wrote:
>> On Sat, Nov 1, 2025 at 6:16 AM Bryan Green <dbryan.green@gmail.com> wrote:
>>> Hello,
>>
>> Catching up with all your emails, and I must say it's great to see
>> some solid investigation of PostgreSQL-on-Windows problems.  There are
>> ... more.
>>
>>> 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.
>>
>> Yeah, it looks like I was just wrong.  Oops.  Your analysis looks good to me.
>>
>>> 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;
>>
>> Looking at fcntl.h, that's the next free bit, but also the one they'll
>> presumably define next (I guess "private use range" is just a turn of
>> phrase and not a real thing?), so why not use the highest free bit
>> after O_DIRECT?  We have three fake open flags, one of which
>> cybersquats a real flag from fcntl.h, ironically the one that actually
>> means O_CLOEXEC.  We can't change existing values in released
>> branches, so that'd give:
>>
>> #define     O_DIRECT    0x80000000
>> #define     O_CLOEXEC   0x04000000
>> #define     O_DSYNC     _O_NO_INHERIT
>>
>> Perhaps in master we could rearrange them:
>>
>> #define     O_DIRECT    0x80000000
>> #define     O_DSYNC     0x04000000
>> #define     O_CLOEXEC   _O_NO_INHERIT
>>
>>> 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.
>>
>> Yeah, seems like it, and like we should back-patch this.  I vote for
>> doing that after the upcoming minor releases.  Holding files open on
>> Windows unintentionally is worse on Windows than on Unix (preventing
>> directories from being unlinked etc).  Of course we've done that for
>> decades so I doubt it's really a big deal, but we should clean up this
>> mistake.
> 
> Thanks for reviewing this and confirming the analysis. Good to know I
> wasn't missing something about Windows handle inheritance.
> 
> Your point about the bit value makes sense - using 0x04000000 (highest
> free bit after O_DIRECT) is definitely safer than 0x80000 which could
> collide with future fcntl.h additions. I also appreciate the irony you
> pointed out - we're currently using _O_NO_INHERIT (which literally
> prevents handle inheritance on Windows) for O_DSYNC instead of
> O_CLOEXEC. The rearrangement in master to use _O_NO_INHERIT for what it
> actually means semantically makes a lot of sense.
> 
> So the plan would be:
> 
> Backport branches (v16+):
> #define O_DIRECT    0x80000000
> #define O_CLOEXEC   0x04000000
> #define O_DSYNC     _O_NO_INHERIT
> 
> Master:
> #define O_DIRECT    0x80000000
> #define O_DSYNC     0x04000000
> #define O_CLOEXEC   _O_NO_INHERIT
> 
> And then in pgwin32_open():
>     sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE;
> 
> I will prepare a new version of the patch that implements the suggested
> change for master.
> 
> 
The changes for master, along with a tap test, are provided with the
attached patch.

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