Message-ID: <7f6779d5-7a0e-44c5-a85f-b4ef265db766@gmail.com>
Date: Thu, 6 Nov 2025 08:42:14 -0600
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] O_CLOEXEC not honored on Windows - handle inheritance
 chain
To: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <e2b16375-7430-4053-bda3-5d2194ff1880@gmail.com>
 <CA+hUKGJS=xdaQ6iCTSgvBrf8Cm+a_ZMmDjZYhdQX_srHkhOuqA@mail.gmail.com>
Content-Language: en-US
From: Bryan Green <dbryan.green@gmail.com>
In-Reply-To: 
 <CA+hUKGJS=xdaQ6iCTSgvBrf8Cm+a_ZMmDjZYhdQX_srHkhOuqA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/7f6779d5-7a0e-44c5-a85f-b4ef265db766%40gmail.com>
Precedence: bulk

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