Re: [PATCH v2 2/2] compat/mingw: fix EACCESS when opening files with `O_CREAT | O_EXCL`

> In our CI systems we can observe that t0610 fails rather frequently.
> This testcase races a bunch of git-update-ref(1) processes with one
> another which are all trying to update a unique reference, where we
> expect that all processes succeed and end up updating the reftable
> stack. The error message in this case looks like the following:
>
>     fatal: update_ref failed for ref 'refs/heads/branch-88': reftable: transaction prepare: I/O error
>
> Instrumenting the code with a couple of calls to `BUG()` in relevant
> sites where we return `REFTABLE_IO_ERROR` quickly leads one to discover
> that this error is caused when calling `flock_acquire()`, which is a
> thin wrapper around our lockfile API. Curiously, the error code we get
> in such cases is `EACCESS`, indicating that we are not allowed to access
> the file.
>
> The root cause of this is an oddity of `CreateFileW()`, which is what
> `_wopen()` uses internally. Quoting its documentation [1]:
>
>     If you call CreateFile on a file that is pending deletion as a
>     result of a previous call to DeleteFile, the function fails. The
>     operating system delays file deletion until all handles to the file
>     are closed. GetLastError returns ERROR_ACCESS_DENIED.
>
> This behaviour is triggered quite often in the above testcase because
> all the processes race with one another trying to acquire the lock for
> the "tables.list" file. This is due to how locking works in the reftable
> library when compacting a stack:
>
>     1. Lock the "tables.list" file and reads its contents.
>
>     2. Decide which tables to compact.
>
>     3. Lock each of the individual tables that we are about to compact.
>
>     4. Unlock the "tables.list" file.
>
>     5. Compact the individual tables into one large table.
>
>     6. Re-lock the "tables.list" file.
>
>     7. Write the new list of tables into it.
>
>     8. Commit the "tables.list" file.
>
> The important step is (4): we don't commit the file directly by renaming
> it into place, but instead we delete the lockfile so that concurrent
> processes can continue to append to the reftable stack while we compact
> the tables. And because we use `DeleteFileW()` to do so, we may now race
> with another process that wants to acquire that lockfile. So if we are
> unlucky, we would now see `ERROR_ACCESS_DENIED` instead of the expected
> `ERROR_FILE_EXISTS`, which the lockfile subsystem isn't prepared to
> handle and thus it will bail out without retrying to acquire the lock.
>
> In theory, the issue is not limited to the reftable library and can be
> triggered by every other user of the lockfile subsystem, as well. My gut
> feeling tells me it's rather unlikely to surface elsewhere though.
>
> Fix the issue by translating the error to `EEXIST`. This makes the
> lockfile subsystem handle the error correctly: in case a timeout is set
> it will now retry acquiring the lockfile until the timeout has expired.
>
> With this, t0610 is now always passing on my machine whereas it was
> previously failing in around 20-30% of all test runs.
>
> [1]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  compat/mingw.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index f524c54d06d..50c80b1b750 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -21,6 +21,9 @@
>  #include "gettext.h"
>  #define SECURITY_WIN32
>  #include <sspi.h>
> +#include <winternl.h>
> +
> +#define STATUS_DELETE_PENDING ((NTSTATUS) 0xC0000056)
>
>  #define HCAST(type, handle) ((type)(intptr_t)handle)
>
> @@ -621,6 +624,8 @@ int mingw_open (const char *filename, int oflags, ...)
>  	wchar_t wfilename[MAX_PATH];
>  	open_fn_t open_fn;
>
> +	DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NTAPI, RtlGetLastNtStatus, void);
> +
>  	va_start(args, oflags);
>  	mode = va_arg(args, int);
>  	va_end(args);
> @@ -644,6 +649,21 @@ int mingw_open (const char *filename, int oflags, ...)
>
>  	fd = open_fn(wfilename, oflags, mode);
>
> +	/*
> +	 * Internally, `_wopen()` uses the `CreateFile()` API with CREATE_NEW,
> +	 * which may error out with ERROR_ACCESS_DENIED and an NtStatus of
> +	 * STATUS_DELETE_PENDING when the file is scheduled for deletion via
> +	 * `DeleteFileW()`. The file essentially exists, so we map errno to
> +	 * EEXIST instead of EACCESS so that callers don't have to special-case
> +	 * this.
> +	 *
> +	 * This fixes issues for example with the lockfile interface when one
> +	 * process has a lock that it is about to commit or release while
> +	 * another process wants to acquire it.
> +	 */
> +	if (fd < 0 && create && GetLastError() == ERROR_ACCESS_DENIED &&
> +	    INIT_PROC_ADDR(RtlGetLastNtStatus) && RtlGetLastNtStatus() == STATUS_DELETE_PENDING)
> +		errno = EEXIST;
>  	if (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {
>  		DWORD attrs = GetFileAttributesW(wfilename);
>  		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))
>
> --
> 2.49.0.472.ge94155a9ec.dirty
>
>