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

[Taylor Blau <me@ttaylorr.com>]

On Sun, Oct 27, 2024 at 02:14:30PM +0100, Johannes Sixt wrote:
> Am 23.10.24 um 19:23 schrieb Taylor Blau:
> > 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.
>
> My attitude in the past was that deleting a file that is open elsewhere
> is a bug, so FILE_SHARE_DELETE would not be needed, but its absence
> could point to a bug elsewhere. Now that we have a reftable
> implementation, it looks like I can't uphold this attitude anymore.

Makes sense.

Thanks,
Taylor