Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 24, 2024, 06:30 UTC
Message-ID: <ZxnpmPbKDNiQl66p@pks.im>
In-Reply-To: <ZxkxAvLeMtxstVdS@nand.local>

On Wed, Oct 23, 2024 at 01:23:14PM -0400, Taylor Blau wrote:

Show 30 quoted lines

> On Wed, Oct 23, 2024 at 05:04:58PM +0200, Patrick Steinhardt wrote:
> > Unless told otherwise, Windows will keep other processes from reading,
> > writing and deleting files when one has an open handle that was created
> > via `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`
> > flags such that other processes _can_ read and/or modify such a file.
> > This sharing mechanism is quite important in the context of Git, as we
> > assume POSIX semantics all over the place.
> >
> > There are two calls where we don't set up those flags though:
> >
> >   - We don't set `FILE_SHARE_DELETE` when creating a file for appending
> >     via `mingw_open_append()`. This makes it impossible to delete the
> >     file from another process or to replace it via an atomic rename.
> >
> >   - When opening a file such that we can change its access/modification
> >     times. This makes it impossible to perform any kind of operation
> >     on this file at all from another process. While we only open the
> >     file for a short amount of time to update its timestamps, this still
> >     opens us up for a race condition with another process.
> >
> > Adapt both of these callsites to pass all three sharing flags.
> 
> Interesting, and especially so noting that we *do* call CreateFileW()
> with the FILE_SHARE_DELETE flag in other functions like create_watch(),
> mingw_open_existing(), mingw_getcwd(), etc.
> 
> Why didn't we call CreateFileW() with FILE_SHARE_DELETE in the below two
> functions? I assume Johannes Schindelin and/or Johannes Sixt (both CC'd)
> would know the answer. Regardless, it would be interesting and useful
> (IMHO) to include such a detail in the commit message.

Hard to tell, but I think it's basically an oversight.

  - `mingw_utime()` was originally implemented via `_wopen()`, which
    doesn't give you full control over the sharing mode. It was then
    refactored via 090a3085bc (t/helper/test-chmtime: update mingw to
    support chmtime on directories, 2022-03-02) to use `CreateFileW()`.
    This refactoring wasn't quite to the old code, because we use no
    sharing flags at all. But in fact, `_wopen()` calls `_wsopen()` with
    `_SH_DENYNO`, which ultimately translates to `FILE_SHARE_READ |
    `FILE_SHARE_WRITE`. So we lost the read/write sharing with that
    conversion.

  - `mingw_open_append()` was introduced via d641097589 (mingw: enable
    atomic O_APPEND, 2018-08-13) and had `FILE_SHARE_READ |
    FILE_SHARE_WRITE` since the beginning.

Why we didn't set `FILE_SHARE_DELETE` is a different question though, and none that I can answer based on the commit messages. I would claim that it's likely an oversight and wasn't done intentionally, but I cannot say for sure.

Anyway, I'll include the above information in the commit message.

Patrick