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.
--
2.47.0.118.gfd3785337b.dirty
