Thread

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

Bryan Green <dbryan.green@gmail.com> — 2025-11-12T22:17:21Z

On 11/12/2025 4:10 PM, Bryan Green wrote:
> On 11/11/2025 8:34 PM, Thomas Munro wrote:
>> On Wed, Nov 12, 2025 at 2:01 PM Bryan Green <dbryan.green@gmail.com> wrote:
>>> [v3]
>>
>> "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."
>>
>> s/it appears this may have been been/was/ :-)
>>
> 
> Changed.
> 
>> "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()"
>>
> 
> Removed.
>> Out of date, maybe skip mentioning the value in the commit message?
>> Maybe add a note here about the back-branches preserving existing
>> values and master cleaning up?  Do you happen to have a fixup that
>> supplies the difference in the back-branches so we can see it passing
>> in CI?
>>
> 
> I have attached a back-patch for v16-v18.
> 
>> +     * Note: We could instead use SetHandleInformation() after CreateFile() to
>> +     * clear HANDLE_FLAG_INHERIT, but setting bInheritHandle=FALSE is simpler
>> +     * and achieves the same result.
>> +     */
>>
>> It also wouldn't be thread-safe.  That is meaningful today for
>> frontend programs (and hopefully some day soon also in the backend).
>>
>> +    sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE;
>>
>> Just out of sheer curiosity, I often see gratuitous FALSE and TRUE in
>> Windowsian code, not false and true and not reduced to eg !(fileFlags
>> & O_CLOEXEC).  Is that a code convention thing from somewhere in
>> Windows-land?
>>
> 
> Yes, old habits die hard.  I learned this pattern on Windows.
> Interestingly, enough when I am not on Windows I write the way you suggest.
> 
>> +++ b/src/test/modules/test_cloexec/test_cloexec.c
>>
>> I like the test.  Very helpful for people reliant on CI for Windows coverage.
>>
>> Independently of all this, and just on the off-chance that it might be
>> interesting to you in future, I have previously tried to write tests
>> for our whole Windows filesystem shim layer and found lots of bugs and
>> understood lots of quirks that way, but never got it to be good enough
>> for inclusion in the tree:
>>
>> https://www.postgresql.org/message-id/flat/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com
>>
> 
> I shall take a look.
> 
>> There is some overlap with several of your recent patches, as I was
>> testing some of the same wrappers.  One of the main things we've
>> battled with in this project is the whole asynchronous-unlink thing,
>> and generally the NT/VMS file locking concept which can't quite be
>> entirely emulated away, and that was one of my main focuses there
>> after we got CI and started debugging the madness.  Doing so revealed
>> a bunch of interesting differences in Windows versions and file
>> systems, and to this day we don't really have a project policy on
>> which Windows filesystems PostgreSQL supports (cf your mention of
>> needing NTFS for sparse files in one of your other threads, though I
>> can't imagine that ReFS AKA DevDrive doesn't have those too being a
>> COW system).
>>
>> Speaking of file I/O, and just as an FYI, I have a bunch of
>> semi-working unfinished patches that bring true scatter/gather I/O
>> (instead of the simple looping fallback in pg_preadv()) and native
>> async I/O (for files, but actually also pipes and sockets but let me
>> stick to talking about files for now) to Windows (traditional
>> OVERLAPPED and/or IoRing.h, neither can do everything we need would
>> you believe).  Development via trial-by-CI from the safety of my Unix
>> box is slow and painful going, but... let's say a real Windows
>> programmer with a systems programming bent showed up and were
>> interested in this stuff, I would be more than happy to write down
>> everything I think I know about those subjects and post the unfinished
>> work and then I bet development would go fast... just sayin'.
> 
> I would absolutely love to read everything you think you know about
> those subjects and contribute to the work.
> 
> 
Corrected master patch and back patch for v16-v18.

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