Thread

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

Bryan Green <dbryan.green@gmail.com> — 2025-11-06T14:42:14Z

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.


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