| Message ID | cover.1729770140.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | compat/mingw: implement POSIX-style atomic renames | expand |
On Thu, Oct 24, 2024 at 01:46:08PM +0200, Patrick Steinhardt wrote: > Patrick Steinhardt (3): > compat/mingw: share file handles created via `CreateFileW()` > compat/mingw: allow deletion of most opened files > compat/mingw: support POSIX semantics for atomic renames Thanks, will queue. Can other reviewers chime in if this looks ready to start merging down? Thanks, Taylor
Am 24.10.24 um 13:46 schrieb Patrick Steinhardt: > Hi, > > this is the second patch series that implements POSIX-style atomic > renames on Windows in order to fix concurrent writes with the reftable > backend. > > Changes compared to v1: > > - Added some historic digging to the first commit message. > > - Fix various spelling mistakes. > > - Fix indentation. > > - Don't use the comma operator to assign `errno`. > > Thanks! > > Patrick Thank you for working on this. The patches look good in general, but I noticed that oflags handling in 2/3 needs to be fixed. I ran the test suite on my Windows 10 box with this series, and it passed all tests (with my suggested oflags fix applied). I also cross-checked whether I would observe the failure that the series attempts to fix, and I do see the failure, and the series fixes it. -- Hannes
Hi, this is the second patch series that implements POSIX-style atomic renames on Windows in order to fix concurrent writes with the reftable backend. Changes compared to v1: - Added some historic digging to the first commit message. - Fix various spelling mistakes. - Fix indentation. - Don't use the comma operator to assign `errno`. Thanks! Patrick Patrick Steinhardt (3): compat/mingw: share file handles created via `CreateFileW()` compat/mingw: allow deletion of most opened files compat/mingw: support POSIX semantics for atomic renames compat/mingw.c | 149 +++++++++++++++++++++++++++++++++++-- t/t0610-reftable-basics.sh | 8 +- 2 files changed, 148 insertions(+), 9 deletions(-) Range-diff against v1: 1: 907576d23d1 ! 1: 63c2cfa87a4 compat/mingw: share file handles created via `CreateFileW()` @@ Commit message 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. + flags: + + - `FILE_SHARE_READ` allows a concurrent process to open the file for + reading. - There are two calls where we don't set up those flags though: + - `FILE_SHARE_WRITE` allows a concurrent process to open the file for + writing. + + - `FILE_SHARE_DELETE` allows a concurrent process to delete the file + or to replace it via an atomic rename. + + This sharing mechanism is quite important in the context of Git, as we + assume POSIX semantics all over the place. But there are two callsites + where we don't pass all three of these flags: - 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. + file from another process or to replace it via an atomic rename. The + function was introduced via d641097589 (mingw: enable atomic + O_APPEND, 2018-08-13) and has been using `FILE_SHARE_READ | + FILE_SHARE_WRITE` since the inception. There aren't any indicators + that the omission of `FILE_SHARE_DELETE` was intentional. + + - We don't set any sharing flags in `mingw_utime()`, which changes the + access and modification of a file. 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. + + `mingw_utime()` was originally implemented via `_wopen()`, which + doesn't give you full control over the sharing mode. Instead, it + calls `_wsopen()` with `_SH_DENYNO`, which ultimately translates to + `FILE_SHARE_READ | FILE_SHARE_WRITE`. It was then refactored via + 090a3085bc (t/helper/test-chmtime: update mingw to support chmtime + on directories, 2022-03-02) to use `CreateFileW()`, but we stopped + setting any sharing flags at all, which seems like an unintentional + side effect. By restoring `FILE_SHARE_READ | FILE_SHARE_WRITE` we + thus fix this and get back the old behaviour of `_wopen()`. - - 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. + The fact that we didn't set the equivalent of `FILE_SHARE_DELETE` + can be explained, as well: neither `_wopen()` nor `_wsopen()` allow + you to do so. So overall, it doesn't seem intentional that we didn't + allow deletions here, either. Adapt both of these callsites to pass all three sharing flags. 2: 3552feddb33 ! 2: c308eafbbb5 compat/mingw: allow deletion of most opened files @@ Commit message compat/mingw: allow deletion of most opened files On Windows, we emulate open(3p) via `mingw_open()`. This function - implements handling of some platform- specific quirks that are required + implements handling of some platform-specific quirks that are required to make it behave as closely as possible like open(3p) would, but for most cases we just call the Windows-specific `_wopen()` function. @@ compat/mingw.c: static int mingw_open_append(wchar_t const *wfilename, int oflag +static int mingw_open_existing(const wchar_t *filename, int oflags, ...) +{ + SECURITY_ATTRIBUTES security_attributes = { -+ .nLength = sizeof(security_attributes), -+ .bInheritHandle = !(oflags & O_NOINHERIT), ++ .nLength = sizeof(security_attributes), ++ .bInheritHandle = !(oflags & O_NOINHERIT), + }; + HANDLE handle; + int access; + int fd; + + /* We only support basic flags. */ -+ if (oflags & ~(O_ACCMODE | O_NOINHERIT)) -+ return errno = ENOSYS, -1; ++ if (oflags & ~(O_ACCMODE | O_NOINHERIT)) { ++ errno = ENOSYS; ++ return -1; ++ } ++ + if (oflags & O_RDWR) + access = GENERIC_READ | GENERIC_WRITE; + else if (oflags & O_WRONLY) 3: d17ca1c7ce3 ! 3: ee1e92e593e compat/mingw: support POSIX semantics for atomic renames @@ Commit message commits. Careful readers might have noticed that [1] does not mention the above - flag, but instead mentions `FILE_RENAME_POSIX_SEMANICS`. This flag is + flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is not for use with `SetFileInformationByHandle()` though, which is what we use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is not documented on [2] or anywhere else as far as I can tell. - Unfortuntaly, we still support Windows systems older than Windows 10 + Unfortunately, we still support Windows systems older than Windows 10 that do not yet have this new flag. Our `_WIN32_WINNT` SDK version still targets 0x0600, which is Windows Vista and later. And even though that Windows version is out-of-support, bumping the SDK version all the way @@ Commit message On another note: `mingw_rename()` has a retry loop that is used in case deleting a file failed because it's still open in another process. One might be pressed to not use this loop anymore when we can use POSIX - semantics. But unfortuntaley, we have to keep it around due to our + semantics. But unfortunately, we have to keep it around due to our dependence on the `FILE_SHARE_DELETE` flag. While we know to set that sharing flag now, other applications may not do so and may thus still cause sharing violations when we try to rename a file.