Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 23, 2024, 17:25 UTC
- Message-ID
- <Zxkxij1FThn7QoOg@nand.local>
- In-Reply-To
- <b106fb4d-3488-4fef-aca8-32e49efd0124@app.fastmail.com>
On Wed, Oct 23, 2024 at 06:18:58PM +0200, Kristoffer Haugsbakk wrote:
Show 38 quoted lines
> 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