| Message ID | 907576d23d1dc39b679a323e74b6bfb227d6c17d.1729695349.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | compat/mingw: implement POSIX-style atomic renames | expand |
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.
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. Thanks, Taylor
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. Heh. mingw_open_existing() passes FILE_SHARE_DELETE because you added it. Can you tell which branch I'm 'git grep'-ing on? ;-) Thanks, Taylor
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
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
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. -- Hannes
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
diff --git a/compat/mingw.c b/compat/mingw.c index 0e851ecae29..e326c6fcd2d 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -502,7 +502,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) * to append to the file. */ handle = CreateFileW(wfilename, FILE_APPEND_DATA, - FILE_SHARE_WRITE | FILE_SHARE_READ, + FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE, NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); if (handle == INVALID_HANDLE_VALUE) { DWORD err = GetLastError(); @@ -1006,7 +1006,7 @@ int mingw_utime (const char *file_name, const struct utimbuf *times) osfilehandle = CreateFileW(wfilename, FILE_WRITE_ATTRIBUTES, - 0 /*FileShare.None*/, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_EXISTING, (attrs != INVALID_FILE_ATTRIBUTES &&
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> --- compat/mingw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)