[v2,0/3] compat/mingw: implement POSIX-style atomic renames

Message

Patrick Steinhardt Oct. 24, 2024, 11:46 a.m. UTC

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.

Comments

Taylor Blau Oct. 24, 2024, 4:47 p.m. UTC | #1

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