Skip to content

Commit 391bcea

Browse files
pks-tgitster
authored andcommitted
compat/mingw: support POSIX semantics for atomic renames
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> Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent a270cb1 commit 391bcea

File tree

2 files changed

+88
-7
lines changed

2 files changed

+88
-7
lines changed

compat/mingw.c

+83-4
Original file line numberDiff line numberDiff line change
@@ -2217,10 +2217,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)
22172217
#undef rename
22182218
int mingw_rename(const char *pold, const char *pnew)
22192219
{
2220+
static int supports_file_rename_info_ex = 1;
22202221
DWORD attrs, gle;
22212222
int tries = 0;
22222223
wchar_t wpold[MAX_PATH], wpnew[MAX_PATH];
2223-
if (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)
2224+
int wpnew_len;
2225+
2226+
if (xutftowcs_path(wpold, pold) < 0)
2227+
return -1;
2228+
wpnew_len = xutftowcs_path(wpnew, pnew);
2229+
if (wpnew_len < 0)
22242230
return -1;
22252231

22262232
/*
@@ -2231,11 +2237,84 @@ int mingw_rename(const char *pold, const char *pnew)
22312237
return 0;
22322238
if (errno != EEXIST)
22332239
return -1;
2240+
22342241
repeat:
2235-
if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
2236-
return 0;
2242+
if (supports_file_rename_info_ex) {
2243+
/*
2244+
* Our minimum required Windows version is still set to Windows
2245+
* Vista. We thus have to declare required infrastructure for
2246+
* FileRenameInfoEx ourselves until we bump _WIN32_WINNT to
2247+
* 0x0A00. Furthermore, we have to handle cases where the
2248+
* FileRenameInfoEx call isn't supported yet.
2249+
*/
2250+
#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS 0x00000001
2251+
#define FILE_RENAME_FLAG_POSIX_SEMANTICS 0x00000002
2252+
FILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22;
2253+
struct {
2254+
/*
2255+
* This is usually an unnamed union, but that is not
2256+
* part of ISO C99. We thus inline the field, as we
2257+
* really only care for the Flags field anyway.
2258+
*/
2259+
DWORD Flags;
2260+
HANDLE RootDirectory;
2261+
DWORD FileNameLength;
2262+
/*
2263+
* The actual structure is defined with a single-character
2264+
* flex array so that the structure has to be allocated on
2265+
* the heap. As we declare this structure ourselves though
2266+
* we can avoid the allocation and define FileName to have
2267+
* MAX_PATH bytes.
2268+
*/
2269+
WCHAR FileName[MAX_PATH];
2270+
} rename_info = { 0 };
2271+
HANDLE old_handle = INVALID_HANDLE_VALUE;
2272+
BOOL success;
2273+
2274+
old_handle = CreateFileW(wpold, DELETE,
2275+
FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
2276+
NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
2277+
if (old_handle == INVALID_HANDLE_VALUE) {
2278+
errno = err_win_to_posix(GetLastError());
2279+
return -1;
2280+
}
2281+
2282+
rename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |
2283+
FILE_RENAME_FLAG_POSIX_SEMANTICS;
2284+
rename_info.FileNameLength = wpnew_len * sizeof(WCHAR);
2285+
memcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));
2286+
2287+
success = SetFileInformationByHandle(old_handle, FileRenameInfoEx,
2288+
&rename_info, sizeof(rename_info));
2289+
gle = GetLastError();
2290+
CloseHandle(old_handle);
2291+
if (success)
2292+
return 0;
2293+
2294+
/*
2295+
* When we see ERROR_INVALID_PARAMETER we can assume that the
2296+
* current system doesn't support FileRenameInfoEx. Keep us
2297+
* from using it in future calls and retry.
2298+
*/
2299+
if (gle == ERROR_INVALID_PARAMETER) {
2300+
supports_file_rename_info_ex = 0;
2301+
goto repeat;
2302+
}
2303+
2304+
/*
2305+
* In theory, we shouldn't get ERROR_ACCESS_DENIED because we
2306+
* always open files with FILE_SHARE_DELETE But in practice we
2307+
* cannot assume that Git is the only one accessing files, and
2308+
* other applications may not set FILE_SHARE_DELETE. So we have
2309+
* to retry.
2310+
*/
2311+
} else {
2312+
if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))
2313+
return 0;
2314+
gle = GetLastError();
2315+
}
2316+
22372317
/* TODO: translate more errors */
2238-
gle = GetLastError();
22392318
if (gle == ERROR_ACCESS_DENIED &&
22402319
(attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {
22412320
if (attrs & FILE_ATTRIBUTE_DIRECTORY) {

t/t0610-reftable-basics.sh

+5-3
Original file line numberDiff line numberDiff line change
@@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '
450450
)
451451
'
452452

453-
# This test fails most of the time on Windows systems. The root cause is
453+
# This test fails most of the time on Cygwin systems. The root cause is
454454
# that Windows does not allow us to rename the "tables.list.lock" file into
455-
# place when "tables.list" is open for reading by a concurrent process.
456-
test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '
455+
# place when "tables.list" is open for reading by a concurrent process. We have
456+
# worked around that in our MinGW-based rename emulation, but the Cygwin
457+
# emulation seems to be insufficient.
458+
test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '
457459
test_when_finished "rm -rf repo" &&
458460
git init repo &&
459461
(

0 commit comments

Comments
 (0)