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

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

[Patrick Steinhardt]

Hi,

so after chatting with Johannes recently I decided to have another stab
at the failures we've seen on Windows with the reftable backend. A quick
refresher: reftables use a central "tables.lock" file that gets updated
via renames such that the update itself is atomic. This causes issues on
Windows when there are many concurrent readers/writers because Windows
does not allow you to rename a file over another file that is held open
by another process by default. This issue is surfaced by t0610, where we
spawn a 100 concurrent git-update-ref(1) processes.

This patch series aims to fix the issue on Windows by implementing
POSIX-style atomic renames. The first two patches refactor the code such
that we use `FILE_SHARE_DELETE` to open files, which allows other
processes to delete the file even while we have an open handle. This
sharing mode is also important in the context of renames, which are
treated like a deletion of the replaced file. The third patch then
reimplements renames via `SetFileAttributesByHandle(FileRenameInfoEx)`,
which has a flag `FILE_RENAME_FLAG_POSIX_SEMANTICS`. When set, Windows
allows us to replace files during a rename which still have an open
handle.

There's one catch: this is only supported in Windows 10 and newer. On
one hand this means that we have to provide the required definitions
ourselves, as we cannot bump the Windows SDK version. On the other hand
we also have to support the case where older Windows versions now fail
at runtime due to `FileRenameInfoEx` being unsupported. But overall this
isn't too bad, I'd say. Johannes has also kindly tested the fallback
logic for me on Windows 8.1.

With these changes t0610 now passes on my Windows 10 machine, but this
may also have a positive impact on other parts of Git where we might
have previously failed.

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             | 146 +++++++++++++++++++++++++++++++++++--
 t/t0610-reftable-basics.sh |   8 +-
 2 files changed, 145 insertions(+), 9 deletions(-)
-- 
2.47.0.118.gfd3785337b.dirty

[PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`

[Patrick Steinhardt]

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(-)

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

[PATCH 2/3] compat/mingw: allow deletion of most opened files

[Patrick Steinhardt]

On Windows, we emulate open(3p) via `mingw_open()`. This function
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.

This function has a major downside though: it does not allow us to
specify the sharing mode. While there is `_wsopen()` that allows us to
pass sharing flags, those sharing flags are not the same `FILE_SHARE_*`
flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows
concurrent read- and write-access, but does not allow for concurrent
deletions. Unfortunately though, we have to allow concurrent deletions
if we want to have POSIX-style atomic renames on top of an existing file
that has open file handles.

Implement a new function that emulates open(3p) for existing files via
`CreateFileW()` such that we can set the required sharing flags.

While we have the same issue when calling open(3p) with `O_CREAT`,
implementing that mode would be more complex due to the required
permission handling. Furthermore, atomic updates via renames typically
write to exclusive lockfile and then perform the rename, and thus we
don't have to handle the case where the locked path has been created
with `O_CREATE`. So while it would be nice to have proper POSIX
semantics in all paths, we instead aim for a minimum viable fix here.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index e326c6fcd2d..ce95f3a5968 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -532,6 +532,59 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 	return fd;
 }
 
+/*
+ * Ideally, we'd use `_wopen()` to implement this functionality so that we
+ * don't have to reimplement it, but unfortunately we do not have tight control
+ * over the share mode there. And while `_wsopen()` and friends exist that give
+ * us _some_ control over the share mode, this family of functions doesn't give
+ * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict
+ * requirement for us though so that we can unlink or rename over files that
+ * are held open by another process.
+ *
+ * We are thus forced to implement our own emulation of `open()`. To make our
+ * life simpler we only implement basic support for this, namely opening
+ * existing files for reading and/or writing. This means that newly created
+ * files won't have their sharing mode set up correctly, but for now I couldn't
+ * find any case where this matters. We may have to revisit that in the future
+ * though based on our needs.
+ */
+static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
+{
+	SECURITY_ATTRIBUTES security_attributes = {
+	    .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_RDWR)
+		access = GENERIC_READ | GENERIC_WRITE;
+	else if (oflags & O_WRONLY)
+		access = GENERIC_WRITE;
+	else
+		access = GENERIC_READ;
+
+	handle = CreateFileW(filename, access,
+			     FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
+			     &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+	if (handle == INVALID_HANDLE_VALUE) {
+		DWORD err = GetLastError();
+		if (err == ERROR_INVALID_PARAMETER)
+			err = ERROR_PATH_NOT_FOUND;
+		errno = err_win_to_posix(err);
+		return -1;
+	}
+
+	fd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);
+	if (fd < 0)
+		CloseHandle(handle);
+	return fd;
+}
+
 /*
  * Does the pathname map to the local named pipe filesystem?
  * That is, does it have a "//./pipe/" prefix?
@@ -567,6 +620,8 @@ int mingw_open (const char *filename, int oflags, ...)
 
 	if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
 		open_fn = mingw_open_append;
+	else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))
+		open_fn = mingw_open_existing;
 	else
 		open_fn = _wopen;
 
-- 
2.47.0.118.gfd3785337b.dirty

[PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames

[Patrick Steinhardt]

By default, Windows restricts access to files when those files have been
opened by another process. As explained in the preceding commits, these
restrictions can be loosened such that reads, writes and/or deletes of
files with open handles _are_ allowed.

While we set up those sharing flags in most relevant code paths now, we
still don't properly handle POSIX-style atomic renames in case the
target path is open. This is failure demonstrated by t0610, where one of
our tests spawns concurrent writes in a reftable-enabled repository and
expects all of them to succeed. This test fails most of the time because
the process that has acquired the "tables.list" lock is unable to rename
it into place while other processes are busy reading that file.

Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag
that allows us to fix this usecase [1]. When set, it is possible to
rename a file over a preexisting file even when the target file still
has handles open. Those handles must have been opened with the
`FILE_SHARE_DELETE` flag, which we have ensured in the preceding
commits.

Careful readers might have noticed that [1] does not mention the above
flag, but instead mentions `FILE_RENAME_POSIX_SEMANICS`. 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
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
to 0x0A00, which is Windows 10 and later, is not an option as it would
make it impossible to compile on Windows 8.1, which is still supported.
Instead, we have to manually declare the relevant infrastructure to make
this feature available and have fallback logic in place in case we run
on a Windows version that does not yet have this flag.

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
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.

This fixes concurrent writes in the reftable backend as demonstrated in
t0610, but may also end up fixing other usecases where Git wants to
perform renames.

[1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information
[2]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c             | 87 ++++++++++++++++++++++++++++++++++++--
 t/t0610-reftable-basics.sh |  8 ++--
 2 files changed, 88 insertions(+), 7 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index ce95f3a5968..df78f61f7f9 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2206,10 +2206,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
+	static int supports_file_rename_info_ex = 1;
 	DWORD attrs, gle;
 	int tries = 0;
 	wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];
-	if (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)
+	int wpnew_len;
+
+	if (xutftowcs_path(wpold, pold) < 0)
+		return -1;
+	wpnew_len = xutftowcs_path(wpnew, pnew);
+	if (wpnew_len < 0)
 		return -1;
 
 	/*
@@ -2220,11 +2226,84 @@ int mingw_rename(const char *pold, const char *pnew)
 		return 0;
 	if (errno != EEXIST)
 		return -1;
+
 repeat:
-	if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
-		return 0;
+	if (supports_file_rename_info_ex) {
+		/*
+		 * Our minimum required Windows version is still set to Windows
+		 * Vista. We thus have to declare required infrastructure for
+		 * FileRenameInfoEx ourselves until we bump _WIN32_WINNT to
+		 * 0x0A00. Furthermore, we have to handle cases where the
+		 * FileRenameInfoEx call isn't supported yet.
+		 */
+#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS                  0x00000001
+#define FILE_RENAME_FLAG_POSIX_SEMANTICS                    0x00000002
+		FILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22;
+		struct {
+			/*
+			 * This is usually an unnamed union, but that is not
+			 * part of ISO C99. We thus inline the field, as we
+			 * really only care for the Flags field anyway.
+			 */
+			DWORD Flags;
+			HANDLE RootDirectory;
+			DWORD FileNameLength;
+			/*
+			 * The actual structure is defined with a single-character
+			 * flex array so that the structure has to be allocated on
+			 * the heap. As we declare this structure ourselves though
+			 * we can avoid the allocation and define FileName to have
+			 * MAX_PATH bytes.
+			 */
+			WCHAR FileName[MAX_PATH];
+		} rename_info = { 0 };
+		HANDLE old_handle = INVALID_HANDLE_VALUE;
+		BOOL success;
+
+		old_handle = CreateFileW(wpold, DELETE,
+					 FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
+					 NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+		if (old_handle == INVALID_HANDLE_VALUE) {
+			errno = err_win_to_posix(GetLastError());
+			return -1;
+		}
+
+		rename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |
+				    FILE_RENAME_FLAG_POSIX_SEMANTICS;
+		rename_info.FileNameLength = wpnew_len * sizeof(WCHAR);
+		memcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));
+
+		success = SetFileInformationByHandle(old_handle, FileRenameInfoEx,
+						     &rename_info, sizeof(rename_info));
+		gle = GetLastError();
+		CloseHandle(old_handle);
+		if (success)
+			return 0;
+
+		/*
+		 * When we see ERROR_INVALID_PARAMETER we can assume that the
+		 * current system doesn't support FileRenameInfoEx. Keep us
+		 * from using it in future calls and retry.
+		 */
+		if (gle == ERROR_INVALID_PARAMETER) {
+			supports_file_rename_info_ex = 0;
+			goto repeat;
+		}
+
+		/*
+		 * In theory, we shouldn't get ERROR_ACCESS_DENIED because we
+		 * always open files with FILE_SHARE_DELETE But in practice we
+		 * cannot assume that Git is the only one accessing files, and
+		 * other applications may not set FILE_SHARE_DELETE. So we have
+		 * to retry.
+		 */
+	} else {
+		if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
+			return 0;
+		gle = GetLastError();
+	}
+
 	/* TODO: translate more errors */
-	gle = GetLastError();
 	if (gle == ERROR_ACCESS_DENIED &&
 	    (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {
 		if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index babec7993e3..eaf6fab6d29 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '
 	)
 '
 
-# This test fails most of the time on Windows systems. The root cause is
+# This test fails most of the time on Cygwin systems. The root cause is
 # that Windows does not allow us to rename the "tables.list.lock" file into
-# place when "tables.list" is open for reading by a concurrent process.
-test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '
+# place when "tables.list" is open for reading by a concurrent process. We have
+# worked around that in our MinGW-based rename emulation, but the Cygwin
+# emulation seems to be insufficient.
+test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
 	(
-- 
2.47.0.118.gfd3785337b.dirty

Re: [PATCH 0/3] compat/mingw: implement POSIX-style atomic renames

[Taylor Blau]

On Wed, Oct 23, 2024 at 05:04:55PM +0200, Patrick Steinhardt wrote:
> With these changes t0610 now passes on my Windows 10 machine, but this
> may also have a positive impact on other parts of Git where we might
> have previously failed.

Ooo. Thank you both for collaborating and working on this.

Thanks,
Taylor

Re: [PATCH 2/3] compat/mingw: allow deletion of most opened files

[Kristoffer Haugsbakk]

On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:
> On Windows, we emulate open(3p) via `mingw_open()`. This function
> implements handling of some platform- specific quirks that are required

s/platform- specific/platform-specific/

Linewrapping artifact?

> to make it behave as closely as possible like open(3p) would, but for
> most cases we just call the Windows-specific `_wopen()` function.
>
> This function has a major downside though: it does not allow us to
> specify the sharing mode. While there is `_wsopen()` that allows us to
> pass sharing flags, those sharing flags are not the same `FILE_SHARE_*`
> flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows
> concurrent read- and write-access, but does not allow for concurrent
> deletions. Unfortunately though, we have to allow concurrent deletions
> if we want to have POSIX-style atomic renames on top of an existing file
> that has open file handles.
>
> Implement a new function that emulates open(3p) for existing files via
> `CreateFileW()` such that we can set the required sharing flags.
>
> While we have the same issue when calling open(3p) with `O_CREAT`,

s/O_CREAT/O_CREATE/ ?

> implementing that mode would be more complex due to the required
> permission handling. Furthermore, atomic updates via renames typically
> write to exclusive lockfile and then perform the rename, and thus we
> don't have to handle the case where the locked path has been created
> with `O_CREATE`. So while it would be nice to have proper POSIX
> semantics in all paths, we instead aim for a minimum viable fix here.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  compat/mingw.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index e326c6fcd2d..ce95f3a5968 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -532,6 +532,59 @@ static int mingw_open_append(wchar_t const
> *wfilename, int oflags, ...)
>  	return fd;
>  }
>
> +/*
> + * Ideally, we'd use `_wopen()` to implement this functionality so that we
> + * don't have to reimplement it, but unfortunately we do not have tight control
> + * over the share mode there. And while `_wsopen()` and friends exist that give
> + * us _some_ control over the share mode, this family of functions doesn't give
> + * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict
> + * requirement for us though so that we can unlink or rename over files that
> + * are held open by another process.
> + *
> + * We are thus forced to implement our own emulation of `open()`. To make our
> + * life simpler we only implement basic support for this, namely opening
> + * existing files for reading and/or writing. This means that newly created
> + * files won't have their sharing mode set up correctly, but for now I couldn't
> + * find any case where this matters. We may have to revisit that in the future
> + * though based on our needs.
> + */

This is above my technical level but the comment reads nicely.

> +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
> +{
> +	SECURITY_ATTRIBUTES security_attributes = {
> +	    .nLength = sizeof(security_attributes),
> +	    .bInheritHandle = !(oflags & O_NOINHERIT),

`clang-format` thinks that these two lines should be indented with tabs
instead (so two tabs in total).

(Ubuntu clang-format version 14.0.0-1ubuntu1.1)

> +	};
> +	HANDLE handle;
> +	int access;
> +	int fd;
> +
> +	/* We only support basic flags. */
> +	if (oflags & ~(O_ACCMODE | O_NOINHERIT))
> +		return errno = ENOSYS, -1;

This use of the comma operator is maybe an idiom to save space and avoid
a brace around the `if`?  This pattern is already in use in
`mingw_open_append`.  I see in `mingw.h` that it uses:

```
static inline int symlink(const char *oldpath UNUSED, const char *newpath UNUSED)
{ errno = ENOSYS; return -1; }
```

> +	if (oflags & O_RDWR)
> +		access = GENERIC_READ | GENERIC_WRITE;
> +	else if (oflags & O_WRONLY)
> +		access = GENERIC_WRITE;
> +	else
> +		access = GENERIC_READ;
> +
> +	handle = CreateFileW(filename, access,
> +			     FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
> +			     &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

`clang-format` says that these two lines are too long.

> +	if (handle == INVALID_HANDLE_VALUE) {
> +		DWORD err = GetLastError();
> +		if (err == ERROR_INVALID_PARAMETER)
> +			err = ERROR_PATH_NOT_FOUND;
> +		errno = err_win_to_posix(err);
> +		return -1;

Here you don’t use the comma operator, presumably because it wouldn’t
turn the `if` into a one-statement block.

> +	}
> +
> +	fd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);
> +	if (fd < 0)
> +		CloseHandle(handle);
> +	return fd;
> +}
> +
>  /*
>   * Does the pathname map to the local named pipe filesystem?
>   * That is, does it have a "//./pipe/" prefix?
> @@ -567,6 +620,8 @@ int mingw_open (const char *filename, int oflags, ...)
>
>  	if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
>  		open_fn = mingw_open_append;
> +	else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))
> +		open_fn = mingw_open_existing;
>  	else
>  		open_fn = _wopen;
>
> --
> 2.47.0.118.gfd3785337b.dirty

Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`

[Kristoffer Haugsbakk]

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.

-- 
Kristoffer Haugsbakk

Re: [PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames

[Kristoffer Haugsbakk]

On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:
> […]
> Careful readers might have noticed that [1] does not mention the above
> flag, but instead mentions `FILE_RENAME_POSIX_SEMANICS`. This flag is

s/FILE_RENAME_POSIX_SEMANICS/FILE_RENAME_FLAG_POSIX_SEMANTICS/

> 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

s/Unfortuntaly/Unfortunately/

> […]
>
> 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

s/unfortuntaley/unfortunately/

> [1]:
> https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information
> [2]:
> https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  compat/mingw.c             | 87 ++++++++++++++++++++++++++++++++++++--
>  t/t0610-reftable-basics.sh |  8 ++--
>  2 files changed, 88 insertions(+), 7 deletions(-)
> […]
> +
>  	/* TODO: translate more errors */

It seems that `Documentation` doesn’t mention the difference between
`TODO` and `NEEDSWORK`.  What is it?

> […]
> --
> 2.47.0.118.gfd3785337b.dirty

Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`

[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.

Thanks,
Taylor

Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`

[Taylor Blau]

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

Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`

[Taylor Blau]

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

Re: [PATCH 2/3] compat/mingw: allow deletion of most opened files

[Taylor Blau]

On Wed, Oct 23, 2024 at 06:17:15PM +0200, Kristoffer Haugsbakk wrote:
> On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:
> > On Windows, we emulate open(3p) via `mingw_open()`. This function
> > implements handling of some platform- specific quirks that are required
>
> s/platform- specific/platform-specific/
>
> Linewrapping artifact?

Looks like it. Thanks for spotting.

> > to make it behave as closely as possible like open(3p) would, but for
> > most cases we just call the Windows-specific `_wopen()` function.
> >
> > This function has a major downside though: it does not allow us to
> > specify the sharing mode. While there is `_wsopen()` that allows us to
> > pass sharing flags, those sharing flags are not the same `FILE_SHARE_*`
> > flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows
> > concurrent read- and write-access, but does not allow for concurrent
> > deletions. Unfortunately though, we have to allow concurrent deletions
> > if we want to have POSIX-style atomic renames on top of an existing file
> > that has open file handles.
> >
> > Implement a new function that emulates open(3p) for existing files via
> > `CreateFileW()` such that we can set the required sharing flags.
> >
> > While we have the same issue when calling open(3p) with `O_CREAT`,
>
> s/O_CREAT/O_CREATE/ ?

No, O_CREAT is the flag name.

> > +	};
> > +	HANDLE handle;
> > +	int access;
> > +	int fd;
> > +
> > +	/* We only support basic flags. */
> > +	if (oflags & ~(O_ACCMODE | O_NOINHERIT))
> > +		return errno = ENOSYS, -1;
>
> This use of the comma operator is maybe an idiom to save space and avoid
> a brace around the `if`?  This pattern is already in use in
> `mingw_open_append`.  I see in `mingw.h` that it uses:
>
> ```
> static inline int symlink(const char *oldpath UNUSED, const char *newpath UNUSED)
> { errno = ENOSYS; return -1; }
> ```
>

Yeah. What Patrick wrote here is not technically wrong, but I would not
consider it in the style of the rest of the project. Perhaps
compat/mingw-stuff is a bit of a wild west, but I think it would be
match the rest of the project's conventions here.

I actually think from grepping around that mingw_open_append is the only
other function in the tree that uses the "return errno = XXX, -1;"
trick. It might be nice to keep it that way, and/or rewrite that portion
like so:

--- 8< ---
diff --git a/compat/mingw.c b/compat/mingw.c
index df78f61f7f..c36147549a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -494,8 +494,10 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 	DWORD create = (oflags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING;

 	/* only these flags are supported */
-	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))
-		return errno = ENOSYS, -1;
+	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {
+		errno = ENOSYS;
+		return -1;
+	}

 	/*
 	 * FILE_SHARE_WRITE is required to permit child processes
--- >8 ---

Thanks,
Taylor

Re: [PATCH 2/3] compat/mingw: allow deletion of most opened files

[Taylor Blau]

On Wed, Oct 23, 2024 at 05:05:00PM +0200, Patrick Steinhardt wrote:
>  compat/mingw.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)

Very well reasoned and explained. I am certainly not a Windows expert,
but from what I read the changes look reasonable to me.

Thanks,
Taylor

Re: [PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames

[Taylor Blau]

On Wed, Oct 23, 2024 at 05:05:03PM +0200, Patrick Steinhardt wrote:
> ---
>  compat/mingw.c             | 87 ++++++++++++++++++++++++++++++++++++--
>  t/t0610-reftable-basics.sh |  8 ++--
>  2 files changed, 88 insertions(+), 7 deletions(-)

All well reasoned and explained as usual. I'm glossing over the
Windows-specific parts, though they look correct to me if I squint and
pretend that I have even a passing familiarity with the platform ;-).

This looks good, and I am glad to hear that it was tested on Windows 8.1
by Johannes, and on Windows 10 by you. Do we have any reason to believe
it would break on Vista? If so, should we test there as well?

Otherwise, this is looking good, modulo the handful of typos that was
noticed earlier in the thread on this patch. Thanks to you and Johannes
for working on this together!

Thanks,
Taylor

Re: [PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames

[Patrick Steinhardt]

On Wed, Oct 23, 2024 at 06:19:43PM +0200, Kristoffer Haugsbakk wrote:
> On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:
> > […]
> > Careful readers might have noticed that [1] does not mention the above
> > flag, but instead mentions `FILE_RENAME_POSIX_SEMANICS`. This flag is
> 
> s/FILE_RENAME_POSIX_SEMANICS/FILE_RENAME_FLAG_POSIX_SEMANTICS/

No, this is actually intentional. There are two different flags:

  - FILE_RENAME_POSIX_SEMANTICS is what ntifs.h exposes and what I've
    been linking for documentation. This flag cannot be used though with
    `FileRenameInfoEx`.

  - FILE_RENAME_FLAG_POSIX_SEMANTICS is what needs to be used with
    `FileRenameInfoEx`, but it is undocumented.

> >  	/* TODO: translate more errors */
> 
> It seems that `Documentation` doesn’t mention the difference between
> `TODO` and `NEEDSWORK`.  What is it?

No idea, to be honest.

Patrick

Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`

[Patrick Steinhardt]

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

Re: [PATCH 2/3] compat/mingw: allow deletion of most opened files

[Patrick Steinhardt]

On Wed, Oct 23, 2024 at 06:17:15PM +0200, Kristoffer Haugsbakk wrote:
> On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:
> > While we have the same issue when calling open(3p) with `O_CREAT`,
> 
> s/O_CREAT/O_CREATE/ ?

Yeah, you'd think that, but no, it's really `O_CREAT`.

> > +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
> > +{
> > +	SECURITY_ATTRIBUTES security_attributes = {
> > +	    .nLength = sizeof(security_attributes),
> > +	    .bInheritHandle = !(oflags & O_NOINHERIT),
> 
> `clang-format` thinks that these two lines should be indented with tabs
> instead (so two tabs in total).
> 
> (Ubuntu clang-format version 14.0.0-1ubuntu1.1)

And they indeed should be.

> > +	};
> > +	HANDLE handle;
> > +	int access;
> > +	int fd;
> > +
> > +	/* We only support basic flags. */
> > +	if (oflags & ~(O_ACCMODE | O_NOINHERIT))
> > +		return errno = ENOSYS, -1;
> 
> This use of the comma operator is maybe an idiom to save space and avoid
> a brace around the `if`?  This pattern is already in use in
> `mingw_open_append`.  I see in `mingw.h` that it uses:
> 
> ```
> static inline int symlink(const char *oldpath UNUSED, const char *newpath UNUSED)
> { errno = ENOSYS; return -1; }
> ```

I basically copied this from other code, but don't care strongly about
it either way.

> > +	if (oflags & O_RDWR)
> > +		access = GENERIC_READ | GENERIC_WRITE;
> > +	else if (oflags & O_WRONLY)
> > +		access = GENERIC_WRITE;
> > +	else
> > +		access = GENERIC_READ;
> > +
> > +	handle = CreateFileW(filename, access,
> > +			     FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
> > +			     &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
> 
> `clang-format` says that these two lines are too long.

Might be. But the `FILE_SHARE_*` line cannot really be any shorter
without hurting readability. And the second line just follows the first
line then.

Patrick

Re: [PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames

[Kristoffer Haugsbakk]

On Thu, Oct 24, 2024, at 08:30, Patrick Steinhardt wrote:
>> s/FILE_RENAME_POSIX_SEMANICS/FILE_RENAME_FLAG_POSIX_SEMANTICS/
>
> No, this is actually intentional. There are two different flags:
>
>   - FILE_RENAME_POSIX_SEMANTICS is what ntifs.h exposes and what I've
>     been linking for documentation. This flag cannot be used though with
>     `FileRenameInfoEx`.
>
>   - FILE_RENAME_FLAG_POSIX_SEMANTICS is what needs to be used with
>     `FileRenameInfoEx`, but it is undocumented.
>

I made a copy–paste mistake. :P I should have just pointed out this
(missing T):

    _SEMANICS

So you wrote it the correct-looking way in your first bullet
point here.

Re: [PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames

[Patrick Steinhardt]

On Thu, Oct 24, 2024 at 09:18:18AM +0200, Kristoffer Haugsbakk wrote:
> On Thu, Oct 24, 2024, at 08:30, Patrick Steinhardt wrote:
> >> s/FILE_RENAME_POSIX_SEMANICS/FILE_RENAME_FLAG_POSIX_SEMANTICS/
> >
> > No, this is actually intentional. There are two different flags:
> >
> >   - FILE_RENAME_POSIX_SEMANTICS is what ntifs.h exposes and what I've
> >     been linking for documentation. This flag cannot be used though with
> >     `FileRenameInfoEx`.
> >
> >   - FILE_RENAME_FLAG_POSIX_SEMANTICS is what needs to be used with
> >     `FileRenameInfoEx`, but it is undocumented.
> >
> 
> I made a copy–paste mistake. :P I should have just pointed out this
> (missing T):
> 
>     _SEMANICS
> 
> So you wrote it the correct-looking way in your first bullet
> point here.

Oh, indeed! Thanks.

Patrick

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

[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

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

[PATCH v2 1/3] compat/mingw: share file handles created via `CreateFileW()`

[Patrick Steinhardt]

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:

  - `FILE_SHARE_READ` allows a concurrent process to open the file for
    reading.

  - `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. 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()`.

    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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

[PATCH v2 2/3] compat/mingw: allow deletion of most opened files

[Patrick Steinhardt]

On Windows, we emulate open(3p) via `mingw_open()`. This function
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.

This function has a major downside though: it does not allow us to
specify the sharing mode. While there is `_wsopen()` that allows us to
pass sharing flags, those sharing flags are not the same `FILE_SHARE_*`
flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows
concurrent read- and write-access, but does not allow for concurrent
deletions. Unfortunately though, we have to allow concurrent deletions
if we want to have POSIX-style atomic renames on top of an existing file
that has open file handles.

Implement a new function that emulates open(3p) for existing files via
`CreateFileW()` such that we can set the required sharing flags.

While we have the same issue when calling open(3p) with `O_CREAT`,
implementing that mode would be more complex due to the required
permission handling. Furthermore, atomic updates via renames typically
write to exclusive lockfile and then perform the rename, and thus we
don't have to handle the case where the locked path has been created
with `O_CREATE`. So while it would be nice to have proper POSIX
semantics in all paths, we instead aim for a minimum viable fix here.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index e326c6fcd2d..6c75fe36b15 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -532,6 +532,62 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 	return fd;
 }
 
+/*
+ * Ideally, we'd use `_wopen()` to implement this functionality so that we
+ * don't have to reimplement it, but unfortunately we do not have tight control
+ * over the share mode there. And while `_wsopen()` and friends exist that give
+ * us _some_ control over the share mode, this family of functions doesn't give
+ * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict
+ * requirement for us though so that we can unlink or rename over files that
+ * are held open by another process.
+ *
+ * We are thus forced to implement our own emulation of `open()`. To make our
+ * life simpler we only implement basic support for this, namely opening
+ * existing files for reading and/or writing. This means that newly created
+ * files won't have their sharing mode set up correctly, but for now I couldn't
+ * find any case where this matters. We may have to revisit that in the future
+ * though based on our needs.
+ */
+static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
+{
+	SECURITY_ATTRIBUTES security_attributes = {
+		.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)) {
+		errno = ENOSYS;
+		return -1;
+	}
+
+	if (oflags & O_RDWR)
+		access = GENERIC_READ | GENERIC_WRITE;
+	else if (oflags & O_WRONLY)
+		access = GENERIC_WRITE;
+	else
+		access = GENERIC_READ;
+
+	handle = CreateFileW(filename, access,
+			     FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
+			     &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+	if (handle == INVALID_HANDLE_VALUE) {
+		DWORD err = GetLastError();
+		if (err == ERROR_INVALID_PARAMETER)
+			err = ERROR_PATH_NOT_FOUND;
+		errno = err_win_to_posix(err);
+		return -1;
+	}
+
+	fd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);
+	if (fd < 0)
+		CloseHandle(handle);
+	return fd;
+}
+
 /*
  * Does the pathname map to the local named pipe filesystem?
  * That is, does it have a "//./pipe/" prefix?
@@ -567,6 +623,8 @@ int mingw_open (const char *filename, int oflags, ...)
 
 	if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
 		open_fn = mingw_open_append;
+	else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))
+		open_fn = mingw_open_existing;
 	else
 		open_fn = _wopen;
 
-- 
2.47.0.118.gfd3785337b.dirty

[PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames

[Patrick Steinhardt]

By default, Windows restricts access to files when those files have been
opened by another process. As explained in the preceding commits, these
restrictions can be loosened such that reads, writes and/or deletes of
files with open handles _are_ allowed.

While we set up those sharing flags in most relevant code paths now, we
still don't properly handle POSIX-style atomic renames in case the
target path is open. This is failure demonstrated by t0610, where one of
our tests spawns concurrent writes in a reftable-enabled repository and
expects all of them to succeed. This test fails most of the time because
the process that has acquired the "tables.list" lock is unable to rename
it into place while other processes are busy reading that file.

Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag
that allows us to fix this usecase [1]. When set, it is possible to
rename a file over a preexisting file even when the target file still
has handles open. Those handles must have been opened with the
`FILE_SHARE_DELETE` flag, which we have ensured in the preceding
commits.

Careful readers might have noticed that [1] does not mention the above
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.

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
to 0x0A00, which is Windows 10 and later, is not an option as it would
make it impossible to compile on Windows 8.1, which is still supported.
Instead, we have to manually declare the relevant infrastructure to make
this feature available and have fallback logic in place in case we run
on a Windows version that does not yet have this flag.

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 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.

This fixes concurrent writes in the reftable backend as demonstrated in
t0610, but may also end up fixing other usecases where Git wants to
perform renames.

[1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information
[2]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c             | 87 ++++++++++++++++++++++++++++++++++++--
 t/t0610-reftable-basics.sh |  8 ++--
 2 files changed, 88 insertions(+), 7 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 6c75fe36b15..22c005a4533 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2209,10 +2209,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
+	static int supports_file_rename_info_ex = 1;
 	DWORD attrs, gle;
 	int tries = 0;
 	wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];
-	if (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)
+	int wpnew_len;
+
+	if (xutftowcs_path(wpold, pold) < 0)
+		return -1;
+	wpnew_len = xutftowcs_path(wpnew, pnew);
+	if (wpnew_len < 0)
 		return -1;
 
 	/*
@@ -2223,11 +2229,84 @@ int mingw_rename(const char *pold, const char *pnew)
 		return 0;
 	if (errno != EEXIST)
 		return -1;
+
 repeat:
-	if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
-		return 0;
+	if (supports_file_rename_info_ex) {
+		/*
+		 * Our minimum required Windows version is still set to Windows
+		 * Vista. We thus have to declare required infrastructure for
+		 * FileRenameInfoEx ourselves until we bump _WIN32_WINNT to
+		 * 0x0A00. Furthermore, we have to handle cases where the
+		 * FileRenameInfoEx call isn't supported yet.
+		 */
+#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS                  0x00000001
+#define FILE_RENAME_FLAG_POSIX_SEMANTICS                    0x00000002
+		FILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22;
+		struct {
+			/*
+			 * This is usually an unnamed union, but that is not
+			 * part of ISO C99. We thus inline the field, as we
+			 * really only care for the Flags field anyway.
+			 */
+			DWORD Flags;
+			HANDLE RootDirectory;
+			DWORD FileNameLength;
+			/*
+			 * The actual structure is defined with a single-character
+			 * flex array so that the structure has to be allocated on
+			 * the heap. As we declare this structure ourselves though
+			 * we can avoid the allocation and define FileName to have
+			 * MAX_PATH bytes.
+			 */
+			WCHAR FileName[MAX_PATH];
+		} rename_info = { 0 };
+		HANDLE old_handle = INVALID_HANDLE_VALUE;
+		BOOL success;
+
+		old_handle = CreateFileW(wpold, DELETE,
+					 FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
+					 NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+		if (old_handle == INVALID_HANDLE_VALUE) {
+			errno = err_win_to_posix(GetLastError());
+			return -1;
+		}
+
+		rename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |
+				    FILE_RENAME_FLAG_POSIX_SEMANTICS;
+		rename_info.FileNameLength = wpnew_len * sizeof(WCHAR);
+		memcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));
+
+		success = SetFileInformationByHandle(old_handle, FileRenameInfoEx,
+						     &rename_info, sizeof(rename_info));
+		gle = GetLastError();
+		CloseHandle(old_handle);
+		if (success)
+			return 0;
+
+		/*
+		 * When we see ERROR_INVALID_PARAMETER we can assume that the
+		 * current system doesn't support FileRenameInfoEx. Keep us
+		 * from using it in future calls and retry.
+		 */
+		if (gle == ERROR_INVALID_PARAMETER) {
+			supports_file_rename_info_ex = 0;
+			goto repeat;
+		}
+
+		/*
+		 * In theory, we shouldn't get ERROR_ACCESS_DENIED because we
+		 * always open files with FILE_SHARE_DELETE But in practice we
+		 * cannot assume that Git is the only one accessing files, and
+		 * other applications may not set FILE_SHARE_DELETE. So we have
+		 * to retry.
+		 */
+	} else {
+		if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
+			return 0;
+		gle = GetLastError();
+	}
+
 	/* TODO: translate more errors */
-	gle = GetLastError();
 	if (gle == ERROR_ACCESS_DENIED &&
 	    (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {
 		if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index babec7993e3..eaf6fab6d29 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '
 	)
 '
 
-# This test fails most of the time on Windows systems. The root cause is
+# This test fails most of the time on Cygwin systems. The root cause is
 # that Windows does not allow us to rename the "tables.list.lock" file into
-# place when "tables.list" is open for reading by a concurrent process.
-test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '
+# place when "tables.list" is open for reading by a concurrent process. We have
+# worked around that in our MinGW-based rename emulation, but the Cygwin
+# emulation seems to be insufficient.
+test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
 	(
-- 
2.47.0.118.gfd3785337b.dirty

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

[Taylor Blau]

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

Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`

[Johannes Sixt]

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

Re: [PATCH v2 2/3] compat/mingw: allow deletion of most opened files

[Johannes Sixt]

Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:
> On Windows, we emulate open(3p) via `mingw_open()`. This function
> 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.
> 
> This function has a major downside though: it does not allow us to
> specify the sharing mode. While there is `_wsopen()` that allows us to
> pass sharing flags, those sharing flags are not the same `FILE_SHARE_*`
> flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows
> concurrent read- and write-access, but does not allow for concurrent
> deletions. Unfortunately though, we have to allow concurrent deletions
> if we want to have POSIX-style atomic renames on top of an existing file
> that has open file handles.
> 
> Implement a new function that emulates open(3p) for existing files via
> `CreateFileW()` such that we can set the required sharing flags.
> 
> While we have the same issue when calling open(3p) with `O_CREAT`,
> implementing that mode would be more complex due to the required
> permission handling. Furthermore, atomic updates via renames typically
> write to exclusive lockfile and then perform the rename, and thus we
> don't have to handle the case where the locked path has been created
> with `O_CREATE`. So while it would be nice to have proper POSIX
> semantics in all paths, we instead aim for a minimum viable fix here.

Agreed!

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  compat/mingw.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index e326c6fcd2d..6c75fe36b15 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -532,6 +532,62 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
>  	return fd;
>  }
>  
> +/*
> + * Ideally, we'd use `_wopen()` to implement this functionality so that we
> + * don't have to reimplement it, but unfortunately we do not have tight control
> + * over the share mode there. And while `_wsopen()` and friends exist that give
> + * us _some_ control over the share mode, this family of functions doesn't give
> + * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict
> + * requirement for us though so that we can unlink or rename over files that
> + * are held open by another process.
> + *
> + * We are thus forced to implement our own emulation of `open()`. To make our
> + * life simpler we only implement basic support for this, namely opening
> + * existing files for reading and/or writing. This means that newly created
> + * files won't have their sharing mode set up correctly, but for now I couldn't
> + * find any case where this matters. We may have to revisit that in the future
> + * though based on our needs.
> + */
> +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
> +{
> +	SECURITY_ATTRIBUTES security_attributes = {
> +		.nLength = sizeof(security_attributes),
> +		.bInheritHandle = !(oflags & O_NOINHERIT),
> +	};
> +	HANDLE handle;
> +	int access;

I would have made this variable DWORD or unsigned instead of plain int,
because it receives a bit pattern and would match the parameter type of
CreateFileW() better; but no big deal.

> +	int fd;
> +
> +	/* We only support basic flags. */
> +	if (oflags & ~(O_ACCMODE | O_NOINHERIT)) {
> +		errno = ENOSYS;
> +		return -1;
> +	}
> +
> +	if (oflags & O_RDWR)
> +		access = GENERIC_READ | GENERIC_WRITE;
> +	else if (oflags & O_WRONLY)
> +		access = GENERIC_WRITE;
> +	else
> +		access = GENERIC_READ;

O_RDWR, O_WRONLY and O_RDONLY are not flags, but values occupying two
bits of oflags. This must be:

	if ((oflags & O_ACCMODE) == O_RDWR)
		access = GENERIC_READ | GENERIC_WRITE;
	else if ((oflags & O_ACCMODE) == O_WRONLY)
		access = GENERIC_WRITE;
	else
		access = GENERIC_READ;

or similar.

> +
> +	handle = CreateFileW(filename, access,
> +			     FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
> +			     &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
> +	if (handle == INVALID_HANDLE_VALUE) {
> +		DWORD err = GetLastError();
> +		if (err == ERROR_INVALID_PARAMETER)
> +			err = ERROR_PATH_NOT_FOUND;

First I wondered what this is about, but then noticed that it is just
copied from the mingw_open_append() implementation catering to some
bogus network filesystems. Good.

> +		errno = err_win_to_posix(err);
> +		return -1;
> +	}
> +
> +	fd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);
> +	if (fd < 0)
> +		CloseHandle(handle);
> +	return fd;
> +}
> +
>  /*
>   * Does the pathname map to the local named pipe filesystem?
>   * That is, does it have a "//./pipe/" prefix?
> @@ -567,6 +623,8 @@ int mingw_open (const char *filename, int oflags, ...)
>  
>  	if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
>  		open_fn = mingw_open_append;
> +	else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))
> +		open_fn = mingw_open_existing;
>  	else
>  		open_fn = _wopen;
>  

Makes sense!

-- Hannes

Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames

[Johannes Sixt]

Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:
> By default, Windows restricts access to files when those files have been
> opened by another process. As explained in the preceding commits, these
> restrictions can be loosened such that reads, writes and/or deletes of
> files with open handles _are_ allowed.
> 
> While we set up those sharing flags in most relevant code paths now, we
> still don't properly handle POSIX-style atomic renames in case the
> target path is open. This is failure demonstrated by t0610, where one of
> our tests spawns concurrent writes in a reftable-enabled repository and
> expects all of them to succeed. This test fails most of the time because
> the process that has acquired the "tables.list" lock is unable to rename
> it into place while other processes are busy reading that file.
> 
> Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag
> that allows us to fix this usecase [1]. When set, it is possible to
> rename a file over a preexisting file even when the target file still
> has handles open. Those handles must have been opened with the
> `FILE_SHARE_DELETE` flag, which we have ensured in the preceding
> commits.
> > Careful readers might have noticed that [1] does not mention the above
> 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.

The Windows 10 SDK defines FILE_RENAME_FLAG_REPLACE_IF_EXISTS and
FILE_RENAME_FLAG_POSIX_SEMANTICS for SetFileInformationByHandle(). That
the documentation lacks "_FLAG_" in the names must be an error in the
documentation.

I found the mention of FILE_RENAME_POSIX_SEMANTICS quite distracting,
because it is a flag to be used with CreateFileW() and basically only
has to do with case-sensitivity, but nothing with POSIX semantics of
renaming.

> 
> 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
> to 0x0A00, which is Windows 10 and later, is not an option as it would
> make it impossible to compile on Windows 8.1, which is still supported.
> Instead, we have to manually declare the relevant infrastructure to make
> this feature available and have fallback logic in place in case we run
> on a Windows version that does not yet have this flag.
> 
> 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 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.
> 
> This fixes concurrent writes in the reftable backend as demonstrated in
> t0610, but may also end up fixing other usecases where Git wants to
> perform renames.
> 
> [1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information
> [2]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  compat/mingw.c             | 87 ++++++++++++++++++++++++++++++++++++--
>  t/t0610-reftable-basics.sh |  8 ++--
>  2 files changed, 88 insertions(+), 7 deletions(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 6c75fe36b15..22c005a4533 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2209,10 +2209,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)
>  #undef rename
>  int mingw_rename(const char *pold, const char *pnew)
>  {
> +	static int supports_file_rename_info_ex = 1;
>  	DWORD attrs, gle;
>  	int tries = 0;
>  	wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];
> -	if (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)
> +	int wpnew_len;
> +
> +	if (xutftowcs_path(wpold, pold) < 0)
> +		return -1;
> +	wpnew_len = xutftowcs_path(wpnew, pnew);
> +	if (wpnew_len < 0)
>  		return -1;
>  
>  	/*
> @@ -2223,11 +2229,84 @@ int mingw_rename(const char *pold, const char *pnew)
>  		return 0;
>  	if (errno != EEXIST)
>  		return -1;
> +
>  repeat:
> -	if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
> -		return 0;
> +	if (supports_file_rename_info_ex) {
> +		/*
> +		 * Our minimum required Windows version is still set to Windows
> +		 * Vista. We thus have to declare required infrastructure for
> +		 * FileRenameInfoEx ourselves until we bump _WIN32_WINNT to
> +		 * 0x0A00. Furthermore, we have to handle cases where the
> +		 * FileRenameInfoEx call isn't supported yet.
> +		 */
> +#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS                  0x00000001
> +#define FILE_RENAME_FLAG_POSIX_SEMANTICS                    0x00000002
> +		FILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22;
> +		struct {
> +			/*
> +			 * This is usually an unnamed union, but that is not
> +			 * part of ISO C99. We thus inline the field, as we
> +			 * really only care for the Flags field anyway.
> +			 */
> +			DWORD Flags;
> +			HANDLE RootDirectory;
> +			DWORD FileNameLength;
> +			/*
> +			 * The actual structure is defined with a single-character
> +			 * flex array so that the structure has to be allocated on
> +			 * the heap. As we declare this structure ourselves though
> +			 * we can avoid the allocation and define FileName to have
> +			 * MAX_PATH bytes.
> +			 */
> +			WCHAR FileName[MAX_PATH];
> +		} rename_info = { 0 };

Good.

> +		HANDLE old_handle = INVALID_HANDLE_VALUE;
> +		BOOL success;
> +
> +		old_handle = CreateFileW(wpold, DELETE,
> +					 FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
> +					 NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
> +		if (old_handle == INVALID_HANDLE_VALUE) {
> +			errno = err_win_to_posix(GetLastError());
> +			return -1;
> +		}
> +
> +		rename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |
> +				    FILE_RENAME_FLAG_POSIX_SEMANTICS;
> +		rename_info.FileNameLength = wpnew_len * sizeof(WCHAR);

Size is in bytes, not in characters, and without the NUL. Good. I read
one comment on SO, which said that this value is ignored...

> +		memcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));

... which makes it all the more important that this path is
NUL-terminated. Yet, this does not copy the NUL. We are still good,
because the buffer is zero-initialized and xutftowcs_path() ensures that
wpnew_len is at most MAX_PATH-1.

> +
> +		success = SetFileInformationByHandle(old_handle, FileRenameInfoEx,
> +						     &rename_info, sizeof(rename_info));
> +		gle = GetLastError();
> +		CloseHandle(old_handle);
> +		if (success)
> +			return 0;
> +
> +		/*
> +		 * When we see ERROR_INVALID_PARAMETER we can assume that the
> +		 * current system doesn't support FileRenameInfoEx. Keep us
> +		 * from using it in future calls and retry.
> +		 */
> +		if (gle == ERROR_INVALID_PARAMETER) {
> +			supports_file_rename_info_ex = 0;
> +			goto repeat;
> +		}
> +
> +		/*
> +		 * In theory, we shouldn't get ERROR_ACCESS_DENIED because we
> +		 * always open files with FILE_SHARE_DELETE But in practice we
> +		 * cannot assume that Git is the only one accessing files, and
> +		 * other applications may not set FILE_SHARE_DELETE. So we have
> +		 * to retry.
> +		 */

Good thinking!

> +	} else {
> +		if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
> +			return 0;
> +		gle = GetLastError();
> +	}
> +
>  	/* TODO: translate more errors */
> -	gle = GetLastError();
>  	if (gle == ERROR_ACCESS_DENIED &&
>  	    (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {
>  		if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> index babec7993e3..eaf6fab6d29 100755
> --- a/t/t0610-reftable-basics.sh
> +++ b/t/t0610-reftable-basics.sh
> @@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '
>  	)
>  '
>  
> -# This test fails most of the time on Windows systems. The root cause is
> +# This test fails most of the time on Cygwin systems. The root cause is
>  # that Windows does not allow us to rename the "tables.list.lock" file into
> -# place when "tables.list" is open for reading by a concurrent process.
> -test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '
> +# place when "tables.list" is open for reading by a concurrent process. We have
> +# worked around that in our MinGW-based rename emulation, but the Cygwin
> +# emulation seems to be insufficient.
> +test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '
>  	test_when_finished "rm -rf repo" &&
>  	git init repo &&
>  	(

The general structure of the patch makes a lot of sense!

-- Hannes

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

[Johannes Sixt]

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

Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames

[Patrick Steinhardt]

On Sun, Oct 27, 2024 at 02:23:28PM +0100, Johannes Sixt wrote:
> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:
> > By default, Windows restricts access to files when those files have been
> > opened by another process. As explained in the preceding commits, these
> > restrictions can be loosened such that reads, writes and/or deletes of
> > files with open handles _are_ allowed.
> > 
> > While we set up those sharing flags in most relevant code paths now, we
> > still don't properly handle POSIX-style atomic renames in case the
> > target path is open. This is failure demonstrated by t0610, where one of
> > our tests spawns concurrent writes in a reftable-enabled repository and
> > expects all of them to succeed. This test fails most of the time because
> > the process that has acquired the "tables.list" lock is unable to rename
> > it into place while other processes are busy reading that file.
> > 
> > Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag
> > that allows us to fix this usecase [1]. When set, it is possible to
> > rename a file over a preexisting file even when the target file still
> > has handles open. Those handles must have been opened with the
> > `FILE_SHARE_DELETE` flag, which we have ensured in the preceding
> > commits.
> > > Careful readers might have noticed that [1] does not mention the above
> > 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.
> 
> The Windows 10 SDK defines FILE_RENAME_FLAG_REPLACE_IF_EXISTS and
> FILE_RENAME_FLAG_POSIX_SEMANTICS for SetFileInformationByHandle(). That
> the documentation lacks "_FLAG_" in the names must be an error in the
> documentation.
> 
> I found the mention of FILE_RENAME_POSIX_SEMANTICS quite distracting,
> because it is a flag to be used with CreateFileW() and basically only
> has to do with case-sensitivity, but nothing with POSIX semantics of
> renaming.

I'd still prefer to mention this, because otherwise an astute reader
might notice that I'm using a different flag name than what is
documented in the docs and figure out that I defined the wrong flag
name.

[snip]
> > +		HANDLE old_handle = INVALID_HANDLE_VALUE;
> > +		BOOL success;
> > +
> > +		old_handle = CreateFileW(wpold, DELETE,
> > +					 FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
> > +					 NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
> > +		if (old_handle == INVALID_HANDLE_VALUE) {
> > +			errno = err_win_to_posix(GetLastError());
> > +			return -1;
> > +		}
> > +
> > +		rename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |
> > +				    FILE_RENAME_FLAG_POSIX_SEMANTICS;
> > +		rename_info.FileNameLength = wpnew_len * sizeof(WCHAR);
> 
> Size is in bytes, not in characters, and without the NUL. Good. I read
> one comment on SO, which said that this value is ignored...

Yeah, I noticed at one point that it didn't really make a difference
what I pass here.

> > +		memcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));
> 
> ... which makes it all the more important that this path is
> NUL-terminated. Yet, this does not copy the NUL. We are still good,
> because the buffer is zero-initialized and xutftowcs_path() ensures that
> wpnew_len is at most MAX_PATH-1.

Yup.

[snip]
> The general structure of the patch makes a lot of sense!

Great, thanks for your review! I'll send a revised version of this
series where I adapt the second patch.

Patrick

Re: [PATCH v2 2/3] compat/mingw: allow deletion of most opened files

[Patrick Steinhardt]

On Sun, Oct 27, 2024 at 02:17:36PM +0100, Johannes Sixt wrote:
> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index e326c6fcd2d..6c75fe36b15 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -532,6 +532,62 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
> >  	return fd;
> >  }
> >  
> > +/*
> > + * Ideally, we'd use `_wopen()` to implement this functionality so that we
> > + * don't have to reimplement it, but unfortunately we do not have tight control
> > + * over the share mode there. And while `_wsopen()` and friends exist that give
> > + * us _some_ control over the share mode, this family of functions doesn't give
> > + * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict
> > + * requirement for us though so that we can unlink or rename over files that
> > + * are held open by another process.
> > + *
> > + * We are thus forced to implement our own emulation of `open()`. To make our
> > + * life simpler we only implement basic support for this, namely opening
> > + * existing files for reading and/or writing. This means that newly created
> > + * files won't have their sharing mode set up correctly, but for now I couldn't
> > + * find any case where this matters. We may have to revisit that in the future
> > + * though based on our needs.
> > + */
> > +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
> > +{
> > +	SECURITY_ATTRIBUTES security_attributes = {
> > +		.nLength = sizeof(security_attributes),
> > +		.bInheritHandle = !(oflags & O_NOINHERIT),
> > +	};
> > +	HANDLE handle;
> > +	int access;
> 
> I would have made this variable DWORD or unsigned instead of plain int,
> because it receives a bit pattern and would match the parameter type of
> CreateFileW() better; but no big deal.

I have to reroll this series anyway, so I'll just fix this while at it.

> > +	int fd;
> > +
> > +	/* We only support basic flags. */
> > +	if (oflags & ~(O_ACCMODE | O_NOINHERIT)) {
> > +		errno = ENOSYS;
> > +		return -1;
> > +	}
> > +
> > +	if (oflags & O_RDWR)
> > +		access = GENERIC_READ | GENERIC_WRITE;
> > +	else if (oflags & O_WRONLY)
> > +		access = GENERIC_WRITE;
> > +	else
> > +		access = GENERIC_READ;
> 
> O_RDWR, O_WRONLY and O_RDONLY are not flags, but values occupying two
> bits of oflags. This must be:
> 
> 	if ((oflags & O_ACCMODE) == O_RDWR)
> 		access = GENERIC_READ | GENERIC_WRITE;
> 	else if ((oflags & O_ACCMODE) == O_WRONLY)
> 		access = GENERIC_WRITE;
> 	else
> 		access = GENERIC_READ;
> 
> or similar.

Ah, that makes sense indeed. Will fix.

> > +
> > +	handle = CreateFileW(filename, access,
> > +			     FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
> > +			     &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
> > +	if (handle == INVALID_HANDLE_VALUE) {
> > +		DWORD err = GetLastError();
> > +		if (err == ERROR_INVALID_PARAMETER)
> > +			err = ERROR_PATH_NOT_FOUND;
> 
> First I wondered what this is about, but then noticed that it is just
> copied from the mingw_open_append() implementation catering to some
> bogus network filesystems. Good.

I'll add a comment.

Patrick

[PATCH v3 0/3] compat/mingw: implement POSIX-style atomic renames

[Patrick Steinhardt]

Hi,

this is third version of my patch series that introduces POSIX-style
atomic renames on Windows to fix concurrent writes with the reftable
backend.

Changes compared to v2:

  - Use `DWORD` instead of `int` to store the `access` flags.

  - Fix handling of `oflags`.

  - Add a comment referring to `mingw_open_append()` for why convert
    `ERROR_INVALID_PARAMETER` to `ERROR_PATH_NOT_FOUND`.

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             | 157 +++++++++++++++++++++++++++++++++++--
 t/t0610-reftable-basics.sh |   8 +-
 2 files changed, 156 insertions(+), 9 deletions(-)

Range-diff against v2:
1:  63c2cfa87a4 = 1:  63c2cfa87a4 compat/mingw: share file handles created via `CreateFileW()`
2:  c308eafbbb5 ! 2:  91d434116f3 compat/mingw: allow deletion of most opened files
    @@ compat/mingw.c: static int mingw_open_append(wchar_t const *wfilename, int oflag
     +		.bInheritHandle = !(oflags & O_NOINHERIT),
     +	};
     +	HANDLE handle;
    -+	int access;
    ++	DWORD access;
     +	int fd;
     +
     +	/* We only support basic flags. */
    @@ compat/mingw.c: static int mingw_open_append(wchar_t const *wfilename, int oflag
     +		return -1;
     +	}
     +
    -+	if (oflags & O_RDWR)
    ++	switch (oflags & O_ACCMODE) {
    ++	case O_RDWR:
     +		access = GENERIC_READ | GENERIC_WRITE;
    -+	else if (oflags & O_WRONLY)
    ++		break;
    ++	case O_WRONLY:
     +		access = GENERIC_WRITE;
    -+	else
    ++		break;
    ++	default:
     +		access = GENERIC_READ;
    ++		break;
    ++	}
     +
     +	handle = CreateFileW(filename, access,
     +			     FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
     +			     &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
     +	if (handle == INVALID_HANDLE_VALUE) {
     +		DWORD err = GetLastError();
    ++
    ++		/* See `mingw_open_append()` for why we have this conversion. */
     +		if (err == ERROR_INVALID_PARAMETER)
     +			err = ERROR_PATH_NOT_FOUND;
    ++
     +		errno = err_win_to_posix(err);
     +		return -1;
     +	}
3:  ee1e92e593e = 3:  2447d332a85 compat/mingw: support POSIX semantics for atomic renames
-- 
2.47.0.118.gfd3785337b.dirty

[PATCH v3 1/3] compat/mingw: share file handles created via `CreateFileW()`

[Patrick Steinhardt]

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:

  - `FILE_SHARE_READ` allows a concurrent process to open the file for
    reading.

  - `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. 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()`.

    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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

[PATCH v3 2/3] compat/mingw: allow deletion of most opened files

[Patrick Steinhardt]

On Windows, we emulate open(3p) via `mingw_open()`. This function
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.

This function has a major downside though: it does not allow us to
specify the sharing mode. While there is `_wsopen()` that allows us to
pass sharing flags, those sharing flags are not the same `FILE_SHARE_*`
flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows
concurrent read- and write-access, but does not allow for concurrent
deletions. Unfortunately though, we have to allow concurrent deletions
if we want to have POSIX-style atomic renames on top of an existing file
that has open file handles.

Implement a new function that emulates open(3p) for existing files via
`CreateFileW()` such that we can set the required sharing flags.

While we have the same issue when calling open(3p) with `O_CREAT`,
implementing that mode would be more complex due to the required
permission handling. Furthermore, atomic updates via renames typically
write to exclusive lockfile and then perform the rename, and thus we
don't have to handle the case where the locked path has been created
with `O_CREATE`. So while it would be nice to have proper POSIX
semantics in all paths, we instead aim for a minimum viable fix here.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index e326c6fcd2d..0d9600543cb 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -532,6 +532,70 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 	return fd;
 }
 
+/*
+ * Ideally, we'd use `_wopen()` to implement this functionality so that we
+ * don't have to reimplement it, but unfortunately we do not have tight control
+ * over the share mode there. And while `_wsopen()` and friends exist that give
+ * us _some_ control over the share mode, this family of functions doesn't give
+ * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict
+ * requirement for us though so that we can unlink or rename over files that
+ * are held open by another process.
+ *
+ * We are thus forced to implement our own emulation of `open()`. To make our
+ * life simpler we only implement basic support for this, namely opening
+ * existing files for reading and/or writing. This means that newly created
+ * files won't have their sharing mode set up correctly, but for now I couldn't
+ * find any case where this matters. We may have to revisit that in the future
+ * though based on our needs.
+ */
+static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
+{
+	SECURITY_ATTRIBUTES security_attributes = {
+		.nLength = sizeof(security_attributes),
+		.bInheritHandle = !(oflags & O_NOINHERIT),
+	};
+	HANDLE handle;
+	DWORD access;
+	int fd;
+
+	/* We only support basic flags. */
+	if (oflags & ~(O_ACCMODE | O_NOINHERIT)) {
+		errno = ENOSYS;
+		return -1;
+	}
+
+	switch (oflags & O_ACCMODE) {
+	case O_RDWR:
+		access = GENERIC_READ | GENERIC_WRITE;
+		break;
+	case O_WRONLY:
+		access = GENERIC_WRITE;
+		break;
+	default:
+		access = GENERIC_READ;
+		break;
+	}
+
+	handle = CreateFileW(filename, access,
+			     FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
+			     &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+	if (handle == INVALID_HANDLE_VALUE) {
+		DWORD err = GetLastError();
+
+		/* See `mingw_open_append()` for why we have this conversion. */
+		if (err == ERROR_INVALID_PARAMETER)
+			err = ERROR_PATH_NOT_FOUND;
+
+		errno = err_win_to_posix(err);
+		return -1;
+	}
+
+	fd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);
+	if (fd < 0)
+		CloseHandle(handle);
+	return fd;
+}
+
 /*
  * Does the pathname map to the local named pipe filesystem?
  * That is, does it have a "//./pipe/" prefix?
@@ -567,6 +631,8 @@ int mingw_open (const char *filename, int oflags, ...)
 
 	if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))
 		open_fn = mingw_open_append;
+	else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))
+		open_fn = mingw_open_existing;
 	else
 		open_fn = _wopen;
 
-- 
2.47.0.118.gfd3785337b.dirty

[PATCH v3 3/3] compat/mingw: support POSIX semantics for atomic renames

[Patrick Steinhardt]

By default, Windows restricts access to files when those files have been
opened by another process. As explained in the preceding commits, these
restrictions can be loosened such that reads, writes and/or deletes of
files with open handles _are_ allowed.

While we set up those sharing flags in most relevant code paths now, we
still don't properly handle POSIX-style atomic renames in case the
target path is open. This is failure demonstrated by t0610, where one of
our tests spawns concurrent writes in a reftable-enabled repository and
expects all of them to succeed. This test fails most of the time because
the process that has acquired the "tables.list" lock is unable to rename
it into place while other processes are busy reading that file.

Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag
that allows us to fix this usecase [1]. When set, it is possible to
rename a file over a preexisting file even when the target file still
has handles open. Those handles must have been opened with the
`FILE_SHARE_DELETE` flag, which we have ensured in the preceding
commits.

Careful readers might have noticed that [1] does not mention the above
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.

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
to 0x0A00, which is Windows 10 and later, is not an option as it would
make it impossible to compile on Windows 8.1, which is still supported.
Instead, we have to manually declare the relevant infrastructure to make
this feature available and have fallback logic in place in case we run
on a Windows version that does not yet have this flag.

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 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.

This fixes concurrent writes in the reftable backend as demonstrated in
t0610, but may also end up fixing other usecases where Git wants to
perform renames.

[1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information
[2]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 compat/mingw.c             | 87 ++++++++++++++++++++++++++++++++++++--
 t/t0610-reftable-basics.sh |  8 ++--
 2 files changed, 88 insertions(+), 7 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 0d9600543cb..c4320769db6 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2217,10 +2217,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)
 #undef rename
 int mingw_rename(const char *pold, const char *pnew)
 {
+	static int supports_file_rename_info_ex = 1;
 	DWORD attrs, gle;
 	int tries = 0;
 	wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];
-	if (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)
+	int wpnew_len;
+
+	if (xutftowcs_path(wpold, pold) < 0)
+		return -1;
+	wpnew_len = xutftowcs_path(wpnew, pnew);
+	if (wpnew_len < 0)
 		return -1;
 
 	/*
@@ -2231,11 +2237,84 @@ int mingw_rename(const char *pold, const char *pnew)
 		return 0;
 	if (errno != EEXIST)
 		return -1;
+
 repeat:
-	if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
-		return 0;
+	if (supports_file_rename_info_ex) {
+		/*
+		 * Our minimum required Windows version is still set to Windows
+		 * Vista. We thus have to declare required infrastructure for
+		 * FileRenameInfoEx ourselves until we bump _WIN32_WINNT to
+		 * 0x0A00. Furthermore, we have to handle cases where the
+		 * FileRenameInfoEx call isn't supported yet.
+		 */
+#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS                  0x00000001
+#define FILE_RENAME_FLAG_POSIX_SEMANTICS                    0x00000002
+		FILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22;
+		struct {
+			/*
+			 * This is usually an unnamed union, but that is not
+			 * part of ISO C99. We thus inline the field, as we
+			 * really only care for the Flags field anyway.
+			 */
+			DWORD Flags;
+			HANDLE RootDirectory;
+			DWORD FileNameLength;
+			/*
+			 * The actual structure is defined with a single-character
+			 * flex array so that the structure has to be allocated on
+			 * the heap. As we declare this structure ourselves though
+			 * we can avoid the allocation and define FileName to have
+			 * MAX_PATH bytes.
+			 */
+			WCHAR FileName[MAX_PATH];
+		} rename_info = { 0 };
+		HANDLE old_handle = INVALID_HANDLE_VALUE;
+		BOOL success;
+
+		old_handle = CreateFileW(wpold, DELETE,
+					 FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
+					 NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+		if (old_handle == INVALID_HANDLE_VALUE) {
+			errno = err_win_to_posix(GetLastError());
+			return -1;
+		}
+
+		rename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |
+				    FILE_RENAME_FLAG_POSIX_SEMANTICS;
+		rename_info.FileNameLength = wpnew_len * sizeof(WCHAR);
+		memcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));
+
+		success = SetFileInformationByHandle(old_handle, FileRenameInfoEx,
+						     &rename_info, sizeof(rename_info));
+		gle = GetLastError();
+		CloseHandle(old_handle);
+		if (success)
+			return 0;
+
+		/*
+		 * When we see ERROR_INVALID_PARAMETER we can assume that the
+		 * current system doesn't support FileRenameInfoEx. Keep us
+		 * from using it in future calls and retry.
+		 */
+		if (gle == ERROR_INVALID_PARAMETER) {
+			supports_file_rename_info_ex = 0;
+			goto repeat;
+		}
+
+		/*
+		 * In theory, we shouldn't get ERROR_ACCESS_DENIED because we
+		 * always open files with FILE_SHARE_DELETE But in practice we
+		 * cannot assume that Git is the only one accessing files, and
+		 * other applications may not set FILE_SHARE_DELETE. So we have
+		 * to retry.
+		 */
+	} else {
+		if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
+			return 0;
+		gle = GetLastError();
+	}
+
 	/* TODO: translate more errors */
-	gle = GetLastError();
 	if (gle == ERROR_ACCESS_DENIED &&
 	    (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {
 		if (attrs & FILE_ATTRIBUTE_DIRECTORY) {
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index babec7993e3..eaf6fab6d29 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '
 	)
 '
 
-# This test fails most of the time on Windows systems. The root cause is
+# This test fails most of the time on Cygwin systems. The root cause is
 # that Windows does not allow us to rename the "tables.list.lock" file into
-# place when "tables.list" is open for reading by a concurrent process.
-test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '
+# place when "tables.list" is open for reading by a concurrent process. We have
+# worked around that in our MinGW-based rename emulation, but the Cygwin
+# emulation seems to be insufficient.
+test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
 	(
-- 
2.47.0.118.gfd3785337b.dirty

Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames

[Johannes Sixt]

Am 27.10.24 um 16:38 schrieb Patrick Steinhardt:
> On Sun, Oct 27, 2024 at 02:23:28PM +0100, Johannes Sixt wrote:
>> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:
>>> Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag
>>> that allows us to fix this usecase [1]. When set, it is possible to
>>> rename a file over a preexisting file even when the target file still
>>> has handles open. Those handles must have been opened with the
>>> `FILE_SHARE_DELETE` flag, which we have ensured in the preceding
>>> commits.
>>>> Careful readers might have noticed that [1] does not mention the above
>>> 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.
>>
>> The Windows 10 SDK defines FILE_RENAME_FLAG_REPLACE_IF_EXISTS and
>> FILE_RENAME_FLAG_POSIX_SEMANTICS for SetFileInformationByHandle(). That
>> the documentation lacks "_FLAG_" in the names must be an error in the
>> documentation.
>>
>> I found the mention of FILE_RENAME_POSIX_SEMANTICS quite distracting,
>> because it is a flag to be used with CreateFileW() and basically only
>> has to do with case-sensitivity, but nothing with POSIX semantics of
>> renaming.
> 
> I'd still prefer to mention this, because otherwise an astute reader
> might notice that I'm using a different flag name than what is
> documented in the docs and figure out that I defined the wrong flag
> name.

Ah, I was confused twice here. First, the documentation that you cite[*]
mentions FILE_RENAME_POSIX_SEMANTICS, but the name does not exist at
all. There does exist FILE_RENAME_FLAG_POSIX_SEMANTICS. The
documentation is just wrong. And in my earlier comment I copied the
inexistent flag name.

But I meant to cite this flag: FILE_FLAG_POSIX_SEMANTICS (no "RENAME").
It exists and is for CreateFileW().

Perhaps you also meant cite the latter one as the flag that "is not for
use with `SetFileInformationByHandle()`"?

At any rate, the paragraph as written isn't correct.

[1]:
https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information

-- Hannes

Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames

[Patrick Steinhardt]

On Sun, Oct 27, 2024 at 05:31:00PM +0100, Johannes Sixt wrote:
> Am 27.10.24 um 16:38 schrieb Patrick Steinhardt:
> > On Sun, Oct 27, 2024 at 02:23:28PM +0100, Johannes Sixt wrote:
> >> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:
> >>> Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag
> >>> that allows us to fix this usecase [1]. When set, it is possible to
> >>> rename a file over a preexisting file even when the target file still
> >>> has handles open. Those handles must have been opened with the
> >>> `FILE_SHARE_DELETE` flag, which we have ensured in the preceding
> >>> commits.
> >>>> Careful readers might have noticed that [1] does not mention the above
> >>> 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.
> >>
> >> The Windows 10 SDK defines FILE_RENAME_FLAG_REPLACE_IF_EXISTS and
> >> FILE_RENAME_FLAG_POSIX_SEMANTICS for SetFileInformationByHandle(). That
> >> the documentation lacks "_FLAG_" in the names must be an error in the
> >> documentation.
> >>
> >> I found the mention of FILE_RENAME_POSIX_SEMANTICS quite distracting,
> >> because it is a flag to be used with CreateFileW() and basically only
> >> has to do with case-sensitivity, but nothing with POSIX semantics of
> >> renaming.
> > 
> > I'd still prefer to mention this, because otherwise an astute reader
> > might notice that I'm using a different flag name than what is
> > documented in the docs and figure out that I defined the wrong flag
> > name.
> 
> Ah, I was confused twice here. First, the documentation that you cite[*]
> mentions FILE_RENAME_POSIX_SEMANTICS, but the name does not exist at
> all. There does exist FILE_RENAME_FLAG_POSIX_SEMANTICS. The
> documentation is just wrong. And in my earlier comment I copied the
> inexistent flag name.
> 
> But I meant to cite this flag: FILE_FLAG_POSIX_SEMANTICS (no "RENAME").
> It exists and is for CreateFileW().
> 
> Perhaps you also meant cite the latter one as the flag that "is not for
> use with `SetFileInformationByHandle()`"?
> 
> At any rate, the paragraph as written isn't correct.

I think I'm missing something. That's what the paragraph says:

    Careful readers might have noticed that [1] does not mention the above
    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.

And I'd claim it is correct.

`FILE_RENAME_POSIX_SEMANTICS` exists, this it is not a documentation
error. It is at a lower level than `FILE_RENAME_FLAG_POSIX_SEMANTICS`,
the documentation at [1] refers to "ntifs.h", which is part of the
Windows Driver Kit interfaces. So it is not supposed to be used with
`SetFileInformationByHandle()`, but with `FtlSetInformationFile()` [2],
which _also_ has a separate `FILE_RENAME_INFO` structure that looks the
same as `FILE_RENAME_INFO` defined for `SetFileInformationByHandle()`.
The only difference as far as I can tell is that the flags used for
these structures have slightly different names.

Now I totally get your confusion -- I have been extremely confused by
all of this, as well. It certainly is a documentation error that the
respective `FILE_RENAME_FLAG_POSIX_SEMANTICS` is undocumented, but
having proper docs for this is rather important such that the reader
knows what its behaviour is. So I have no other choice than to link to
the ntifs interfaces, as it documents the actual behaviour, even though
it lives in a different part of the Windows APIs.

Patrick

[1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information
[2]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/fltkernel/nf-fltkernel-fltsetinformationfile

Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames

[Johannes Sixt]

Am 27.10.24 um 18:27 schrieb Patrick Steinhardt:
> On Sun, Oct 27, 2024 at 05:31:00PM +0100, Johannes Sixt wrote:
>> Am 27.10.24 um 16:38 schrieb Patrick Steinhardt:
>>> On Sun, Oct 27, 2024 at 02:23:28PM +0100, Johannes Sixt wrote:
>>>> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:
>>>>> Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag
>>>>> that allows us to fix this usecase [1]. When set, it is possible to
>>>>> rename a file over a preexisting file even when the target file still
>>>>> has handles open. Those handles must have been opened with the
>>>>> `FILE_SHARE_DELETE` flag, which we have ensured in the preceding
>>>>> commits.
>>>>>> Careful readers might have noticed that [1] does not mention the above
>>>>> 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.
>>>>
>>>> The Windows 10 SDK defines FILE_RENAME_FLAG_REPLACE_IF_EXISTS and
>>>> FILE_RENAME_FLAG_POSIX_SEMANTICS for SetFileInformationByHandle(). That
>>>> the documentation lacks "_FLAG_" in the names must be an error in the
>>>> documentation.
>>>>
>>>> I found the mention of FILE_RENAME_POSIX_SEMANTICS quite distracting,
>>>> because it is a flag to be used with CreateFileW() and basically only
>>>> has to do with case-sensitivity, but nothing with POSIX semantics of
>>>> renaming.
>>>
>>> I'd still prefer to mention this, because otherwise an astute reader
>>> might notice that I'm using a different flag name than what is
>>> documented in the docs and figure out that I defined the wrong flag
>>> name.
>>
>> Ah, I was confused twice here. First, the documentation that you cite[*]
>> mentions FILE_RENAME_POSIX_SEMANTICS, but the name does not exist at
>> all. There does exist FILE_RENAME_FLAG_POSIX_SEMANTICS. The
>> documentation is just wrong. And in my earlier comment I copied the
>> inexistent flag name.
>>
>> But I meant to cite this flag: FILE_FLAG_POSIX_SEMANTICS (no "RENAME").
>> It exists and is for CreateFileW().
>>
>> Perhaps you also meant cite the latter one as the flag that "is not for
>> use with `SetFileInformationByHandle()`"?
>>
>> At any rate, the paragraph as written isn't correct.
> 
> I think I'm missing something. That's what the paragraph says:
> 
>     Careful readers might have noticed that [1] does not mention the above
>     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.
> 
> And I'd claim it is correct.
> 
> `FILE_RENAME_POSIX_SEMANTICS` exists, this it is not a documentation
> error. It is at a lower level than `FILE_RENAME_FLAG_POSIX_SEMANTICS`,
> the documentation at [1] refers to "ntifs.h", which is part of the
> Windows Driver Kit interfaces. So it is not supposed to be used with
> `SetFileInformationByHandle()`, but with `FtlSetInformationFile()` [2],
> which _also_ has a separate `FILE_RENAME_INFO` structure that looks the
> same as `FILE_RENAME_INFO` defined for `SetFileInformationByHandle()`.
> The only difference as far as I can tell is that the flags used for
> these structures have slightly different names.
> 
> Now I totally get your confusion -- I have been extremely confused by
> all of this, as well. It certainly is a documentation error that the
> respective `FILE_RENAME_FLAG_POSIX_SEMANTICS` is undocumented, but
> having proper docs for this is rather important such that the reader
> knows what its behaviour is. So I have no other choice than to link to
> the ntifs interfaces, as it documents the actual behaviour, even though
> it lives in a different part of the Windows APIs.
> > Patrick
> 
> [1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information
> [2]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/fltkernel/nf-fltkernel-fltsetinformationfile

OK, then let's leave the text as it is.

Thanks,
-- Hannes

Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`

[Taylor Blau]

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

Re: [PATCH v2 2/3] compat/mingw: allow deletion of most opened files

[Taylor Blau]

On Sun, Oct 27, 2024 at 04:38:50PM +0100, Patrick Steinhardt wrote:
> > > +	int fd;
> > > +
> > > +	/* We only support basic flags. */
> > > +	if (oflags & ~(O_ACCMODE | O_NOINHERIT)) {
> > > +		errno = ENOSYS;
> > > +		return -1;
> > > +	}
> > > +
> > > +	if (oflags & O_RDWR)
> > > +		access = GENERIC_READ | GENERIC_WRITE;
> > > +	else if (oflags & O_WRONLY)
> > > +		access = GENERIC_WRITE;
> > > +	else
> > > +		access = GENERIC_READ;
> >
> > O_RDWR, O_WRONLY and O_RDONLY are not flags, but values occupying two
> > bits of oflags. This must be:
> >
> > 	if ((oflags & O_ACCMODE) == O_RDWR)
> > 		access = GENERIC_READ | GENERIC_WRITE;
> > 	else if ((oflags & O_ACCMODE) == O_WRONLY)
> > 		access = GENERIC_WRITE;
> > 	else
> > 		access = GENERIC_READ;
> >
> > or similar.
>
> Ah, that makes sense indeed. Will fix.

It may be nice to write this as a switch statement, since we're always
comparing the value of oflags & O_ACCMODE, like so:

    switch (oflags & O_ACCMODE) {
    case O_RDWR:
        access = GENERIC_READ | GENERIC_WRITE;
        break;
    case O_WRONLY:
        access = GENERIC_WRITE;
        break;
    default:
        access = GENERIC_READ;
        break;
    }

, but it is a minor point and I certainly do not have very strong
feelings here.

Thanks,
Taylor

Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames

[Taylor Blau]

On Sun, Oct 27, 2024 at 10:36:55PM +0100, Johannes Sixt wrote:
> >> Ah, I was confused twice here. First, the documentation that you cite[*]
> >> mentions FILE_RENAME_POSIX_SEMANTICS, but the name does not exist at
> >> all. There does exist FILE_RENAME_FLAG_POSIX_SEMANTICS. The
> >> documentation is just wrong. And in my earlier comment I copied the
> >> inexistent flag name.
> >>
> >> But I meant to cite this flag: FILE_FLAG_POSIX_SEMANTICS (no "RENAME").
> >> It exists and is for CreateFileW().
> >>
> >> Perhaps you also meant cite the latter one as the flag that "is not for
> >> use with `SetFileInformationByHandle()`"?
> >>
> >> At any rate, the paragraph as written isn't correct.
> >
> > I think I'm missing something. That's what the paragraph says:
> >
> >     Careful readers might have noticed that [1] does not mention the above
> >     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.
> >
> > And I'd claim it is correct.
> >
> > [snip]
> >
> > [1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information
> > [2]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/fltkernel/nf-fltkernel-fltsetinformationfile
>
> OK, then let's leave the text as it is.

Thanks, both, for vetting it carefully.

Thanks,
Taylor

Re: [PATCH v2 2/3] compat/mingw: allow deletion of most opened files

[Taylor Blau]

On Sun, Oct 27, 2024 at 07:48:35PM -0400, Taylor Blau wrote:
> It may be nice to write this as a switch statement, since we're always
> comparing the value of oflags & O_ACCMODE, like so:
>
>     switch (oflags & O_ACCMODE) {
>     case O_RDWR:
>         access = GENERIC_READ | GENERIC_WRITE;
>         break;
>     case O_WRONLY:
>         access = GENERIC_WRITE;
>         break;
>     default:
>         access = GENERIC_READ;
>         break;
>     }
>
> , but it is a minor point and I certainly do not have very strong
> feelings here.

...and I see that's exactly what you went with in the subsequent round
;-).

Thanks,
Taylor

Re: [PATCH v3 0/3] compat/mingw: implement POSIX-style atomic renames

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this is third version of my patch series that introduces POSIX-style
> atomic renames on Windows to fix concurrent writes with the reftable
> backend.
>
> Changes compared to v2:
>
>   - Use `DWORD` instead of `int` to store the `access` flags.
>
>   - Fix handling of `oflags`.
>
>   - Add a comment referring to `mingw_open_append()` for why convert
>     `ERROR_INVALID_PARAMETER` to `ERROR_PATH_NOT_FOUND`.
>
> Thanks!
>
> Patrick

Thanks, Patrick, for listing those who gave reviews on the previous
iterations of these patches.

Reviewers, how does this round look to you?  Is this a version you
folks are comfortable to have in 'next' and later in 'master'?

Thanks.

Re: [PATCH v3 0/3] compat/mingw: implement POSIX-style atomic renames

[Johannes Sixt]

Am 06.11.24 um 04:54 schrieb Junio C Hamano:
> Reviewers, how does this round look to you?  Is this a version you
> folks are comfortable to have in 'next' and later in 'master'?

This round looks fine!

Reviewed-by: Johannes Sixt <j6t@kdbg.org>

-- Hannes

Re: [PATCH v3 0/3] compat/mingw: implement POSIX-style atomic renames

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 06.11.24 um 04:54 schrieb Junio C Hamano:
>> Reviewers, how does this round look to you?  Is this a version you
>> folks are comfortable to have in 'next' and later in 'master'?
>
> This round looks fine!
>
> Reviewed-by: Johannes Sixt <j6t@kdbg.org>

Thanks.