On Wed, Oct 23, 2024 at 01:23:14PM -0400, Taylor Blau wrote:
> 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
