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

[Taylor Blau <me@ttaylorr.com>]

On Wed, Oct 23, 2024 at 06:18:58PM +0200, Kristoffer Haugsbakk wrote:
> On Wed, Oct 23, 2024, at 17:04, 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.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
>
> (Reading back) By default Windows restricts other processes (so these
> files could be in use by other procesess) from reading, writing, and
> deleting them (presumably doing anything it looks like).  But it does
> provide flags if you need these permissions.
>
> There are two calls/places where you need to expand the permissions:
>
> 1. “Delete” for appending: need for deletion or replace via
>    atomic rename
> 2. When opening a file to modify the access/modification metadata.  The
>    current permissions are *likely* to work but you could run into race
>    conditions, so the current set of permissions are buggy.
>
> The commit seems well-explained to this naïve reader.

Thanks for thinking aloud and summarizing your thoughts here. I found
the read-back to be quite useful indeed.

Thanks,
Taylor