{"thread":{"id":"62397","subject":"[PATCH 0/3] compat/mingw: implement POSIX-style atomic renames","startedAt":"2024-10-23T15:05:02Z","lastAt":"2024-11-06T12:09:16Z","messageCount":44,"participants":["Patrick Steinhardt","Taylor Blau","Kristoffer Haugsbakk","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"505928","messageId":"cover.1729695349.git.ps@pks.im","threadId":"62397","inReplyTo":null,"subject":"[PATCH 0/3] compat/mingw: implement POSIX-style atomic renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-23T15:04:55Z","receivedAt":"2024-10-23T15:05:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nso after chatting with Johannes recently I decided to have another stab\nat the failures we've seen on Windows with the reftable backend. A quick\nrefresher: reftables use a central \"tables.lock\" file that gets updated\nvia renames such that the update itself is atomic. This causes issues on\nWindows when there are many concurrent readers/writers because Windows\ndoes not allow you to rename a file over another file that is held open\nby another process by default. This issue is surfaced by t0610, where we\nspawn a 100 concurrent git-update-ref(1) processes.\n\nThis patch series aims to fix the issue on Windows by implementing\nPOSIX-style atomic renames. The first two patches refactor the code such\nthat we use `FILE_SHARE_DELETE` to open files, which allows other\nprocesses to delete the file even while we have an open handle. This\nsharing mode is also important in the context of renames, which are\ntreated like a deletion of the replaced file. The third patch then\nreimplements renames via `SetFileAttributesByHandle(FileRenameInfoEx)`,\nwhich has a flag `FILE_RENAME_FLAG_POSIX_SEMANTICS`. When set, Windows\nallows us to replace files during a rename which still have an open\nhandle.\n\nThere's one catch: this is only supported in Windows 10 and newer. On\none hand this means that we have to provide the required definitions\nourselves, as we cannot bump the Windows SDK version. On the other hand\nwe also have to support the case where older Windows versions now fail\nat runtime due to `FileRenameInfoEx` being unsupported. But overall this\nisn't too bad, I'd say. Johannes has also kindly tested the fallback\nlogic for me on Windows 8.1.\n\nWith these changes t0610 now passes on my Windows 10 machine, but this\nmay also have a positive impact on other parts of Git where we might\nhave previously failed.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n compat/mingw: share file handles created via `CreateFileW()`\n compat/mingw: allow deletion of most opened files\n compat/mingw: support POSIX semantics for atomic renames\n\n compat/mingw.c | 146 +++++++++++++++++++++++++++++++++++--\n t/t0610-reftable-basics.sh | 8 +-\n 2 files changed, 145 insertions(+), 9 deletions(-)\n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"505929","messageId":"907576d23d1dc39b679a323e74b6bfb227d6c17d.1729695349.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1729695349.git.ps@pks.im","subject":"[PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-23T15:04:58Z","receivedAt":"2024-10-23T15:05:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Unless told otherwise, Windows will keep other processes from reading,\nwriting and deleting files when one has an open handle that was created\nvia `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\nflags such that other processes _can_ read and/or modify such a file.\nThis sharing mechanism is quite important in the context of Git, as we\nassume POSIX semantics all over the place.\n\nThere are two calls where we don't set up those flags though:\n\n - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n via `mingw_open_append()`. This makes it impossible to delete the\n file from another process or to replace it via an atomic rename.\n\n - When opening a file such that we can change its access/modification\n times. This makes it impossible to perform any kind of operation\n on this file at all from another process. While we only open the\n file for a short amount of time to update its timestamps, this still\n opens us up for a race condition with another process.\n\nAdapt both of these callsites to pass all three sharing flags.\n\nSigned-off-by: Patrick Steinhardt \n---\n compat/mingw.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 0e851ecae29..e326c6fcd2d 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -502,7 +502,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n \t * to append to the file.\n \t */\n \thandle = CreateFileW(wfilename, FILE_APPEND_DATA,\n-\t\t\tFILE_SHARE_WRITE | FILE_SHARE_READ,\n+\t\t\tFILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n \t\t\tNULL, create, FILE_ATTRIBUTE_NORMAL, NULL);\n \tif (handle == INVALID_HANDLE_VALUE) {\n \t\tDWORD err = GetLastError();\n@@ -1006,7 +1006,7 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \n \tosfilehandle = CreateFileW(wfilename,\n \t\t\t\t FILE_WRITE_ATTRIBUTES,\n-\t\t\t\t 0 /*FileShare.None*/,\n+\t\t\t\t FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,\n \t\t\t\t NULL,\n \t\t\t\t OPEN_EXISTING,\n \t\t\t\t (attrs != INVALID_FILE_ATTRIBUTES &&\n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"505930","messageId":"3552feddb332b31744c2ab723b112b9b08926436.1729695349.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1729695349.git.ps@pks.im","subject":"[PATCH 2/3] compat/mingw: allow deletion of most opened files","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-23T15:05:00Z","receivedAt":"2024-10-23T15:05:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Windows, we emulate open(3p) via `mingw_open()`. This function\nimplements handling of some platform- specific quirks that are required\nto make it behave as closely as possible like open(3p) would, but for\nmost cases we just call the Windows-specific `_wopen()` function.\n\nThis function has a major downside though: it does not allow us to\nspecify the sharing mode. While there is `_wsopen()` that allows us to\npass sharing flags, those sharing flags are not the same `FILE_SHARE_*`\nflags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows\nconcurrent read- and write-access, but does not allow for concurrent\ndeletions. Unfortunately though, we have to allow concurrent deletions\nif we want to have POSIX-style atomic renames on top of an existing file\nthat has open file handles.\n\nImplement a new function that emulates open(3p) for existing files via\n`CreateFileW()` such that we can set the required sharing flags.\n\nWhile we have the same issue when calling open(3p) with `O_CREAT`,\nimplementing that mode would be more complex due to the required\npermission handling. Furthermore, atomic updates via renames typically\nwrite to exclusive lockfile and then perform the rename, and thus we\ndon't have to handle the case where the locked path has been created\nwith `O_CREATE`. So while it would be nice to have proper POSIX\nsemantics in all paths, we instead aim for a minimum viable fix here.\n\nSigned-off-by: Patrick Steinhardt \n---\n compat/mingw.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 55 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex e326c6fcd2d..ce95f3a5968 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -532,6 +532,59 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n \treturn fd;\n }\n \n+/*\n+ * Ideally, we'd use `_wopen()` to implement this functionality so that we\n+ * don't have to reimplement it, but unfortunately we do not have tight control\n+ * over the share mode there. And while `_wsopen()` and friends exist that give\n+ * us _some_ control over the share mode, this family of functions doesn't give\n+ * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict\n+ * requirement for us though so that we can unlink or rename over files that\n+ * are held open by another process.\n+ *\n+ * We are thus forced to implement our own emulation of `open()`. To make our\n+ * life simpler we only implement basic support for this, namely opening\n+ * existing files for reading and/or writing. This means that newly created\n+ * files won't have their sharing mode set up correctly, but for now I couldn't\n+ * find any case where this matters. We may have to revisit that in the future\n+ * though based on our needs.\n+ */\n+static int mingw_open_existing(const wchar_t *filename, int oflags, ...)\n+{\n+\tSECURITY_ATTRIBUTES security_attributes = {\n+\t .nLength = sizeof(security_attributes),\n+\t .bInheritHandle = !(oflags & O_NOINHERIT),\n+\t};\n+\tHANDLE handle;\n+\tint access;\n+\tint fd;\n+\n+\t/* We only support basic flags. */\n+\tif (oflags & ~(O_ACCMODE | O_NOINHERIT))\n+\t\treturn errno = ENOSYS, -1;\n+\tif (oflags & O_RDWR)\n+\t\taccess = GENERIC_READ | GENERIC_WRITE;\n+\telse if (oflags & O_WRONLY)\n+\t\taccess = GENERIC_WRITE;\n+\telse\n+\t\taccess = GENERIC_READ;\n+\n+\thandle = CreateFileW(filename, access,\n+\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n+\t\t\t &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n+\tif (handle == INVALID_HANDLE_VALUE) {\n+\t\tDWORD err = GetLastError();\n+\t\tif (err == ERROR_INVALID_PARAMETER)\n+\t\t\terr = ERROR_PATH_NOT_FOUND;\n+\t\terrno = err_win_to_posix(err);\n+\t\treturn -1;\n+\t}\n+\n+\tfd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);\n+\tif (fd < 0)\n+\t\tCloseHandle(handle);\n+\treturn fd;\n+}\n+\n /*\n * Does the pathname map to the local named pipe filesystem?\n * That is, does it have a \"//./pipe/\" prefix?\n@@ -567,6 +620,8 @@ int mingw_open (const char *filename, int oflags, ...)\n \n \tif ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))\n \t\topen_fn = mingw_open_append;\n+\telse if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))\n+\t\topen_fn = mingw_open_existing;\n \telse\n \t\topen_fn = _wopen;\n \n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"505931","messageId":"d17ca1c7ce38e12fe113f15b078c12bc92e8f0aa.1729695349.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1729695349.git.ps@pks.im","subject":"[PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-23T15:05:03Z","receivedAt":"2024-10-23T15:05:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"By default, Windows restricts access to files when those files have been\nopened by another process. As explained in the preceding commits, these\nrestrictions can be loosened such that reads, writes and/or deletes of\nfiles with open handles _are_ allowed.\n\nWhile we set up those sharing flags in most relevant code paths now, we\nstill don't properly handle POSIX-style atomic renames in case the\ntarget path is open. This is failure demonstrated by t0610, where one of\nour tests spawns concurrent writes in a reftable-enabled repository and\nexpects all of them to succeed. This test fails most of the time because\nthe process that has acquired the \"tables.list\" lock is unable to rename\nit into place while other processes are busy reading that file.\n\nWindows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag\nthat allows us to fix this usecase [1]. When set, it is possible to\nrename a file over a preexisting file even when the target file still\nhas handles open. Those handles must have been opened with the\n`FILE_SHARE_DELETE` flag, which we have ensured in the preceding\ncommits.\n\nCareful readers might have noticed that [1] does not mention the above\nflag, but instead mentions `FILE_RENAME_POSIX_SEMANICS`. This flag is\nnot for use with `SetFileInformationByHandle()` though, which is what we\nuse. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\nnot documented on [2] or anywhere else as far as I can tell.\n\nUnfortuntaly, we still support Windows systems older than Windows 10\nthat do not yet have this new flag. Our `_WIN32_WINNT` SDK version still\ntargets 0x0600, which is Windows Vista and later. And even though that\nWindows version is out-of-support, bumping the SDK version all the way\nto 0x0A00, which is Windows 10 and later, is not an option as it would\nmake it impossible to compile on Windows 8.1, which is still supported.\nInstead, we have to manually declare the relevant infrastructure to make\nthis feature available and have fallback logic in place in case we run\non a Windows version that does not yet have this flag.\n\nOn another note: `mingw_rename()` has a retry loop that is used in case\ndeleting a file failed because it's still open in another process. One\nmight be pressed to not use this loop anymore when we can use POSIX\nsemantics. But unfortuntaley, we have to keep it around due to our\ndependence on the `FILE_SHARE_DELETE` flag. While we know to set that\nsharing flag now, other applications may not do so and may thus still\ncause sharing violations when we try to rename a file.\n\nThis fixes concurrent writes in the reftable backend as demonstrated in\nt0610, but may also end up fixing other usecases where Git wants to\nperform renames.\n\n[1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information\n[2]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info\n\nSigned-off-by: Patrick Steinhardt \n---\n compat/mingw.c | 87 ++++++++++++++++++++++++++++++++++++--\n t/t0610-reftable-basics.sh | 8 ++--\n 2 files changed, 88 insertions(+), 7 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex ce95f3a5968..df78f61f7f9 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2206,10 +2206,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)\n #undef rename\n int mingw_rename(const char *pold, const char *pnew)\n {\n+\tstatic int supports_file_rename_info_ex = 1;\n \tDWORD attrs, gle;\n \tint tries = 0;\n \twchar_t wpold[MAX_PATH], wpnew[MAX_PATH];\n-\tif (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)\n+\tint wpnew_len;\n+\n+\tif (xutftowcs_path(wpold, pold) < 0)\n+\t\treturn -1;\n+\twpnew_len = xutftowcs_path(wpnew, pnew);\n+\tif (wpnew_len < 0)\n \t\treturn -1;\n \n \t/*\n@@ -2220,11 +2226,84 @@ int mingw_rename(const char *pold, const char *pnew)\n \t\treturn 0;\n \tif (errno != EEXIST)\n \t\treturn -1;\n+\n repeat:\n-\tif (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))\n-\t\treturn 0;\n+\tif (supports_file_rename_info_ex) {\n+\t\t/*\n+\t\t * Our minimum required Windows version is still set to Windows\n+\t\t * Vista. We thus have to declare required infrastructure for\n+\t\t * FileRenameInfoEx ourselves until we bump _WIN32_WINNT to\n+\t\t * 0x0A00. Furthermore, we have to handle cases where the\n+\t\t * FileRenameInfoEx call isn't supported yet.\n+\t\t */\n+#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS 0x00000001\n+#define FILE_RENAME_FLAG_POSIX_SEMANTICS 0x00000002\n+\t\tFILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22;\n+\t\tstruct {\n+\t\t\t/*\n+\t\t\t * This is usually an unnamed union, but that is not\n+\t\t\t * part of ISO C99. We thus inline the field, as we\n+\t\t\t * really only care for the Flags field anyway.\n+\t\t\t */\n+\t\t\tDWORD Flags;\n+\t\t\tHANDLE RootDirectory;\n+\t\t\tDWORD FileNameLength;\n+\t\t\t/*\n+\t\t\t * The actual structure is defined with a single-character\n+\t\t\t * flex array so that the structure has to be allocated on\n+\t\t\t * the heap. As we declare this structure ourselves though\n+\t\t\t * we can avoid the allocation and define FileName to have\n+\t\t\t * MAX_PATH bytes.\n+\t\t\t */\n+\t\t\tWCHAR FileName[MAX_PATH];\n+\t\t} rename_info = { 0 };\n+\t\tHANDLE old_handle = INVALID_HANDLE_VALUE;\n+\t\tBOOL success;\n+\n+\t\told_handle = CreateFileW(wpold, DELETE,\n+\t\t\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n+\t\t\t\t\t NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n+\t\tif (old_handle == INVALID_HANDLE_VALUE) {\n+\t\t\terrno = err_win_to_posix(GetLastError());\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\trename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |\n+\t\t\t\t FILE_RENAME_FLAG_POSIX_SEMANTICS;\n+\t\trename_info.FileNameLength = wpnew_len * sizeof(WCHAR);\n+\t\tmemcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));\n+\n+\t\tsuccess = SetFileInformationByHandle(old_handle, FileRenameInfoEx,\n+\t\t\t\t\t\t &rename_info, sizeof(rename_info));\n+\t\tgle = GetLastError();\n+\t\tCloseHandle(old_handle);\n+\t\tif (success)\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * When we see ERROR_INVALID_PARAMETER we can assume that the\n+\t\t * current system doesn't support FileRenameInfoEx. Keep us\n+\t\t * from using it in future calls and retry.\n+\t\t */\n+\t\tif (gle == ERROR_INVALID_PARAMETER) {\n+\t\t\tsupports_file_rename_info_ex = 0;\n+\t\t\tgoto repeat;\n+\t\t}\n+\n+\t\t/*\n+\t\t * In theory, we shouldn't get ERROR_ACCESS_DENIED because we\n+\t\t * always open files with FILE_SHARE_DELETE But in practice we\n+\t\t * cannot assume that Git is the only one accessing files, and\n+\t\t * other applications may not set FILE_SHARE_DELETE. So we have\n+\t\t * to retry.\n+\t\t */\n+\t} else {\n+\t\tif (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))\n+\t\t\treturn 0;\n+\t\tgle = GetLastError();\n+\t}\n+\n \t/* TODO: translate more errors */\n-\tgle = GetLastError();\n \tif (gle == ERROR_ACCESS_DENIED &&\n \t (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {\n \t\tif (attrs & FILE_ATTRIBUTE_DIRECTORY) {\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex babec7993e3..eaf6fab6d29 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n \t)\n '\n \n-# This test fails most of the time on Windows systems. The root cause is\n+# This test fails most of the time on Cygwin systems. The root cause is\n # that Windows does not allow us to rename the \"tables.list.lock\" file into\n-# place when \"tables.list\" is open for reading by a concurrent process.\n-test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n+# place when \"tables.list\" is open for reading by a concurrent process. We have\n+# worked around that in our MinGW-based rename emulation, but the Cygwin\n+# emulation seems to be insufficient.\n+test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"505934","messageId":"ZxkX5XgghHp74NAT@nand.local","threadId":"62397","inReplyTo":"cover.1729695349.git.ps@pks.im","subject":"Re: [PATCH 0/3] compat/mingw: implement POSIX-style atomic renames","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-23T15:36:05Z","receivedAt":"2024-10-23T15:36:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 23, 2024 at 05:04:55PM +0200, Patrick Steinhardt wrote:\n> With these changes t0610 now passes on my Windows 10 machine, but this\n> may also have a positive impact on other parts of Git where we might\n> have previously failed.\n\nOoo. Thank you both for collaborating and working on this.\n\nThanks,\nTaylor\n"},{"id":"505945","messageId":"b106fb4d-3488-4fef-aca8-32e49efd0124@app.fastmail.com","threadId":"62397","inReplyTo":"907576d23d1dc39b679a323e74b6bfb227d6c17d.1729695349.git.ps@pks.im","subject":"Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-23T16:18:58Z","receivedAt":"2024-10-23T16:19:20Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Oct 23, 2024, at 17:04, Patrick Steinhardt wrote:\n> Unless told otherwise, Windows will keep other processes from reading,\n> writing and deleting files when one has an open handle that was created\n> via `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\n> flags such that other processes _can_ read and/or modify such a file.\n> This sharing mechanism is quite important in the context of Git, as we\n> assume POSIX semantics all over the place.\n>\n> There are two calls where we don't set up those flags though:\n>\n> - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n> via `mingw_open_append()`. This makes it impossible to delete the\n> file from another process or to replace it via an atomic rename.\n>\n> - When opening a file such that we can change its access/modification\n> times. This makes it impossible to perform any kind of operation\n> on this file at all from another process. While we only open the\n> file for a short amount of time to update its timestamps, this still\n> opens us up for a race condition with another process.\n>\n> Adapt both of these callsites to pass all three sharing flags.\n>\n> Signed-off-by: Patrick Steinhardt \n\n(Reading back) By default Windows restricts other processes (so these\nfiles could be in use by other procesess) from reading, writing, and\ndeleting them (presumably doing anything it looks like). But it does\nprovide flags if you need these permissions.\n\nThere are two calls/places where you need to expand the permissions:\n\n1. “Delete” for appending: need for deletion or replace via\n atomic rename\n2. When opening a file to modify the access/modification metadata. The\n current permissions are *likely* to work but you could run into race\n conditions, so the current set of permissions are buggy.\n\nThe commit seems well-explained to this naïve reader.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"505946","messageId":"bc829d84-6696-4e16-9687-8ba611af29ff@app.fastmail.com","threadId":"62397","inReplyTo":"3552feddb332b31744c2ab723b112b9b08926436.1729695349.git.ps@pks.im","subject":"Re: [PATCH 2/3] compat/mingw: allow deletion of most opened files","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-23T16:17:15Z","receivedAt":"2024-10-23T16:19:55Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:\n> On Windows, we emulate open(3p) via `mingw_open()`. This function\n> implements handling of some platform- specific quirks that are required\n\ns/platform- specific/platform-specific/\n\nLinewrapping artifact?\n\n> to make it behave as closely as possible like open(3p) would, but for\n> most cases we just call the Windows-specific `_wopen()` function.\n>\n> This function has a major downside though: it does not allow us to\n> specify the sharing mode. While there is `_wsopen()` that allows us to\n> pass sharing flags, those sharing flags are not the same `FILE_SHARE_*`\n> flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows\n> concurrent read- and write-access, but does not allow for concurrent\n> deletions. Unfortunately though, we have to allow concurrent deletions\n> if we want to have POSIX-style atomic renames on top of an existing file\n> that has open file handles.\n>\n> Implement a new function that emulates open(3p) for existing files via\n> `CreateFileW()` such that we can set the required sharing flags.\n>\n> While we have the same issue when calling open(3p) with `O_CREAT`,\n\ns/O_CREAT/O_CREATE/ ?\n\n> implementing that mode would be more complex due to the required\n> permission handling. Furthermore, atomic updates via renames typically\n> write to exclusive lockfile and then perform the rename, and thus we\n> don't have to handle the case where the locked path has been created\n> with `O_CREATE`. So while it would be nice to have proper POSIX\n> semantics in all paths, we instead aim for a minimum viable fix here.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> compat/mingw.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 55 insertions(+)\n>\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index e326c6fcd2d..ce95f3a5968 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -532,6 +532,59 @@ static int mingw_open_append(wchar_t const\n> *wfilename, int oflags, ...)\n> \treturn fd;\n> }\n>\n> +/*\n> + * Ideally, we'd use `_wopen()` to implement this functionality so that we\n> + * don't have to reimplement it, but unfortunately we do not have tight control\n> + * over the share mode there. And while `_wsopen()` and friends exist that give\n> + * us _some_ control over the share mode, this family of functions doesn't give\n> + * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict\n> + * requirement for us though so that we can unlink or rename over files that\n> + * are held open by another process.\n> + *\n> + * We are thus forced to implement our own emulation of `open()`. To make our\n> + * life simpler we only implement basic support for this, namely opening\n> + * existing files for reading and/or writing. This means that newly created\n> + * files won't have their sharing mode set up correctly, but for now I couldn't\n> + * find any case where this matters. We may have to revisit that in the future\n> + * though based on our needs.\n> + */\n\nThis is above my technical level but the comment reads nicely.\n\n> +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)\n> +{\n> +\tSECURITY_ATTRIBUTES security_attributes = {\n> +\t .nLength = sizeof(security_attributes),\n> +\t .bInheritHandle = !(oflags & O_NOINHERIT),\n\n`clang-format` thinks that these two lines should be indented with tabs\ninstead (so two tabs in total).\n\n(Ubuntu clang-format version 14.0.0-1ubuntu1.1)\n\n> +\t};\n> +\tHANDLE handle;\n> +\tint access;\n> +\tint fd;\n> +\n> +\t/* We only support basic flags. */\n> +\tif (oflags & ~(O_ACCMODE | O_NOINHERIT))\n> +\t\treturn errno = ENOSYS, -1;\n\nThis use of the comma operator is maybe an idiom to save space and avoid\na brace around the `if`? This pattern is already in use in\n`mingw_open_append`. I see in `mingw.h` that it uses:\n\n```\nstatic inline int symlink(const char *oldpath UNUSED, const char *newpath UNUSED)\n{ errno = ENOSYS; return -1; }\n```\n\n> +\tif (oflags & O_RDWR)\n> +\t\taccess = GENERIC_READ | GENERIC_WRITE;\n> +\telse if (oflags & O_WRONLY)\n> +\t\taccess = GENERIC_WRITE;\n> +\telse\n> +\t\taccess = GENERIC_READ;\n> +\n> +\thandle = CreateFileW(filename, access,\n> +\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n> +\t\t\t &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n\n`clang-format` says that these two lines are too long.\n\n> +\tif (handle == INVALID_HANDLE_VALUE) {\n> +\t\tDWORD err = GetLastError();\n> +\t\tif (err == ERROR_INVALID_PARAMETER)\n> +\t\t\terr = ERROR_PATH_NOT_FOUND;\n> +\t\terrno = err_win_to_posix(err);\n> +\t\treturn -1;\n\nHere you don’t use the comma operator, presumably because it wouldn’t\nturn the `if` into a one-statement block.\n\n> +\t}\n> +\n> +\tfd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);\n> +\tif (fd < 0)\n> +\t\tCloseHandle(handle);\n> +\treturn fd;\n> +}\n> +\n> /*\n> * Does the pathname map to the local named pipe filesystem?\n> * That is, does it have a \"//./pipe/\" prefix?\n> @@ -567,6 +620,8 @@ int mingw_open (const char *filename, int oflags, ...)\n>\n> \tif ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))\n> \t\topen_fn = mingw_open_append;\n> +\telse if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))\n> +\t\topen_fn = mingw_open_existing;\n> \telse\n> \t\topen_fn = _wopen;\n>\n> --\n> 2.47.0.118.gfd3785337b.dirty\n"},{"id":"505947","messageId":"51b63d57-fa01-4a8c-95fa-770a62afd80f@app.fastmail.com","threadId":"62397","inReplyTo":"d17ca1c7ce38e12fe113f15b078c12bc92e8f0aa.1729695349.git.ps@pks.im","subject":"Re: [PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-23T16:19:43Z","receivedAt":"2024-10-23T16:20:06Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:\n> […]\n> Careful readers might have noticed that [1] does not mention the above\n> flag, but instead mentions `FILE_RENAME_POSIX_SEMANICS`. This flag is\n\ns/FILE_RENAME_POSIX_SEMANICS/FILE_RENAME_FLAG_POSIX_SEMANTICS/\n\n> not for use with `SetFileInformationByHandle()` though, which is what we\n> use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\n> not documented on [2] or anywhere else as far as I can tell.\n>\n> Unfortuntaly, we still support Windows systems older than Windows 10\n\ns/Unfortuntaly/Unfortunately/\n\n> […]\n>\n> On another note: `mingw_rename()` has a retry loop that is used in case\n> deleting a file failed because it's still open in another process. One\n> might be pressed to not use this loop anymore when we can use POSIX\n> semantics. But unfortuntaley, we have to keep it around due to our\n\ns/unfortuntaley/unfortunately/\n\n> [1]:\n> https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information\n> [2]:\n> https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> compat/mingw.c | 87 ++++++++++++++++++++++++++++++++++++--\n> t/t0610-reftable-basics.sh | 8 ++--\n> 2 files changed, 88 insertions(+), 7 deletions(-)\n> […]\n> +\n> \t/* TODO: translate more errors */\n\nIt seems that `Documentation` doesn’t mention the difference between\n`TODO` and `NEEDSWORK`. What is it?\n\n> […]\n> --\n> 2.47.0.118.gfd3785337b.dirty\n"},{"id":"505954","messageId":"ZxkxAvLeMtxstVdS@nand.local","threadId":"62397","inReplyTo":"907576d23d1dc39b679a323e74b6bfb227d6c17d.1729695349.git.ps@pks.im","subject":"Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-23T17:23:14Z","receivedAt":"2024-10-23T17:23:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 23, 2024 at 05:04:58PM +0200, Patrick Steinhardt wrote:\n> Unless told otherwise, Windows will keep other processes from reading,\n> writing and deleting files when one has an open handle that was created\n> via `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\n> flags such that other processes _can_ read and/or modify such a file.\n> This sharing mechanism is quite important in the context of Git, as we\n> assume POSIX semantics all over the place.\n>\n> There are two calls where we don't set up those flags though:\n>\n> - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n> via `mingw_open_append()`. This makes it impossible to delete the\n> file from another process or to replace it via an atomic rename.\n>\n> - When opening a file such that we can change its access/modification\n> times. This makes it impossible to perform any kind of operation\n> on this file at all from another process. While we only open the\n> file for a short amount of time to update its timestamps, this still\n> opens us up for a race condition with another process.\n>\n> Adapt both of these callsites to pass all three sharing flags.\n\nInteresting, and especially so noting that we *do* call CreateFileW()\nwith the FILE_SHARE_DELETE flag in other functions like create_watch(),\nmingw_open_existing(), mingw_getcwd(), etc.\n\nWhy didn't we call CreateFileW() with FILE_SHARE_DELETE in the below two\nfunctions? I assume Johannes Schindelin and/or Johannes Sixt (both CC'd)\nwould know the answer. Regardless, it would be interesting and useful\n(IMHO) to include such a detail in the commit message.\n\nThanks,\nTaylor\n"},{"id":"505955","messageId":"ZxkxbAjPqr+t+ssP@nand.local","threadId":"62397","inReplyTo":"ZxkxAvLeMtxstVdS@nand.local","subject":"Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-23T17:25:00Z","receivedAt":"2024-10-23T17:25:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 23, 2024 at 01:23:14PM -0400, Taylor Blau wrote:\n> On Wed, Oct 23, 2024 at 05:04:58PM +0200, Patrick Steinhardt wrote:\n> > Unless told otherwise, Windows will keep other processes from reading,\n> > writing and deleting files when one has an open handle that was created\n> > via `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\n> > flags such that other processes _can_ read and/or modify such a file.\n> > This sharing mechanism is quite important in the context of Git, as we\n> > assume POSIX semantics all over the place.\n> >\n> > There are two calls where we don't set up those flags though:\n> >\n> > - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n> > via `mingw_open_append()`. This makes it impossible to delete the\n> > file from another process or to replace it via an atomic rename.\n> >\n> > - When opening a file such that we can change its access/modification\n> > times. This makes it impossible to perform any kind of operation\n> > on this file at all from another process. While we only open the\n> > file for a short amount of time to update its timestamps, this still\n> > opens us up for a race condition with another process.\n> >\n> > Adapt both of these callsites to pass all three sharing flags.\n>\n> Interesting, and especially so noting that we *do* call CreateFileW()\n> with the FILE_SHARE_DELETE flag in other functions like create_watch(),\n> mingw_open_existing(), mingw_getcwd(), etc.\n\nHeh. mingw_open_existing() passes FILE_SHARE_DELETE because you added\nit. Can you tell which branch I'm 'git grep'-ing on? ;-)\n\nThanks,\nTaylor\n"},{"id":"505956","messageId":"Zxkxij1FThn7QoOg@nand.local","threadId":"62397","inReplyTo":"b106fb4d-3488-4fef-aca8-32e49efd0124@app.fastmail.com","subject":"Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-23T17:25:30Z","receivedAt":"2024-10-23T17:25:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 23, 2024 at 06:18:58PM +0200, Kristoffer Haugsbakk wrote:\n> On Wed, Oct 23, 2024, at 17:04, Patrick Steinhardt wrote:\n> > Unless told otherwise, Windows will keep other processes from reading,\n> > writing and deleting files when one has an open handle that was created\n> > via `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\n> > flags such that other processes _can_ read and/or modify such a file.\n> > This sharing mechanism is quite important in the context of Git, as we\n> > assume POSIX semantics all over the place.\n> >\n> > There are two calls where we don't set up those flags though:\n> >\n> > - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n> > via `mingw_open_append()`. This makes it impossible to delete the\n> > file from another process or to replace it via an atomic rename.\n> >\n> > - When opening a file such that we can change its access/modification\n> > times. This makes it impossible to perform any kind of operation\n> > on this file at all from another process. While we only open the\n> > file for a short amount of time to update its timestamps, this still\n> > opens us up for a race condition with another process.\n> >\n> > Adapt both of these callsites to pass all three sharing flags.\n> >\n> > Signed-off-by: Patrick Steinhardt \n>\n> (Reading back) By default Windows restricts other processes (so these\n> files could be in use by other procesess) from reading, writing, and\n> deleting them (presumably doing anything it looks like). But it does\n> provide flags if you need these permissions.\n>\n> There are two calls/places where you need to expand the permissions:\n>\n> 1. “Delete” for appending: need for deletion or replace via\n> atomic rename\n> 2. When opening a file to modify the access/modification metadata. The\n> current permissions are *likely* to work but you could run into race\n> conditions, so the current set of permissions are buggy.\n>\n> The commit seems well-explained to this naïve reader.\n\nThanks for thinking aloud and summarizing your thoughts here. I found\nthe read-back to be quite useful indeed.\n\nThanks,\nTaylor\n"},{"id":"505958","messageId":"ZxkysCb/d3+PJjW4@nand.local","threadId":"62397","inReplyTo":"bc829d84-6696-4e16-9687-8ba611af29ff@app.fastmail.com","subject":"Re: [PATCH 2/3] compat/mingw: allow deletion of most opened files","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-23T17:30:24Z","receivedAt":"2024-10-23T17:30:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 23, 2024 at 06:17:15PM +0200, Kristoffer Haugsbakk wrote:\n> On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:\n> > On Windows, we emulate open(3p) via `mingw_open()`. This function\n> > implements handling of some platform- specific quirks that are required\n>\n> s/platform- specific/platform-specific/\n>\n> Linewrapping artifact?\n\nLooks like it. Thanks for spotting.\n\n> > to make it behave as closely as possible like open(3p) would, but for\n> > most cases we just call the Windows-specific `_wopen()` function.\n> >\n> > This function has a major downside though: it does not allow us to\n> > specify the sharing mode. While there is `_wsopen()` that allows us to\n> > pass sharing flags, those sharing flags are not the same `FILE_SHARE_*`\n> > flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows\n> > concurrent read- and write-access, but does not allow for concurrent\n> > deletions. Unfortunately though, we have to allow concurrent deletions\n> > if we want to have POSIX-style atomic renames on top of an existing file\n> > that has open file handles.\n> >\n> > Implement a new function that emulates open(3p) for existing files via\n> > `CreateFileW()` such that we can set the required sharing flags.\n> >\n> > While we have the same issue when calling open(3p) with `O_CREAT`,\n>\n> s/O_CREAT/O_CREATE/ ?\n\nNo, O_CREAT is the flag name.\n\n> > +\t};\n> > +\tHANDLE handle;\n> > +\tint access;\n> > +\tint fd;\n> > +\n> > +\t/* We only support basic flags. */\n> > +\tif (oflags & ~(O_ACCMODE | O_NOINHERIT))\n> > +\t\treturn errno = ENOSYS, -1;\n>\n> This use of the comma operator is maybe an idiom to save space and avoid\n> a brace around the `if`? This pattern is already in use in\n> `mingw_open_append`. I see in `mingw.h` that it uses:\n>\n> ```\n> static inline int symlink(const char *oldpath UNUSED, const char *newpath UNUSED)\n> { errno = ENOSYS; return -1; }\n> ```\n>\n\nYeah. What Patrick wrote here is not technically wrong, but I would not\nconsider it in the style of the rest of the project. Perhaps\ncompat/mingw-stuff is a bit of a wild west, but I think it would be\nmatch the rest of the project's conventions here.\n\nI actually think from grepping around that mingw_open_append is the only\nother function in the tree that uses the \"return errno = XXX, -1;\"\ntrick. It might be nice to keep it that way, and/or rewrite that portion\nlike so:\n\n--- 8< ---\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex df78f61f7f..c36147549a 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -494,8 +494,10 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n \tDWORD create = (oflags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING;\n\n \t/* only these flags are supported */\n-\tif ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))\n-\t\treturn errno = ENOSYS, -1;\n+\tif ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {\n+\t\terrno = ENOSYS;\n+\t\treturn -1;\n+\t}\n\n \t/*\n \t * FILE_SHARE_WRITE is required to permit child processes\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"505960","messageId":"Zxk7ffCFJI2mGMhJ@nand.local","threadId":"62397","inReplyTo":"3552feddb332b31744c2ab723b112b9b08926436.1729695349.git.ps@pks.im","subject":"Re: [PATCH 2/3] compat/mingw: allow deletion of most opened files","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-23T18:07:57Z","receivedAt":"2024-10-23T18:08:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 23, 2024 at 05:05:00PM +0200, Patrick Steinhardt wrote:\n> compat/mingw.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 55 insertions(+)\n\nVery well reasoned and explained. I am certainly not a Windows expert,\nbut from what I read the changes look reasonable to me.\n\nThanks,\nTaylor\n"},{"id":"505961","messageId":"ZxlAriZuYZNrOWJF@nand.local","threadId":"62397","inReplyTo":"d17ca1c7ce38e12fe113f15b078c12bc92e8f0aa.1729695349.git.ps@pks.im","subject":"Re: [PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-23T18:30:06Z","receivedAt":"2024-10-23T18:30:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 23, 2024 at 05:05:03PM +0200, Patrick Steinhardt wrote:\n> ---\n> compat/mingw.c | 87 ++++++++++++++++++++++++++++++++++++--\n> t/t0610-reftable-basics.sh | 8 ++--\n> 2 files changed, 88 insertions(+), 7 deletions(-)\n\nAll well reasoned and explained as usual. I'm glossing over the\nWindows-specific parts, though they look correct to me if I squint and\npretend that I have even a passing familiarity with the platform ;-).\n\nThis looks good, and I am glad to hear that it was tested on Windows 8.1\nby Johannes, and on Windows 10 by you. Do we have any reason to believe\nit would break on Vista? If so, should we test there as well?\n\nOtherwise, this is looking good, modulo the handful of typos that was\nnoticed earlier in the thread on this patch. Thanks to you and Johannes\nfor working on this together!\n\nThanks,\nTaylor\n"},{"id":"506000","messageId":"ZxnpitkbPPNTLuGT@pks.im","threadId":"62397","inReplyTo":"51b63d57-fa01-4a8c-95fa-770a62afd80f@app.fastmail.com","subject":"Re: [PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-24T06:30:41Z","receivedAt":"2024-10-24T06:30:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 23, 2024 at 06:19:43PM +0200, Kristoffer Haugsbakk wrote:\n> On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:\n> > […]\n> > Careful readers might have noticed that [1] does not mention the above\n> > flag, but instead mentions `FILE_RENAME_POSIX_SEMANICS`. This flag is\n> \n> s/FILE_RENAME_POSIX_SEMANICS/FILE_RENAME_FLAG_POSIX_SEMANTICS/\n\nNo, this is actually intentional. There are two different flags:\n\n - FILE_RENAME_POSIX_SEMANTICS is what ntifs.h exposes and what I've\n been linking for documentation. This flag cannot be used though with\n `FileRenameInfoEx`.\n\n - FILE_RENAME_FLAG_POSIX_SEMANTICS is what needs to be used with\n `FileRenameInfoEx`, but it is undocumented.\n\n> > \t/* TODO: translate more errors */\n> \n> It seems that `Documentation` doesn’t mention the difference between\n> `TODO` and `NEEDSWORK`. What is it?\n\nNo idea, to be honest.\n\nPatrick\n"},{"id":"506001","messageId":"ZxnpmPbKDNiQl66p@pks.im","threadId":"62397","inReplyTo":"ZxkxAvLeMtxstVdS@nand.local","subject":"Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-24T06:30:48Z","receivedAt":"2024-10-24T06:30:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 23, 2024 at 01:23:14PM -0400, Taylor Blau wrote:\n> On Wed, Oct 23, 2024 at 05:04:58PM +0200, Patrick Steinhardt wrote:\n> > Unless told otherwise, Windows will keep other processes from reading,\n> > writing and deleting files when one has an open handle that was created\n> > via `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\n> > flags such that other processes _can_ read and/or modify such a file.\n> > This sharing mechanism is quite important in the context of Git, as we\n> > assume POSIX semantics all over the place.\n> >\n> > There are two calls where we don't set up those flags though:\n> >\n> > - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n> > via `mingw_open_append()`. This makes it impossible to delete the\n> > file from another process or to replace it via an atomic rename.\n> >\n> > - When opening a file such that we can change its access/modification\n> > times. This makes it impossible to perform any kind of operation\n> > on this file at all from another process. While we only open the\n> > file for a short amount of time to update its timestamps, this still\n> > opens us up for a race condition with another process.\n> >\n> > Adapt both of these callsites to pass all three sharing flags.\n> \n> Interesting, and especially so noting that we *do* call CreateFileW()\n> with the FILE_SHARE_DELETE flag in other functions like create_watch(),\n> mingw_open_existing(), mingw_getcwd(), etc.\n> \n> Why didn't we call CreateFileW() with FILE_SHARE_DELETE in the below two\n> functions? I assume Johannes Schindelin and/or Johannes Sixt (both CC'd)\n> would know the answer. Regardless, it would be interesting and useful\n> (IMHO) to include such a detail in the commit message.\n\nHard to tell, but I think it's basically an oversight.\n\n - `mingw_utime()` was originally implemented via `_wopen()`, which\n doesn't give you full control over the sharing mode. It was then\n refactored via 090a3085bc (t/helper/test-chmtime: update mingw to\n support chmtime on directories, 2022-03-02) to use `CreateFileW()`.\n This refactoring wasn't quite to the old code, because we use no\n sharing flags at all. But in fact, `_wopen()` calls `_wsopen()` with\n `_SH_DENYNO`, which ultimately translates to `FILE_SHARE_READ |\n `FILE_SHARE_WRITE`. So we lost the read/write sharing with that\n conversion.\n\n - `mingw_open_append()` was introduced via d641097589 (mingw: enable\n atomic O_APPEND, 2018-08-13) and had `FILE_SHARE_READ |\n FILE_SHARE_WRITE` since the beginning.\n\nWhy we didn't set `FILE_SHARE_DELETE` is a different question though,\nand none that I can answer based on the commit messages. I would claim\nthat it's likely an oversight and wasn't done intentionally, but I\ncannot say for sure.\n\nAnyway, I'll include the above information in the commit message.\n\nPatrick\n"},{"id":"506002","messageId":"ZxnpnLbETN2-DZsq@pks.im","threadId":"62397","inReplyTo":"bc829d84-6696-4e16-9687-8ba611af29ff@app.fastmail.com","subject":"Re: [PATCH 2/3] compat/mingw: allow deletion of most opened files","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-24T06:30:52Z","receivedAt":"2024-10-24T06:30:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 23, 2024 at 06:17:15PM +0200, Kristoffer Haugsbakk wrote:\n> On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:\n> > While we have the same issue when calling open(3p) with `O_CREAT`,\n> \n> s/O_CREAT/O_CREATE/ ?\n\nYeah, you'd think that, but no, it's really `O_CREAT`.\n\n> > +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)\n> > +{\n> > +\tSECURITY_ATTRIBUTES security_attributes = {\n> > +\t .nLength = sizeof(security_attributes),\n> > +\t .bInheritHandle = !(oflags & O_NOINHERIT),\n> \n> `clang-format` thinks that these two lines should be indented with tabs\n> instead (so two tabs in total).\n> \n> (Ubuntu clang-format version 14.0.0-1ubuntu1.1)\n\nAnd they indeed should be.\n\n> > +\t};\n> > +\tHANDLE handle;\n> > +\tint access;\n> > +\tint fd;\n> > +\n> > +\t/* We only support basic flags. */\n> > +\tif (oflags & ~(O_ACCMODE | O_NOINHERIT))\n> > +\t\treturn errno = ENOSYS, -1;\n> \n> This use of the comma operator is maybe an idiom to save space and avoid\n> a brace around the `if`? This pattern is already in use in\n> `mingw_open_append`. I see in `mingw.h` that it uses:\n> \n> ```\n> static inline int symlink(const char *oldpath UNUSED, const char *newpath UNUSED)\n> { errno = ENOSYS; return -1; }\n> ```\n\nI basically copied this from other code, but don't care strongly about\nit either way.\n\n> > +\tif (oflags & O_RDWR)\n> > +\t\taccess = GENERIC_READ | GENERIC_WRITE;\n> > +\telse if (oflags & O_WRONLY)\n> > +\t\taccess = GENERIC_WRITE;\n> > +\telse\n> > +\t\taccess = GENERIC_READ;\n> > +\n> > +\thandle = CreateFileW(filename, access,\n> > +\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n> > +\t\t\t &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n> \n> `clang-format` says that these two lines are too long.\n\nMight be. But the `FILE_SHARE_*` line cannot really be any shorter\nwithout hurting readability. And the second line just follows the first\nline then.\n\nPatrick\n"},{"id":"506003","messageId":"d6669573-d5cc-4ad9-b248-95bda089bc6c@app.fastmail.com","threadId":"62397","inReplyTo":"ZxnpitkbPPNTLuGT@pks.im","subject":"Re: [PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-24T07:18:18Z","receivedAt":"2024-10-24T07:18:40Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Thu, Oct 24, 2024, at 08:30, Patrick Steinhardt wrote:\n>> s/FILE_RENAME_POSIX_SEMANICS/FILE_RENAME_FLAG_POSIX_SEMANTICS/\n>\n> No, this is actually intentional. There are two different flags:\n>\n> - FILE_RENAME_POSIX_SEMANTICS is what ntifs.h exposes and what I've\n> been linking for documentation. This flag cannot be used though with\n> `FileRenameInfoEx`.\n>\n> - FILE_RENAME_FLAG_POSIX_SEMANTICS is what needs to be used with\n> `FileRenameInfoEx`, but it is undocumented.\n>\n\nI made a copy–paste mistake. :P I should have just pointed out this\n(missing T):\n\n _SEMANICS\n\nSo you wrote it the correct-looking way in your first bullet\npoint here.\n"},{"id":"506007","messageId":"Zxn1S5DMtN8XIrxL@pks.im","threadId":"62397","inReplyTo":"d6669573-d5cc-4ad9-b248-95bda089bc6c@app.fastmail.com","subject":"Re: [PATCH 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-24T07:20:43Z","receivedAt":"2024-10-24T07:20:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 24, 2024 at 09:18:18AM +0200, Kristoffer Haugsbakk wrote:\n> On Thu, Oct 24, 2024, at 08:30, Patrick Steinhardt wrote:\n> >> s/FILE_RENAME_POSIX_SEMANICS/FILE_RENAME_FLAG_POSIX_SEMANTICS/\n> >\n> > No, this is actually intentional. There are two different flags:\n> >\n> > - FILE_RENAME_POSIX_SEMANTICS is what ntifs.h exposes and what I've\n> > been linking for documentation. This flag cannot be used though with\n> > `FileRenameInfoEx`.\n> >\n> > - FILE_RENAME_FLAG_POSIX_SEMANTICS is what needs to be used with\n> > `FileRenameInfoEx`, but it is undocumented.\n> >\n> \n> I made a copy–paste mistake. :P I should have just pointed out this\n> (missing T):\n> \n> _SEMANICS\n> \n> So you wrote it the correct-looking way in your first bullet\n> point here.\n\nOh, indeed! Thanks.\n\nPatrick\n"},{"id":"506014","messageId":"cover.1729770140.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1729695349.git.ps@pks.im","subject":"[PATCH v2 0/3] compat/mingw: implement POSIX-style atomic renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-24T11:46:08Z","receivedAt":"2024-10-24T11:46:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second patch series that implements POSIX-style atomic\nrenames on Windows in order to fix concurrent writes with the reftable\nbackend.\n\nChanges compared to v1:\n\n - Added some historic digging to the first commit message.\n\n - Fix various spelling mistakes.\n\n - Fix indentation.\n\n - Don't use the comma operator to assign `errno`.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n compat/mingw: share file handles created via `CreateFileW()`\n compat/mingw: allow deletion of most opened files\n compat/mingw: support POSIX semantics for atomic renames\n\n compat/mingw.c | 149 +++++++++++++++++++++++++++++++++++--\n t/t0610-reftable-basics.sh | 8 +-\n 2 files changed, 148 insertions(+), 9 deletions(-)\n\nRange-diff against v1:\n1: 907576d23d1 ! 1: 63c2cfa87a4 compat/mingw: share file handles created via `CreateFileW()`\n @@ Commit message\n Unless told otherwise, Windows will keep other processes from reading,\n writing and deleting files when one has an open handle that was created\n via `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\n - flags such that other processes _can_ read and/or modify such a file.\n - This sharing mechanism is quite important in the context of Git, as we\n - assume POSIX semantics all over the place.\n + flags:\n +\n + - `FILE_SHARE_READ` allows a concurrent process to open the file for\n + reading.\n \n - There are two calls where we don't set up those flags though:\n + - `FILE_SHARE_WRITE` allows a concurrent process to open the file for\n + writing.\n +\n + - `FILE_SHARE_DELETE` allows a concurrent process to delete the file\n + or to replace it via an atomic rename.\n +\n + This sharing mechanism is quite important in the context of Git, as we\n + assume POSIX semantics all over the place. But there are two callsites\n + where we don't pass all three of these flags:\n \n - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n via `mingw_open_append()`. This makes it impossible to delete the\n - file from another process or to replace it via an atomic rename.\n + file from another process or to replace it via an atomic rename. The\n + function was introduced via d641097589 (mingw: enable atomic\n + O_APPEND, 2018-08-13) and has been using `FILE_SHARE_READ |\n + FILE_SHARE_WRITE` since the inception. There aren't any indicators\n + that the omission of `FILE_SHARE_DELETE` was intentional.\n +\n + - We don't set any sharing flags in `mingw_utime()`, which changes the\n + access and modification of a file. This makes it impossible to\n + perform any kind of operation on this file at all from another\n + process. While we only open the file for a short amount of time to\n + update its timestamps, this still opens us up for a race condition\n + with another process.\n +\n + `mingw_utime()` was originally implemented via `_wopen()`, which\n + doesn't give you full control over the sharing mode. Instead, it\n + calls `_wsopen()` with `_SH_DENYNO`, which ultimately translates to\n + `FILE_SHARE_READ | FILE_SHARE_WRITE`. It was then refactored via\n + 090a3085bc (t/helper/test-chmtime: update mingw to support chmtime\n + on directories, 2022-03-02) to use `CreateFileW()`, but we stopped\n + setting any sharing flags at all, which seems like an unintentional\n + side effect. By restoring `FILE_SHARE_READ | FILE_SHARE_WRITE` we\n + thus fix this and get back the old behaviour of `_wopen()`.\n \n - - When opening a file such that we can change its access/modification\n - times. This makes it impossible to perform any kind of operation\n - on this file at all from another process. While we only open the\n - file for a short amount of time to update its timestamps, this still\n - opens us up for a race condition with another process.\n + The fact that we didn't set the equivalent of `FILE_SHARE_DELETE`\n + can be explained, as well: neither `_wopen()` nor `_wsopen()` allow\n + you to do so. So overall, it doesn't seem intentional that we didn't\n + allow deletions here, either.\n \n Adapt both of these callsites to pass all three sharing flags.\n \n2: 3552feddb33 ! 2: c308eafbbb5 compat/mingw: allow deletion of most opened files\n @@ Commit message\n compat/mingw: allow deletion of most opened files\n \n On Windows, we emulate open(3p) via `mingw_open()`. This function\n - implements handling of some platform- specific quirks that are required\n + implements handling of some platform-specific quirks that are required\n to make it behave as closely as possible like open(3p) would, but for\n most cases we just call the Windows-specific `_wopen()` function.\n \n @@ compat/mingw.c: static int mingw_open_append(wchar_t const *wfilename, int oflag\n +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)\n +{\n +\tSECURITY_ATTRIBUTES security_attributes = {\n -+\t .nLength = sizeof(security_attributes),\n -+\t .bInheritHandle = !(oflags & O_NOINHERIT),\n ++\t\t.nLength = sizeof(security_attributes),\n ++\t\t.bInheritHandle = !(oflags & O_NOINHERIT),\n +\t};\n +\tHANDLE handle;\n +\tint access;\n +\tint fd;\n +\n +\t/* We only support basic flags. */\n -+\tif (oflags & ~(O_ACCMODE | O_NOINHERIT))\n -+\t\treturn errno = ENOSYS, -1;\n ++\tif (oflags & ~(O_ACCMODE | O_NOINHERIT)) {\n ++\t\terrno = ENOSYS;\n ++\t\treturn -1;\n ++\t}\n ++\n +\tif (oflags & O_RDWR)\n +\t\taccess = GENERIC_READ | GENERIC_WRITE;\n +\telse if (oflags & O_WRONLY)\n3: d17ca1c7ce3 ! 3: ee1e92e593e compat/mingw: support POSIX semantics for atomic renames\n @@ Commit message\n commits.\n \n Careful readers might have noticed that [1] does not mention the above\n - flag, but instead mentions `FILE_RENAME_POSIX_SEMANICS`. This flag is\n + flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\n not for use with `SetFileInformationByHandle()` though, which is what we\n use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\n not documented on [2] or anywhere else as far as I can tell.\n \n - Unfortuntaly, we still support Windows systems older than Windows 10\n + Unfortunately, we still support Windows systems older than Windows 10\n that do not yet have this new flag. Our `_WIN32_WINNT` SDK version still\n targets 0x0600, which is Windows Vista and later. And even though that\n Windows version is out-of-support, bumping the SDK version all the way\n @@ Commit message\n On another note: `mingw_rename()` has a retry loop that is used in case\n deleting a file failed because it's still open in another process. One\n might be pressed to not use this loop anymore when we can use POSIX\n - semantics. But unfortuntaley, we have to keep it around due to our\n + semantics. But unfortunately, we have to keep it around due to our\n dependence on the `FILE_SHARE_DELETE` flag. While we know to set that\n sharing flag now, other applications may not do so and may thus still\n cause sharing violations when we try to rename a file.\n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"506015","messageId":"63c2cfa87a4a3c44367acdc1f7cdb57289b4b761.1729770140.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1729770140.git.ps@pks.im","subject":"[PATCH v2 1/3] compat/mingw: share file handles created via `CreateFileW()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-24T11:46:20Z","receivedAt":"2024-10-24T11:46:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Unless told otherwise, Windows will keep other processes from reading,\nwriting and deleting files when one has an open handle that was created\nvia `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\nflags:\n\n - `FILE_SHARE_READ` allows a concurrent process to open the file for\n reading.\n\n - `FILE_SHARE_WRITE` allows a concurrent process to open the file for\n writing.\n\n - `FILE_SHARE_DELETE` allows a concurrent process to delete the file\n or to replace it via an atomic rename.\n\nThis sharing mechanism is quite important in the context of Git, as we\nassume POSIX semantics all over the place. But there are two callsites\nwhere we don't pass all three of these flags:\n\n - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n via `mingw_open_append()`. This makes it impossible to delete the\n file from another process or to replace it via an atomic rename. The\n function was introduced via d641097589 (mingw: enable atomic\n O_APPEND, 2018-08-13) and has been using `FILE_SHARE_READ |\n FILE_SHARE_WRITE` since the inception. There aren't any indicators\n that the omission of `FILE_SHARE_DELETE` was intentional.\n\n - We don't set any sharing flags in `mingw_utime()`, which changes the\n access and modification of a file. This makes it impossible to\n perform any kind of operation on this file at all from another\n process. While we only open the file for a short amount of time to\n update its timestamps, this still opens us up for a race condition\n with another process.\n\n `mingw_utime()` was originally implemented via `_wopen()`, which\n doesn't give you full control over the sharing mode. Instead, it\n calls `_wsopen()` with `_SH_DENYNO`, which ultimately translates to\n `FILE_SHARE_READ | FILE_SHARE_WRITE`. It was then refactored via\n 090a3085bc (t/helper/test-chmtime: update mingw to support chmtime\n on directories, 2022-03-02) to use `CreateFileW()`, but we stopped\n setting any sharing flags at all, which seems like an unintentional\n side effect. By restoring `FILE_SHARE_READ | FILE_SHARE_WRITE` we\n thus fix this and get back the old behaviour of `_wopen()`.\n\n The fact that we didn't set the equivalent of `FILE_SHARE_DELETE`\n can be explained, as well: neither `_wopen()` nor `_wsopen()` allow\n you to do so. So overall, it doesn't seem intentional that we didn't\n allow deletions here, either.\n\nAdapt both of these callsites to pass all three sharing flags.\n\nSigned-off-by: Patrick Steinhardt \n---\n compat/mingw.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 0e851ecae29..e326c6fcd2d 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -502,7 +502,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n \t * to append to the file.\n \t */\n \thandle = CreateFileW(wfilename, FILE_APPEND_DATA,\n-\t\t\tFILE_SHARE_WRITE | FILE_SHARE_READ,\n+\t\t\tFILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n \t\t\tNULL, create, FILE_ATTRIBUTE_NORMAL, NULL);\n \tif (handle == INVALID_HANDLE_VALUE) {\n \t\tDWORD err = GetLastError();\n@@ -1006,7 +1006,7 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \n \tosfilehandle = CreateFileW(wfilename,\n \t\t\t\t FILE_WRITE_ATTRIBUTES,\n-\t\t\t\t 0 /*FileShare.None*/,\n+\t\t\t\t FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,\n \t\t\t\t NULL,\n \t\t\t\t OPEN_EXISTING,\n \t\t\t\t (attrs != INVALID_FILE_ATTRIBUTES &&\n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"506016","messageId":"c308eafbbb5a7c853b8126750c4c44a8b8635192.1729770140.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1729770140.git.ps@pks.im","subject":"[PATCH v2 2/3] compat/mingw: allow deletion of most opened files","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-24T11:46:23Z","receivedAt":"2024-10-24T11:46:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Windows, we emulate open(3p) via `mingw_open()`. This function\nimplements handling of some platform-specific quirks that are required\nto make it behave as closely as possible like open(3p) would, but for\nmost cases we just call the Windows-specific `_wopen()` function.\n\nThis function has a major downside though: it does not allow us to\nspecify the sharing mode. While there is `_wsopen()` that allows us to\npass sharing flags, those sharing flags are not the same `FILE_SHARE_*`\nflags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows\nconcurrent read- and write-access, but does not allow for concurrent\ndeletions. Unfortunately though, we have to allow concurrent deletions\nif we want to have POSIX-style atomic renames on top of an existing file\nthat has open file handles.\n\nImplement a new function that emulates open(3p) for existing files via\n`CreateFileW()` such that we can set the required sharing flags.\n\nWhile we have the same issue when calling open(3p) with `O_CREAT`,\nimplementing that mode would be more complex due to the required\npermission handling. Furthermore, atomic updates via renames typically\nwrite to exclusive lockfile and then perform the rename, and thus we\ndon't have to handle the case where the locked path has been created\nwith `O_CREATE`. So while it would be nice to have proper POSIX\nsemantics in all paths, we instead aim for a minimum viable fix here.\n\nSigned-off-by: Patrick Steinhardt \n---\n compat/mingw.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 58 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex e326c6fcd2d..6c75fe36b15 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -532,6 +532,62 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n \treturn fd;\n }\n \n+/*\n+ * Ideally, we'd use `_wopen()` to implement this functionality so that we\n+ * don't have to reimplement it, but unfortunately we do not have tight control\n+ * over the share mode there. And while `_wsopen()` and friends exist that give\n+ * us _some_ control over the share mode, this family of functions doesn't give\n+ * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict\n+ * requirement for us though so that we can unlink or rename over files that\n+ * are held open by another process.\n+ *\n+ * We are thus forced to implement our own emulation of `open()`. To make our\n+ * life simpler we only implement basic support for this, namely opening\n+ * existing files for reading and/or writing. This means that newly created\n+ * files won't have their sharing mode set up correctly, but for now I couldn't\n+ * find any case where this matters. We may have to revisit that in the future\n+ * though based on our needs.\n+ */\n+static int mingw_open_existing(const wchar_t *filename, int oflags, ...)\n+{\n+\tSECURITY_ATTRIBUTES security_attributes = {\n+\t\t.nLength = sizeof(security_attributes),\n+\t\t.bInheritHandle = !(oflags & O_NOINHERIT),\n+\t};\n+\tHANDLE handle;\n+\tint access;\n+\tint fd;\n+\n+\t/* We only support basic flags. */\n+\tif (oflags & ~(O_ACCMODE | O_NOINHERIT)) {\n+\t\terrno = ENOSYS;\n+\t\treturn -1;\n+\t}\n+\n+\tif (oflags & O_RDWR)\n+\t\taccess = GENERIC_READ | GENERIC_WRITE;\n+\telse if (oflags & O_WRONLY)\n+\t\taccess = GENERIC_WRITE;\n+\telse\n+\t\taccess = GENERIC_READ;\n+\n+\thandle = CreateFileW(filename, access,\n+\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n+\t\t\t &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n+\tif (handle == INVALID_HANDLE_VALUE) {\n+\t\tDWORD err = GetLastError();\n+\t\tif (err == ERROR_INVALID_PARAMETER)\n+\t\t\terr = ERROR_PATH_NOT_FOUND;\n+\t\terrno = err_win_to_posix(err);\n+\t\treturn -1;\n+\t}\n+\n+\tfd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);\n+\tif (fd < 0)\n+\t\tCloseHandle(handle);\n+\treturn fd;\n+}\n+\n /*\n * Does the pathname map to the local named pipe filesystem?\n * That is, does it have a \"//./pipe/\" prefix?\n@@ -567,6 +623,8 @@ int mingw_open (const char *filename, int oflags, ...)\n \n \tif ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))\n \t\topen_fn = mingw_open_append;\n+\telse if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))\n+\t\topen_fn = mingw_open_existing;\n \telse\n \t\topen_fn = _wopen;\n \n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"506017","messageId":"ee1e92e593e35f3888e3e47b6f603c04a1652ec6.1729770140.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1729770140.git.ps@pks.im","subject":"[PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-24T11:46:26Z","receivedAt":"2024-10-24T11:46:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"By default, Windows restricts access to files when those files have been\nopened by another process. As explained in the preceding commits, these\nrestrictions can be loosened such that reads, writes and/or deletes of\nfiles with open handles _are_ allowed.\n\nWhile we set up those sharing flags in most relevant code paths now, we\nstill don't properly handle POSIX-style atomic renames in case the\ntarget path is open. This is failure demonstrated by t0610, where one of\nour tests spawns concurrent writes in a reftable-enabled repository and\nexpects all of them to succeed. This test fails most of the time because\nthe process that has acquired the \"tables.list\" lock is unable to rename\nit into place while other processes are busy reading that file.\n\nWindows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag\nthat allows us to fix this usecase [1]. When set, it is possible to\nrename a file over a preexisting file even when the target file still\nhas handles open. Those handles must have been opened with the\n`FILE_SHARE_DELETE` flag, which we have ensured in the preceding\ncommits.\n\nCareful readers might have noticed that [1] does not mention the above\nflag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\nnot for use with `SetFileInformationByHandle()` though, which is what we\nuse. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\nnot documented on [2] or anywhere else as far as I can tell.\n\nUnfortunately, we still support Windows systems older than Windows 10\nthat do not yet have this new flag. Our `_WIN32_WINNT` SDK version still\ntargets 0x0600, which is Windows Vista and later. And even though that\nWindows version is out-of-support, bumping the SDK version all the way\nto 0x0A00, which is Windows 10 and later, is not an option as it would\nmake it impossible to compile on Windows 8.1, which is still supported.\nInstead, we have to manually declare the relevant infrastructure to make\nthis feature available and have fallback logic in place in case we run\non a Windows version that does not yet have this flag.\n\nOn another note: `mingw_rename()` has a retry loop that is used in case\ndeleting a file failed because it's still open in another process. One\nmight be pressed to not use this loop anymore when we can use POSIX\nsemantics. But unfortunately, we have to keep it around due to our\ndependence on the `FILE_SHARE_DELETE` flag. While we know to set that\nsharing flag now, other applications may not do so and may thus still\ncause sharing violations when we try to rename a file.\n\nThis fixes concurrent writes in the reftable backend as demonstrated in\nt0610, but may also end up fixing other usecases where Git wants to\nperform renames.\n\n[1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information\n[2]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info\n\nSigned-off-by: Patrick Steinhardt \n---\n compat/mingw.c | 87 ++++++++++++++++++++++++++++++++++++--\n t/t0610-reftable-basics.sh | 8 ++--\n 2 files changed, 88 insertions(+), 7 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 6c75fe36b15..22c005a4533 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2209,10 +2209,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)\n #undef rename\n int mingw_rename(const char *pold, const char *pnew)\n {\n+\tstatic int supports_file_rename_info_ex = 1;\n \tDWORD attrs, gle;\n \tint tries = 0;\n \twchar_t wpold[MAX_PATH], wpnew[MAX_PATH];\n-\tif (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)\n+\tint wpnew_len;\n+\n+\tif (xutftowcs_path(wpold, pold) < 0)\n+\t\treturn -1;\n+\twpnew_len = xutftowcs_path(wpnew, pnew);\n+\tif (wpnew_len < 0)\n \t\treturn -1;\n \n \t/*\n@@ -2223,11 +2229,84 @@ int mingw_rename(const char *pold, const char *pnew)\n \t\treturn 0;\n \tif (errno != EEXIST)\n \t\treturn -1;\n+\n repeat:\n-\tif (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))\n-\t\treturn 0;\n+\tif (supports_file_rename_info_ex) {\n+\t\t/*\n+\t\t * Our minimum required Windows version is still set to Windows\n+\t\t * Vista. We thus have to declare required infrastructure for\n+\t\t * FileRenameInfoEx ourselves until we bump _WIN32_WINNT to\n+\t\t * 0x0A00. Furthermore, we have to handle cases where the\n+\t\t * FileRenameInfoEx call isn't supported yet.\n+\t\t */\n+#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS 0x00000001\n+#define FILE_RENAME_FLAG_POSIX_SEMANTICS 0x00000002\n+\t\tFILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22;\n+\t\tstruct {\n+\t\t\t/*\n+\t\t\t * This is usually an unnamed union, but that is not\n+\t\t\t * part of ISO C99. We thus inline the field, as we\n+\t\t\t * really only care for the Flags field anyway.\n+\t\t\t */\n+\t\t\tDWORD Flags;\n+\t\t\tHANDLE RootDirectory;\n+\t\t\tDWORD FileNameLength;\n+\t\t\t/*\n+\t\t\t * The actual structure is defined with a single-character\n+\t\t\t * flex array so that the structure has to be allocated on\n+\t\t\t * the heap. As we declare this structure ourselves though\n+\t\t\t * we can avoid the allocation and define FileName to have\n+\t\t\t * MAX_PATH bytes.\n+\t\t\t */\n+\t\t\tWCHAR FileName[MAX_PATH];\n+\t\t} rename_info = { 0 };\n+\t\tHANDLE old_handle = INVALID_HANDLE_VALUE;\n+\t\tBOOL success;\n+\n+\t\told_handle = CreateFileW(wpold, DELETE,\n+\t\t\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n+\t\t\t\t\t NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n+\t\tif (old_handle == INVALID_HANDLE_VALUE) {\n+\t\t\terrno = err_win_to_posix(GetLastError());\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\trename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |\n+\t\t\t\t FILE_RENAME_FLAG_POSIX_SEMANTICS;\n+\t\trename_info.FileNameLength = wpnew_len * sizeof(WCHAR);\n+\t\tmemcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));\n+\n+\t\tsuccess = SetFileInformationByHandle(old_handle, FileRenameInfoEx,\n+\t\t\t\t\t\t &rename_info, sizeof(rename_info));\n+\t\tgle = GetLastError();\n+\t\tCloseHandle(old_handle);\n+\t\tif (success)\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * When we see ERROR_INVALID_PARAMETER we can assume that the\n+\t\t * current system doesn't support FileRenameInfoEx. Keep us\n+\t\t * from using it in future calls and retry.\n+\t\t */\n+\t\tif (gle == ERROR_INVALID_PARAMETER) {\n+\t\t\tsupports_file_rename_info_ex = 0;\n+\t\t\tgoto repeat;\n+\t\t}\n+\n+\t\t/*\n+\t\t * In theory, we shouldn't get ERROR_ACCESS_DENIED because we\n+\t\t * always open files with FILE_SHARE_DELETE But in practice we\n+\t\t * cannot assume that Git is the only one accessing files, and\n+\t\t * other applications may not set FILE_SHARE_DELETE. So we have\n+\t\t * to retry.\n+\t\t */\n+\t} else {\n+\t\tif (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))\n+\t\t\treturn 0;\n+\t\tgle = GetLastError();\n+\t}\n+\n \t/* TODO: translate more errors */\n-\tgle = GetLastError();\n \tif (gle == ERROR_ACCESS_DENIED &&\n \t (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {\n \t\tif (attrs & FILE_ATTRIBUTE_DIRECTORY) {\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex babec7993e3..eaf6fab6d29 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n \t)\n '\n \n-# This test fails most of the time on Windows systems. The root cause is\n+# This test fails most of the time on Cygwin systems. The root cause is\n # that Windows does not allow us to rename the \"tables.list.lock\" file into\n-# place when \"tables.list\" is open for reading by a concurrent process.\n-test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n+# place when \"tables.list\" is open for reading by a concurrent process. We have\n+# worked around that in our MinGW-based rename emulation, but the Cygwin\n+# emulation seems to be insufficient.\n+test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"506049","messageId":"Zxp6Owj/y/dEuBYd@nand.local","threadId":"62397","inReplyTo":"cover.1729770140.git.ps@pks.im","subject":"Re: [PATCH v2 0/3] compat/mingw: implement POSIX-style atomic renames","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-24T16:47:55Z","receivedAt":"2024-10-24T16:47:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 24, 2024 at 01:46:08PM +0200, Patrick Steinhardt wrote:\n> Patrick Steinhardt (3):\n> compat/mingw: share file handles created via `CreateFileW()`\n> compat/mingw: allow deletion of most opened files\n> compat/mingw: support POSIX semantics for atomic renames\n\nThanks, will queue. Can other reviewers chime in if this looks ready to\nstart merging down?\n\nThanks,\nTaylor\n"},{"id":"506147","messageId":"a769d5a6-0d0f-4df4-b581-539d00aacbc9@kdbg.org","threadId":"62397","inReplyTo":"c308eafbbb5a7c853b8126750c4c44a8b8635192.1729770140.git.ps@pks.im","subject":"Re: [PATCH v2 2/3] compat/mingw: allow deletion of most opened files","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-10-27T13:17:36Z","receivedAt":"2024-10-27T13:17:51Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:\n> On Windows, we emulate open(3p) via `mingw_open()`. This function\n> implements handling of some platform-specific quirks that are required\n> to make it behave as closely as possible like open(3p) would, but for\n> most cases we just call the Windows-specific `_wopen()` function.\n> \n> This function has a major downside though: it does not allow us to\n> specify the sharing mode. While there is `_wsopen()` that allows us to\n> pass sharing flags, those sharing flags are not the same `FILE_SHARE_*`\n> flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows\n> concurrent read- and write-access, but does not allow for concurrent\n> deletions. Unfortunately though, we have to allow concurrent deletions\n> if we want to have POSIX-style atomic renames on top of an existing file\n> that has open file handles.\n> \n> Implement a new function that emulates open(3p) for existing files via\n> `CreateFileW()` such that we can set the required sharing flags.\n> \n> While we have the same issue when calling open(3p) with `O_CREAT`,\n> implementing that mode would be more complex due to the required\n> permission handling. Furthermore, atomic updates via renames typically\n> write to exclusive lockfile and then perform the rename, and thus we\n> don't have to handle the case where the locked path has been created\n> with `O_CREATE`. So while it would be nice to have proper POSIX\n> semantics in all paths, we instead aim for a minimum viable fix here.\n\nAgreed!\n\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> compat/mingw.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 58 insertions(+)\n> \n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index e326c6fcd2d..6c75fe36b15 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -532,6 +532,62 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n> \treturn fd;\n> }\n> \n> +/*\n> + * Ideally, we'd use `_wopen()` to implement this functionality so that we\n> + * don't have to reimplement it, but unfortunately we do not have tight control\n> + * over the share mode there. And while `_wsopen()` and friends exist that give\n> + * us _some_ control over the share mode, this family of functions doesn't give\n> + * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict\n> + * requirement for us though so that we can unlink or rename over files that\n> + * are held open by another process.\n> + *\n> + * We are thus forced to implement our own emulation of `open()`. To make our\n> + * life simpler we only implement basic support for this, namely opening\n> + * existing files for reading and/or writing. This means that newly created\n> + * files won't have their sharing mode set up correctly, but for now I couldn't\n> + * find any case where this matters. We may have to revisit that in the future\n> + * though based on our needs.\n> + */\n> +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)\n> +{\n> +\tSECURITY_ATTRIBUTES security_attributes = {\n> +\t\t.nLength = sizeof(security_attributes),\n> +\t\t.bInheritHandle = !(oflags & O_NOINHERIT),\n> +\t};\n> +\tHANDLE handle;\n> +\tint access;\n\nI would have made this variable DWORD or unsigned instead of plain int,\nbecause it receives a bit pattern and would match the parameter type of\nCreateFileW() better; but no big deal.\n\n> +\tint fd;\n> +\n> +\t/* We only support basic flags. */\n> +\tif (oflags & ~(O_ACCMODE | O_NOINHERIT)) {\n> +\t\terrno = ENOSYS;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tif (oflags & O_RDWR)\n> +\t\taccess = GENERIC_READ | GENERIC_WRITE;\n> +\telse if (oflags & O_WRONLY)\n> +\t\taccess = GENERIC_WRITE;\n> +\telse\n> +\t\taccess = GENERIC_READ;\n\nO_RDWR, O_WRONLY and O_RDONLY are not flags, but values occupying two\nbits of oflags. This must be:\n\n\tif ((oflags & O_ACCMODE) == O_RDWR)\n\t\taccess = GENERIC_READ | GENERIC_WRITE;\n\telse if ((oflags & O_ACCMODE) == O_WRONLY)\n\t\taccess = GENERIC_WRITE;\n\telse\n\t\taccess = GENERIC_READ;\n\nor similar.\n\n> +\n> +\thandle = CreateFileW(filename, access,\n> +\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n> +\t\t\t &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n> +\tif (handle == INVALID_HANDLE_VALUE) {\n> +\t\tDWORD err = GetLastError();\n> +\t\tif (err == ERROR_INVALID_PARAMETER)\n> +\t\t\terr = ERROR_PATH_NOT_FOUND;\n\nFirst I wondered what this is about, but then noticed that it is just\ncopied from the mingw_open_append() implementation catering to some\nbogus network filesystems. Good.\n\n> +\t\terrno = err_win_to_posix(err);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tfd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);\n> +\tif (fd < 0)\n> +\t\tCloseHandle(handle);\n> +\treturn fd;\n> +}\n> +\n> /*\n> * Does the pathname map to the local named pipe filesystem?\n> * That is, does it have a \"//./pipe/\" prefix?\n> @@ -567,6 +623,8 @@ int mingw_open (const char *filename, int oflags, ...)\n> \n> \tif ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))\n> \t\topen_fn = mingw_open_append;\n> +\telse if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))\n> +\t\topen_fn = mingw_open_existing;\n> \telse\n> \t\topen_fn = _wopen;\n> \n\nMakes sense!\n\n-- Hannes\n\n"},{"id":"506148","messageId":"542b306a-523d-424b-bc9f-c63bb7040beb@kdbg.org","threadId":"62397","inReplyTo":"ee1e92e593e35f3888e3e47b6f603c04a1652ec6.1729770140.git.ps@pks.im","subject":"Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-10-27T13:23:28Z","receivedAt":"2024-10-27T13:23:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:\n> By default, Windows restricts access to files when those files have been\n> opened by another process. As explained in the preceding commits, these\n> restrictions can be loosened such that reads, writes and/or deletes of\n> files with open handles _are_ allowed.\n> \n> While we set up those sharing flags in most relevant code paths now, we\n> still don't properly handle POSIX-style atomic renames in case the\n> target path is open. This is failure demonstrated by t0610, where one of\n> our tests spawns concurrent writes in a reftable-enabled repository and\n> expects all of them to succeed. This test fails most of the time because\n> the process that has acquired the \"tables.list\" lock is unable to rename\n> it into place while other processes are busy reading that file.\n> \n> Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag\n> that allows us to fix this usecase [1]. When set, it is possible to\n> rename a file over a preexisting file even when the target file still\n> has handles open. Those handles must have been opened with the\n> `FILE_SHARE_DELETE` flag, which we have ensured in the preceding\n> commits.\n> > Careful readers might have noticed that [1] does not mention the above\n> flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\n> not for use with `SetFileInformationByHandle()` though, which is what we\n> use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\n> not documented on [2] or anywhere else as far as I can tell.\n\nThe Windows 10 SDK defines FILE_RENAME_FLAG_REPLACE_IF_EXISTS and\nFILE_RENAME_FLAG_POSIX_SEMANTICS for SetFileInformationByHandle(). That\nthe documentation lacks \"_FLAG_\" in the names must be an error in the\ndocumentation.\n\nI found the mention of FILE_RENAME_POSIX_SEMANTICS quite distracting,\nbecause it is a flag to be used with CreateFileW() and basically only\nhas to do with case-sensitivity, but nothing with POSIX semantics of\nrenaming.\n\n> \n> Unfortunately, we still support Windows systems older than Windows 10\n> that do not yet have this new flag. Our `_WIN32_WINNT` SDK version still\n> targets 0x0600, which is Windows Vista and later. And even though that\n> Windows version is out-of-support, bumping the SDK version all the way\n> to 0x0A00, which is Windows 10 and later, is not an option as it would\n> make it impossible to compile on Windows 8.1, which is still supported.\n> Instead, we have to manually declare the relevant infrastructure to make\n> this feature available and have fallback logic in place in case we run\n> on a Windows version that does not yet have this flag.\n> \n> On another note: `mingw_rename()` has a retry loop that is used in case\n> deleting a file failed because it's still open in another process. One\n> might be pressed to not use this loop anymore when we can use POSIX\n> semantics. But unfortunately, we have to keep it around due to our\n> dependence on the `FILE_SHARE_DELETE` flag. While we know to set that\n> sharing flag now, other applications may not do so and may thus still\n> cause sharing violations when we try to rename a file.\n> \n> This fixes concurrent writes in the reftable backend as demonstrated in\n> t0610, but may also end up fixing other usecases where Git wants to\n> perform renames.\n> \n> [1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information\n> [2]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> compat/mingw.c | 87 ++++++++++++++++++++++++++++++++++++--\n> t/t0610-reftable-basics.sh | 8 ++--\n> 2 files changed, 88 insertions(+), 7 deletions(-)\n> \n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 6c75fe36b15..22c005a4533 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -2209,10 +2209,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)\n> #undef rename\n> int mingw_rename(const char *pold, const char *pnew)\n> {\n> +\tstatic int supports_file_rename_info_ex = 1;\n> \tDWORD attrs, gle;\n> \tint tries = 0;\n> \twchar_t wpold[MAX_PATH], wpnew[MAX_PATH];\n> -\tif (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)\n> +\tint wpnew_len;\n> +\n> +\tif (xutftowcs_path(wpold, pold) < 0)\n> +\t\treturn -1;\n> +\twpnew_len = xutftowcs_path(wpnew, pnew);\n> +\tif (wpnew_len < 0)\n> \t\treturn -1;\n> \n> \t/*\n> @@ -2223,11 +2229,84 @@ int mingw_rename(const char *pold, const char *pnew)\n> \t\treturn 0;\n> \tif (errno != EEXIST)\n> \t\treturn -1;\n> +\n> repeat:\n> -\tif (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))\n> -\t\treturn 0;\n> +\tif (supports_file_rename_info_ex) {\n> +\t\t/*\n> +\t\t * Our minimum required Windows version is still set to Windows\n> +\t\t * Vista. We thus have to declare required infrastructure for\n> +\t\t * FileRenameInfoEx ourselves until we bump _WIN32_WINNT to\n> +\t\t * 0x0A00. Furthermore, we have to handle cases where the\n> +\t\t * FileRenameInfoEx call isn't supported yet.\n> +\t\t */\n> +#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS 0x00000001\n> +#define FILE_RENAME_FLAG_POSIX_SEMANTICS 0x00000002\n> +\t\tFILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22;\n> +\t\tstruct {\n> +\t\t\t/*\n> +\t\t\t * This is usually an unnamed union, but that is not\n> +\t\t\t * part of ISO C99. We thus inline the field, as we\n> +\t\t\t * really only care for the Flags field anyway.\n> +\t\t\t */\n> +\t\t\tDWORD Flags;\n> +\t\t\tHANDLE RootDirectory;\n> +\t\t\tDWORD FileNameLength;\n> +\t\t\t/*\n> +\t\t\t * The actual structure is defined with a single-character\n> +\t\t\t * flex array so that the structure has to be allocated on\n> +\t\t\t * the heap. As we declare this structure ourselves though\n> +\t\t\t * we can avoid the allocation and define FileName to have\n> +\t\t\t * MAX_PATH bytes.\n> +\t\t\t */\n> +\t\t\tWCHAR FileName[MAX_PATH];\n> +\t\t} rename_info = { 0 };\n\nGood.\n\n> +\t\tHANDLE old_handle = INVALID_HANDLE_VALUE;\n> +\t\tBOOL success;\n> +\n> +\t\told_handle = CreateFileW(wpold, DELETE,\n> +\t\t\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n> +\t\t\t\t\t NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n> +\t\tif (old_handle == INVALID_HANDLE_VALUE) {\n> +\t\t\terrno = err_win_to_posix(GetLastError());\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\trename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |\n> +\t\t\t\t FILE_RENAME_FLAG_POSIX_SEMANTICS;\n> +\t\trename_info.FileNameLength = wpnew_len * sizeof(WCHAR);\n\nSize is in bytes, not in characters, and without the NUL. Good. I read\none comment on SO, which said that this value is ignored...\n\n> +\t\tmemcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));\n\n... which makes it all the more important that this path is\nNUL-terminated. Yet, this does not copy the NUL. We are still good,\nbecause the buffer is zero-initialized and xutftowcs_path() ensures that\nwpnew_len is at most MAX_PATH-1.\n\n> +\n> +\t\tsuccess = SetFileInformationByHandle(old_handle, FileRenameInfoEx,\n> +\t\t\t\t\t\t &rename_info, sizeof(rename_info));\n> +\t\tgle = GetLastError();\n> +\t\tCloseHandle(old_handle);\n> +\t\tif (success)\n> +\t\t\treturn 0;\n> +\n> +\t\t/*\n> +\t\t * When we see ERROR_INVALID_PARAMETER we can assume that the\n> +\t\t * current system doesn't support FileRenameInfoEx. Keep us\n> +\t\t * from using it in future calls and retry.\n> +\t\t */\n> +\t\tif (gle == ERROR_INVALID_PARAMETER) {\n> +\t\t\tsupports_file_rename_info_ex = 0;\n> +\t\t\tgoto repeat;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * In theory, we shouldn't get ERROR_ACCESS_DENIED because we\n> +\t\t * always open files with FILE_SHARE_DELETE But in practice we\n> +\t\t * cannot assume that Git is the only one accessing files, and\n> +\t\t * other applications may not set FILE_SHARE_DELETE. So we have\n> +\t\t * to retry.\n> +\t\t */\n\nGood thinking!\n\n> +\t} else {\n> +\t\tif (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))\n> +\t\t\treturn 0;\n> +\t\tgle = GetLastError();\n> +\t}\n> +\n> \t/* TODO: translate more errors */\n> -\tgle = GetLastError();\n> \tif (gle == ERROR_ACCESS_DENIED &&\n> \t (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {\n> \t\tif (attrs & FILE_ATTRIBUTE_DIRECTORY) {\n> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> index babec7993e3..eaf6fab6d29 100755\n> --- a/t/t0610-reftable-basics.sh\n> +++ b/t/t0610-reftable-basics.sh\n> @@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n> \t)\n> '\n> \n> -# This test fails most of the time on Windows systems. The root cause is\n> +# This test fails most of the time on Cygwin systems. The root cause is\n> # that Windows does not allow us to rename the \"tables.list.lock\" file into\n> -# place when \"tables.list\" is open for reading by a concurrent process.\n> -test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n> +# place when \"tables.list\" is open for reading by a concurrent process. We have\n> +# worked around that in our MinGW-based rename emulation, but the Cygwin\n> +# emulation seems to be insufficient.\n> +test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '\n> \ttest_when_finished \"rm -rf repo\" &&\n> \tgit init repo &&\n> \t(\n\nThe general structure of the patch makes a lot of sense!\n\n-- Hannes\n\n"},{"id":"506149","messageId":"4931df84-6c5b-4f48-8917-eb7cc99134c2@kdbg.org","threadId":"62397","inReplyTo":"cover.1729770140.git.ps@pks.im","subject":"Re: [PATCH v2 0/3] compat/mingw: implement POSIX-style atomic renames","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-10-27T13:27:59Z","receivedAt":"2024-10-27T13:28:06Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:\n> Hi,\n> \n> this is the second patch series that implements POSIX-style atomic\n> renames on Windows in order to fix concurrent writes with the reftable\n> backend.\n> \n> Changes compared to v1:\n> \n> - Added some historic digging to the first commit message.\n> \n> - Fix various spelling mistakes.\n> \n> - Fix indentation.\n> \n> - Don't use the comma operator to assign `errno`.\n> \n> Thanks!\n> \n> Patrick\n\nThank you for working on this.\n\nThe patches look good in general, but I noticed that oflags handling in\n2/3 needs to be fixed.\n\nI ran the test suite on my Windows 10 box with this series, and it\npassed all tests (with my suggested oflags fix applied). I also\ncross-checked whether I would observe the failure that the series\nattempts to fix, and I do see the failure, and the series fixes it.\n\n-- Hannes\n\n"},{"id":"506151","messageId":"bc0c9853-7dd4-4b1d-9985-25f75815b0a9@kdbg.org","threadId":"62397","inReplyTo":"ZxkxAvLeMtxstVdS@nand.local","subject":"Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-10-27T13:14:30Z","receivedAt":"2024-10-27T13:48:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 23.10.24 um 19:23 schrieb Taylor Blau:\n> On Wed, Oct 23, 2024 at 05:04:58PM +0200, Patrick Steinhardt wrote:\n>> Unless told otherwise, Windows will keep other processes from reading,\n>> writing and deleting files when one has an open handle that was created\n>> via `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\n>> flags such that other processes _can_ read and/or modify such a file.\n>> This sharing mechanism is quite important in the context of Git, as we\n>> assume POSIX semantics all over the place.\n>>\n>> There are two calls where we don't set up those flags though:\n>>\n>> - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n>> via `mingw_open_append()`. This makes it impossible to delete the\n>> file from another process or to replace it via an atomic rename.\n>>\n>> - When opening a file such that we can change its access/modification\n>> times. This makes it impossible to perform any kind of operation\n>> on this file at all from another process. While we only open the\n>> file for a short amount of time to update its timestamps, this still\n>> opens us up for a race condition with another process.\n>>\n>> Adapt both of these callsites to pass all three sharing flags.\n> \n> Interesting, and especially so noting that we *do* call CreateFileW()\n> with the FILE_SHARE_DELETE flag in other functions like create_watch(),\n> mingw_open_existing(), mingw_getcwd(), etc.\n> \n> Why didn't we call CreateFileW() with FILE_SHARE_DELETE in the below two\n> functions? I assume Johannes Schindelin and/or Johannes Sixt (both CC'd)\n> would know the answer. Regardless, it would be interesting and useful\n> (IMHO) to include such a detail in the commit message.\n\nMy attitude in the past was that deleting a file that is open elsewhere\nis a bug, so FILE_SHARE_DELETE would not be needed, but its absence\ncould point to a bug elsewhere. Now that we have a reftable\nimplementation, it looks like I can't uphold this attitude anymore.\n\n-- Hannes\n\n"},{"id":"506154","messageId":"Zx5eg2SkHMmNhQRh@pks.im","threadId":"62397","inReplyTo":"542b306a-523d-424b-bc9f-c63bb7040beb@kdbg.org","subject":"Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-27T15:38:43Z","receivedAt":"2024-10-27T15:38:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Oct 27, 2024 at 02:23:28PM +0100, Johannes Sixt wrote:\n> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:\n> > By default, Windows restricts access to files when those files have been\n> > opened by another process. As explained in the preceding commits, these\n> > restrictions can be loosened such that reads, writes and/or deletes of\n> > files with open handles _are_ allowed.\n> > \n> > While we set up those sharing flags in most relevant code paths now, we\n> > still don't properly handle POSIX-style atomic renames in case the\n> > target path is open. This is failure demonstrated by t0610, where one of\n> > our tests spawns concurrent writes in a reftable-enabled repository and\n> > expects all of them to succeed. This test fails most of the time because\n> > the process that has acquired the \"tables.list\" lock is unable to rename\n> > it into place while other processes are busy reading that file.\n> > \n> > Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag\n> > that allows us to fix this usecase [1]. When set, it is possible to\n> > rename a file over a preexisting file even when the target file still\n> > has handles open. Those handles must have been opened with the\n> > `FILE_SHARE_DELETE` flag, which we have ensured in the preceding\n> > commits.\n> > > Careful readers might have noticed that [1] does not mention the above\n> > flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\n> > not for use with `SetFileInformationByHandle()` though, which is what we\n> > use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\n> > not documented on [2] or anywhere else as far as I can tell.\n> \n> The Windows 10 SDK defines FILE_RENAME_FLAG_REPLACE_IF_EXISTS and\n> FILE_RENAME_FLAG_POSIX_SEMANTICS for SetFileInformationByHandle(). That\n> the documentation lacks \"_FLAG_\" in the names must be an error in the\n> documentation.\n> \n> I found the mention of FILE_RENAME_POSIX_SEMANTICS quite distracting,\n> because it is a flag to be used with CreateFileW() and basically only\n> has to do with case-sensitivity, but nothing with POSIX semantics of\n> renaming.\n\nI'd still prefer to mention this, because otherwise an astute reader\nmight notice that I'm using a different flag name than what is\ndocumented in the docs and figure out that I defined the wrong flag\nname.\n\n[snip]\n> > +\t\tHANDLE old_handle = INVALID_HANDLE_VALUE;\n> > +\t\tBOOL success;\n> > +\n> > +\t\told_handle = CreateFileW(wpold, DELETE,\n> > +\t\t\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n> > +\t\t\t\t\t NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n> > +\t\tif (old_handle == INVALID_HANDLE_VALUE) {\n> > +\t\t\terrno = err_win_to_posix(GetLastError());\n> > +\t\t\treturn -1;\n> > +\t\t}\n> > +\n> > +\t\trename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |\n> > +\t\t\t\t FILE_RENAME_FLAG_POSIX_SEMANTICS;\n> > +\t\trename_info.FileNameLength = wpnew_len * sizeof(WCHAR);\n> \n> Size is in bytes, not in characters, and without the NUL. Good. I read\n> one comment on SO, which said that this value is ignored...\n\nYeah, I noticed at one point that it didn't really make a difference\nwhat I pass here.\n\n> > +\t\tmemcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));\n> \n> ... which makes it all the more important that this path is\n> NUL-terminated. Yet, this does not copy the NUL. We are still good,\n> because the buffer is zero-initialized and xutftowcs_path() ensures that\n> wpnew_len is at most MAX_PATH-1.\n\nYup.\n\n[snip]\n> The general structure of the patch makes a lot of sense!\n\nGreat, thanks for your review! I'll send a revised version of this\nseries where I adapt the second patch.\n\nPatrick\n"},{"id":"506155","messageId":"Zx5eipv_Un3x1j-4@pks.im","threadId":"62397","inReplyTo":"a769d5a6-0d0f-4df4-b581-539d00aacbc9@kdbg.org","subject":"Re: [PATCH v2 2/3] compat/mingw: allow deletion of most opened files","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-27T15:38:50Z","receivedAt":"2024-10-27T15:38:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Oct 27, 2024 at 02:17:36PM +0100, Johannes Sixt wrote:\n> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:\n> > diff --git a/compat/mingw.c b/compat/mingw.c\n> > index e326c6fcd2d..6c75fe36b15 100644\n> > --- a/compat/mingw.c\n> > +++ b/compat/mingw.c\n> > @@ -532,6 +532,62 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n> > \treturn fd;\n> > }\n> > \n> > +/*\n> > + * Ideally, we'd use `_wopen()` to implement this functionality so that we\n> > + * don't have to reimplement it, but unfortunately we do not have tight control\n> > + * over the share mode there. And while `_wsopen()` and friends exist that give\n> > + * us _some_ control over the share mode, this family of functions doesn't give\n> > + * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict\n> > + * requirement for us though so that we can unlink or rename over files that\n> > + * are held open by another process.\n> > + *\n> > + * We are thus forced to implement our own emulation of `open()`. To make our\n> > + * life simpler we only implement basic support for this, namely opening\n> > + * existing files for reading and/or writing. This means that newly created\n> > + * files won't have their sharing mode set up correctly, but for now I couldn't\n> > + * find any case where this matters. We may have to revisit that in the future\n> > + * though based on our needs.\n> > + */\n> > +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)\n> > +{\n> > +\tSECURITY_ATTRIBUTES security_attributes = {\n> > +\t\t.nLength = sizeof(security_attributes),\n> > +\t\t.bInheritHandle = !(oflags & O_NOINHERIT),\n> > +\t};\n> > +\tHANDLE handle;\n> > +\tint access;\n> \n> I would have made this variable DWORD or unsigned instead of plain int,\n> because it receives a bit pattern and would match the parameter type of\n> CreateFileW() better; but no big deal.\n\nI have to reroll this series anyway, so I'll just fix this while at it.\n\n> > +\tint fd;\n> > +\n> > +\t/* We only support basic flags. */\n> > +\tif (oflags & ~(O_ACCMODE | O_NOINHERIT)) {\n> > +\t\terrno = ENOSYS;\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\tif (oflags & O_RDWR)\n> > +\t\taccess = GENERIC_READ | GENERIC_WRITE;\n> > +\telse if (oflags & O_WRONLY)\n> > +\t\taccess = GENERIC_WRITE;\n> > +\telse\n> > +\t\taccess = GENERIC_READ;\n> \n> O_RDWR, O_WRONLY and O_RDONLY are not flags, but values occupying two\n> bits of oflags. This must be:\n> \n> \tif ((oflags & O_ACCMODE) == O_RDWR)\n> \t\taccess = GENERIC_READ | GENERIC_WRITE;\n> \telse if ((oflags & O_ACCMODE) == O_WRONLY)\n> \t\taccess = GENERIC_WRITE;\n> \telse\n> \t\taccess = GENERIC_READ;\n> \n> or similar.\n\nAh, that makes sense indeed. Will fix.\n\n> > +\n> > +\thandle = CreateFileW(filename, access,\n> > +\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n> > +\t\t\t &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n> > +\tif (handle == INVALID_HANDLE_VALUE) {\n> > +\t\tDWORD err = GetLastError();\n> > +\t\tif (err == ERROR_INVALID_PARAMETER)\n> > +\t\t\terr = ERROR_PATH_NOT_FOUND;\n> \n> First I wondered what this is about, but then noticed that it is just\n> copied from the mingw_open_append() implementation catering to some\n> bogus network filesystems. Good.\n\nI'll add a comment.\n\nPatrick\n"},{"id":"506156","messageId":"cover.1730042775.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1729695349.git.ps@pks.im","subject":"[PATCH v3 0/3] compat/mingw: implement POSIX-style atomic renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-27T15:39:32Z","receivedAt":"2024-10-27T15:39:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is third version of my patch series that introduces POSIX-style\natomic renames on Windows to fix concurrent writes with the reftable\nbackend.\n\nChanges compared to v2:\n\n - Use `DWORD` instead of `int` to store the `access` flags.\n\n - Fix handling of `oflags`.\n\n - Add a comment referring to `mingw_open_append()` for why convert\n `ERROR_INVALID_PARAMETER` to `ERROR_PATH_NOT_FOUND`.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n compat/mingw: share file handles created via `CreateFileW()`\n compat/mingw: allow deletion of most opened files\n compat/mingw: support POSIX semantics for atomic renames\n\n compat/mingw.c | 157 +++++++++++++++++++++++++++++++++++--\n t/t0610-reftable-basics.sh | 8 +-\n 2 files changed, 156 insertions(+), 9 deletions(-)\n\nRange-diff against v2:\n1: 63c2cfa87a4 = 1: 63c2cfa87a4 compat/mingw: share file handles created via `CreateFileW()`\n2: c308eafbbb5 ! 2: 91d434116f3 compat/mingw: allow deletion of most opened files\n @@ compat/mingw.c: static int mingw_open_append(wchar_t const *wfilename, int oflag\n +\t\t.bInheritHandle = !(oflags & O_NOINHERIT),\n +\t};\n +\tHANDLE handle;\n -+\tint access;\n ++\tDWORD access;\n +\tint fd;\n +\n +\t/* We only support basic flags. */\n @@ compat/mingw.c: static int mingw_open_append(wchar_t const *wfilename, int oflag\n +\t\treturn -1;\n +\t}\n +\n -+\tif (oflags & O_RDWR)\n ++\tswitch (oflags & O_ACCMODE) {\n ++\tcase O_RDWR:\n +\t\taccess = GENERIC_READ | GENERIC_WRITE;\n -+\telse if (oflags & O_WRONLY)\n ++\t\tbreak;\n ++\tcase O_WRONLY:\n +\t\taccess = GENERIC_WRITE;\n -+\telse\n ++\t\tbreak;\n ++\tdefault:\n +\t\taccess = GENERIC_READ;\n ++\t\tbreak;\n ++\t}\n +\n +\thandle = CreateFileW(filename, access,\n +\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n +\t\t\t &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n +\tif (handle == INVALID_HANDLE_VALUE) {\n +\t\tDWORD err = GetLastError();\n ++\n ++\t\t/* See `mingw_open_append()` for why we have this conversion. */\n +\t\tif (err == ERROR_INVALID_PARAMETER)\n +\t\t\terr = ERROR_PATH_NOT_FOUND;\n ++\n +\t\terrno = err_win_to_posix(err);\n +\t\treturn -1;\n +\t}\n3: ee1e92e593e = 3: 2447d332a85 compat/mingw: support POSIX semantics for atomic renames\n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"506157","messageId":"63c2cfa87a4a3c44367acdc1f7cdb57289b4b761.1730042775.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1730042775.git.ps@pks.im","subject":"[PATCH v3 1/3] compat/mingw: share file handles created via `CreateFileW()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-27T15:39:37Z","receivedAt":"2024-10-27T15:39:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Unless told otherwise, Windows will keep other processes from reading,\nwriting and deleting files when one has an open handle that was created\nvia `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\nflags:\n\n - `FILE_SHARE_READ` allows a concurrent process to open the file for\n reading.\n\n - `FILE_SHARE_WRITE` allows a concurrent process to open the file for\n writing.\n\n - `FILE_SHARE_DELETE` allows a concurrent process to delete the file\n or to replace it via an atomic rename.\n\nThis sharing mechanism is quite important in the context of Git, as we\nassume POSIX semantics all over the place. But there are two callsites\nwhere we don't pass all three of these flags:\n\n - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n via `mingw_open_append()`. This makes it impossible to delete the\n file from another process or to replace it via an atomic rename. The\n function was introduced via d641097589 (mingw: enable atomic\n O_APPEND, 2018-08-13) and has been using `FILE_SHARE_READ |\n FILE_SHARE_WRITE` since the inception. There aren't any indicators\n that the omission of `FILE_SHARE_DELETE` was intentional.\n\n - We don't set any sharing flags in `mingw_utime()`, which changes the\n access and modification of a file. This makes it impossible to\n perform any kind of operation on this file at all from another\n process. While we only open the file for a short amount of time to\n update its timestamps, this still opens us up for a race condition\n with another process.\n\n `mingw_utime()` was originally implemented via `_wopen()`, which\n doesn't give you full control over the sharing mode. Instead, it\n calls `_wsopen()` with `_SH_DENYNO`, which ultimately translates to\n `FILE_SHARE_READ | FILE_SHARE_WRITE`. It was then refactored via\n 090a3085bc (t/helper/test-chmtime: update mingw to support chmtime\n on directories, 2022-03-02) to use `CreateFileW()`, but we stopped\n setting any sharing flags at all, which seems like an unintentional\n side effect. By restoring `FILE_SHARE_READ | FILE_SHARE_WRITE` we\n thus fix this and get back the old behaviour of `_wopen()`.\n\n The fact that we didn't set the equivalent of `FILE_SHARE_DELETE`\n can be explained, as well: neither `_wopen()` nor `_wsopen()` allow\n you to do so. So overall, it doesn't seem intentional that we didn't\n allow deletions here, either.\n\nAdapt both of these callsites to pass all three sharing flags.\n\nSigned-off-by: Patrick Steinhardt \n---\n compat/mingw.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 0e851ecae29..e326c6fcd2d 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -502,7 +502,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n \t * to append to the file.\n \t */\n \thandle = CreateFileW(wfilename, FILE_APPEND_DATA,\n-\t\t\tFILE_SHARE_WRITE | FILE_SHARE_READ,\n+\t\t\tFILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n \t\t\tNULL, create, FILE_ATTRIBUTE_NORMAL, NULL);\n \tif (handle == INVALID_HANDLE_VALUE) {\n \t\tDWORD err = GetLastError();\n@@ -1006,7 +1006,7 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)\n \n \tosfilehandle = CreateFileW(wfilename,\n \t\t\t\t FILE_WRITE_ATTRIBUTES,\n-\t\t\t\t 0 /*FileShare.None*/,\n+\t\t\t\t FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,\n \t\t\t\t NULL,\n \t\t\t\t OPEN_EXISTING,\n \t\t\t\t (attrs != INVALID_FILE_ATTRIBUTES &&\n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"506158","messageId":"91d434116f3eec4444316804dfb3d955c9750a6b.1730042775.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1730042775.git.ps@pks.im","subject":"[PATCH v3 2/3] compat/mingw: allow deletion of most opened files","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-27T15:39:40Z","receivedAt":"2024-10-27T15:39:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Windows, we emulate open(3p) via `mingw_open()`. This function\nimplements handling of some platform-specific quirks that are required\nto make it behave as closely as possible like open(3p) would, but for\nmost cases we just call the Windows-specific `_wopen()` function.\n\nThis function has a major downside though: it does not allow us to\nspecify the sharing mode. While there is `_wsopen()` that allows us to\npass sharing flags, those sharing flags are not the same `FILE_SHARE_*`\nflags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows\nconcurrent read- and write-access, but does not allow for concurrent\ndeletions. Unfortunately though, we have to allow concurrent deletions\nif we want to have POSIX-style atomic renames on top of an existing file\nthat has open file handles.\n\nImplement a new function that emulates open(3p) for existing files via\n`CreateFileW()` such that we can set the required sharing flags.\n\nWhile we have the same issue when calling open(3p) with `O_CREAT`,\nimplementing that mode would be more complex due to the required\npermission handling. Furthermore, atomic updates via renames typically\nwrite to exclusive lockfile and then perform the rename, and thus we\ndon't have to handle the case where the locked path has been created\nwith `O_CREATE`. So while it would be nice to have proper POSIX\nsemantics in all paths, we instead aim for a minimum viable fix here.\n\nSigned-off-by: Patrick Steinhardt \n---\n compat/mingw.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 66 insertions(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex e326c6fcd2d..0d9600543cb 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -532,6 +532,70 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n \treturn fd;\n }\n \n+/*\n+ * Ideally, we'd use `_wopen()` to implement this functionality so that we\n+ * don't have to reimplement it, but unfortunately we do not have tight control\n+ * over the share mode there. And while `_wsopen()` and friends exist that give\n+ * us _some_ control over the share mode, this family of functions doesn't give\n+ * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict\n+ * requirement for us though so that we can unlink or rename over files that\n+ * are held open by another process.\n+ *\n+ * We are thus forced to implement our own emulation of `open()`. To make our\n+ * life simpler we only implement basic support for this, namely opening\n+ * existing files for reading and/or writing. This means that newly created\n+ * files won't have their sharing mode set up correctly, but for now I couldn't\n+ * find any case where this matters. We may have to revisit that in the future\n+ * though based on our needs.\n+ */\n+static int mingw_open_existing(const wchar_t *filename, int oflags, ...)\n+{\n+\tSECURITY_ATTRIBUTES security_attributes = {\n+\t\t.nLength = sizeof(security_attributes),\n+\t\t.bInheritHandle = !(oflags & O_NOINHERIT),\n+\t};\n+\tHANDLE handle;\n+\tDWORD access;\n+\tint fd;\n+\n+\t/* We only support basic flags. */\n+\tif (oflags & ~(O_ACCMODE | O_NOINHERIT)) {\n+\t\terrno = ENOSYS;\n+\t\treturn -1;\n+\t}\n+\n+\tswitch (oflags & O_ACCMODE) {\n+\tcase O_RDWR:\n+\t\taccess = GENERIC_READ | GENERIC_WRITE;\n+\t\tbreak;\n+\tcase O_WRONLY:\n+\t\taccess = GENERIC_WRITE;\n+\t\tbreak;\n+\tdefault:\n+\t\taccess = GENERIC_READ;\n+\t\tbreak;\n+\t}\n+\n+\thandle = CreateFileW(filename, access,\n+\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n+\t\t\t &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n+\tif (handle == INVALID_HANDLE_VALUE) {\n+\t\tDWORD err = GetLastError();\n+\n+\t\t/* See `mingw_open_append()` for why we have this conversion. */\n+\t\tif (err == ERROR_INVALID_PARAMETER)\n+\t\t\terr = ERROR_PATH_NOT_FOUND;\n+\n+\t\terrno = err_win_to_posix(err);\n+\t\treturn -1;\n+\t}\n+\n+\tfd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY);\n+\tif (fd < 0)\n+\t\tCloseHandle(handle);\n+\treturn fd;\n+}\n+\n /*\n * Does the pathname map to the local named pipe filesystem?\n * That is, does it have a \"//./pipe/\" prefix?\n@@ -567,6 +631,8 @@ int mingw_open (const char *filename, int oflags, ...)\n \n \tif ((oflags & O_APPEND) && !is_local_named_pipe_path(filename))\n \t\topen_fn = mingw_open_append;\n+\telse if (!(oflags & ~(O_ACCMODE | O_NOINHERIT)))\n+\t\topen_fn = mingw_open_existing;\n \telse\n \t\topen_fn = _wopen;\n \n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"506159","messageId":"2447d332a851e41e613dec181c347d5a15c310ab.1730042775.git.ps@pks.im","threadId":"62397","inReplyTo":"cover.1730042775.git.ps@pks.im","subject":"[PATCH v3 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-27T15:39:43Z","receivedAt":"2024-10-27T15:39:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"By default, Windows restricts access to files when those files have been\nopened by another process. As explained in the preceding commits, these\nrestrictions can be loosened such that reads, writes and/or deletes of\nfiles with open handles _are_ allowed.\n\nWhile we set up those sharing flags in most relevant code paths now, we\nstill don't properly handle POSIX-style atomic renames in case the\ntarget path is open. This is failure demonstrated by t0610, where one of\nour tests spawns concurrent writes in a reftable-enabled repository and\nexpects all of them to succeed. This test fails most of the time because\nthe process that has acquired the \"tables.list\" lock is unable to rename\nit into place while other processes are busy reading that file.\n\nWindows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag\nthat allows us to fix this usecase [1]. When set, it is possible to\nrename a file over a preexisting file even when the target file still\nhas handles open. Those handles must have been opened with the\n`FILE_SHARE_DELETE` flag, which we have ensured in the preceding\ncommits.\n\nCareful readers might have noticed that [1] does not mention the above\nflag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\nnot for use with `SetFileInformationByHandle()` though, which is what we\nuse. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\nnot documented on [2] or anywhere else as far as I can tell.\n\nUnfortunately, we still support Windows systems older than Windows 10\nthat do not yet have this new flag. Our `_WIN32_WINNT` SDK version still\ntargets 0x0600, which is Windows Vista and later. And even though that\nWindows version is out-of-support, bumping the SDK version all the way\nto 0x0A00, which is Windows 10 and later, is not an option as it would\nmake it impossible to compile on Windows 8.1, which is still supported.\nInstead, we have to manually declare the relevant infrastructure to make\nthis feature available and have fallback logic in place in case we run\non a Windows version that does not yet have this flag.\n\nOn another note: `mingw_rename()` has a retry loop that is used in case\ndeleting a file failed because it's still open in another process. One\nmight be pressed to not use this loop anymore when we can use POSIX\nsemantics. But unfortunately, we have to keep it around due to our\ndependence on the `FILE_SHARE_DELETE` flag. While we know to set that\nsharing flag now, other applications may not do so and may thus still\ncause sharing violations when we try to rename a file.\n\nThis fixes concurrent writes in the reftable backend as demonstrated in\nt0610, but may also end up fixing other usecases where Git wants to\nperform renames.\n\n[1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information\n[2]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info\n\nSigned-off-by: Patrick Steinhardt \n---\n compat/mingw.c | 87 ++++++++++++++++++++++++++++++++++++--\n t/t0610-reftable-basics.sh | 8 ++--\n 2 files changed, 88 insertions(+), 7 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 0d9600543cb..c4320769db6 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2217,10 +2217,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz)\n #undef rename\n int mingw_rename(const char *pold, const char *pnew)\n {\n+\tstatic int supports_file_rename_info_ex = 1;\n \tDWORD attrs, gle;\n \tint tries = 0;\n \twchar_t wpold[MAX_PATH], wpnew[MAX_PATH];\n-\tif (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0)\n+\tint wpnew_len;\n+\n+\tif (xutftowcs_path(wpold, pold) < 0)\n+\t\treturn -1;\n+\twpnew_len = xutftowcs_path(wpnew, pnew);\n+\tif (wpnew_len < 0)\n \t\treturn -1;\n \n \t/*\n@@ -2231,11 +2237,84 @@ int mingw_rename(const char *pold, const char *pnew)\n \t\treturn 0;\n \tif (errno != EEXIST)\n \t\treturn -1;\n+\n repeat:\n-\tif (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))\n-\t\treturn 0;\n+\tif (supports_file_rename_info_ex) {\n+\t\t/*\n+\t\t * Our minimum required Windows version is still set to Windows\n+\t\t * Vista. We thus have to declare required infrastructure for\n+\t\t * FileRenameInfoEx ourselves until we bump _WIN32_WINNT to\n+\t\t * 0x0A00. Furthermore, we have to handle cases where the\n+\t\t * FileRenameInfoEx call isn't supported yet.\n+\t\t */\n+#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS 0x00000001\n+#define FILE_RENAME_FLAG_POSIX_SEMANTICS 0x00000002\n+\t\tFILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22;\n+\t\tstruct {\n+\t\t\t/*\n+\t\t\t * This is usually an unnamed union, but that is not\n+\t\t\t * part of ISO C99. We thus inline the field, as we\n+\t\t\t * really only care for the Flags field anyway.\n+\t\t\t */\n+\t\t\tDWORD Flags;\n+\t\t\tHANDLE RootDirectory;\n+\t\t\tDWORD FileNameLength;\n+\t\t\t/*\n+\t\t\t * The actual structure is defined with a single-character\n+\t\t\t * flex array so that the structure has to be allocated on\n+\t\t\t * the heap. As we declare this structure ourselves though\n+\t\t\t * we can avoid the allocation and define FileName to have\n+\t\t\t * MAX_PATH bytes.\n+\t\t\t */\n+\t\t\tWCHAR FileName[MAX_PATH];\n+\t\t} rename_info = { 0 };\n+\t\tHANDLE old_handle = INVALID_HANDLE_VALUE;\n+\t\tBOOL success;\n+\n+\t\told_handle = CreateFileW(wpold, DELETE,\n+\t\t\t\t\t FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,\n+\t\t\t\t\t NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);\n+\t\tif (old_handle == INVALID_HANDLE_VALUE) {\n+\t\t\terrno = err_win_to_posix(GetLastError());\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\trename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS |\n+\t\t\t\t FILE_RENAME_FLAG_POSIX_SEMANTICS;\n+\t\trename_info.FileNameLength = wpnew_len * sizeof(WCHAR);\n+\t\tmemcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR));\n+\n+\t\tsuccess = SetFileInformationByHandle(old_handle, FileRenameInfoEx,\n+\t\t\t\t\t\t &rename_info, sizeof(rename_info));\n+\t\tgle = GetLastError();\n+\t\tCloseHandle(old_handle);\n+\t\tif (success)\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * When we see ERROR_INVALID_PARAMETER we can assume that the\n+\t\t * current system doesn't support FileRenameInfoEx. Keep us\n+\t\t * from using it in future calls and retry.\n+\t\t */\n+\t\tif (gle == ERROR_INVALID_PARAMETER) {\n+\t\t\tsupports_file_rename_info_ex = 0;\n+\t\t\tgoto repeat;\n+\t\t}\n+\n+\t\t/*\n+\t\t * In theory, we shouldn't get ERROR_ACCESS_DENIED because we\n+\t\t * always open files with FILE_SHARE_DELETE But in practice we\n+\t\t * cannot assume that Git is the only one accessing files, and\n+\t\t * other applications may not set FILE_SHARE_DELETE. So we have\n+\t\t * to retry.\n+\t\t */\n+\t} else {\n+\t\tif (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING))\n+\t\t\treturn 0;\n+\t\tgle = GetLastError();\n+\t}\n+\n \t/* TODO: translate more errors */\n-\tgle = GetLastError();\n \tif (gle == ERROR_ACCESS_DENIED &&\n \t (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) {\n \t\tif (attrs & FILE_ATTRIBUTE_DIRECTORY) {\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex babec7993e3..eaf6fab6d29 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' '\n \t)\n '\n \n-# This test fails most of the time on Windows systems. The root cause is\n+# This test fails most of the time on Cygwin systems. The root cause is\n # that Windows does not allow us to rename the \"tables.list.lock\" file into\n-# place when \"tables.list\" is open for reading by a concurrent process.\n-test_expect_success !WINDOWS 'ref transaction: many concurrent writers' '\n+# place when \"tables.list\" is open for reading by a concurrent process. We have\n+# worked around that in our MinGW-based rename emulation, but the Cygwin\n+# emulation seems to be insufficient.\n+test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n-- \n2.47.0.118.gfd3785337b.dirty\n\n"},{"id":"506160","messageId":"49619b52-0fea-4179-a829-7ec4a6945055@kdbg.org","threadId":"62397","inReplyTo":"Zx5eg2SkHMmNhQRh@pks.im","subject":"Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-10-27T16:31:00Z","receivedAt":"2024-10-27T16:31:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 27.10.24 um 16:38 schrieb Patrick Steinhardt:\n> On Sun, Oct 27, 2024 at 02:23:28PM +0100, Johannes Sixt wrote:\n>> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:\n>>> Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag\n>>> that allows us to fix this usecase [1]. When set, it is possible to\n>>> rename a file over a preexisting file even when the target file still\n>>> has handles open. Those handles must have been opened with the\n>>> `FILE_SHARE_DELETE` flag, which we have ensured in the preceding\n>>> commits.\n>>>> Careful readers might have noticed that [1] does not mention the above\n>>> flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\n>>> not for use with `SetFileInformationByHandle()` though, which is what we\n>>> use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\n>>> not documented on [2] or anywhere else as far as I can tell.\n>>\n>> The Windows 10 SDK defines FILE_RENAME_FLAG_REPLACE_IF_EXISTS and\n>> FILE_RENAME_FLAG_POSIX_SEMANTICS for SetFileInformationByHandle(). That\n>> the documentation lacks \"_FLAG_\" in the names must be an error in the\n>> documentation.\n>>\n>> I found the mention of FILE_RENAME_POSIX_SEMANTICS quite distracting,\n>> because it is a flag to be used with CreateFileW() and basically only\n>> has to do with case-sensitivity, but nothing with POSIX semantics of\n>> renaming.\n> \n> I'd still prefer to mention this, because otherwise an astute reader\n> might notice that I'm using a different flag name than what is\n> documented in the docs and figure out that I defined the wrong flag\n> name.\n\nAh, I was confused twice here. First, the documentation that you cite[*]\nmentions FILE_RENAME_POSIX_SEMANTICS, but the name does not exist at\nall. There does exist FILE_RENAME_FLAG_POSIX_SEMANTICS. The\ndocumentation is just wrong. And in my earlier comment I copied the\ninexistent flag name.\n\nBut I meant to cite this flag: FILE_FLAG_POSIX_SEMANTICS (no \"RENAME\").\nIt exists and is for CreateFileW().\n\nPerhaps you also meant cite the latter one as the flag that \"is not for\nuse with `SetFileInformationByHandle()`\"?\n\nAt any rate, the paragraph as written isn't correct.\n\n[1]:\nhttps://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information\n\n-- Hannes\n\n"},{"id":"506161","messageId":"Zx54B6FdGprtH0HV@pks.im","threadId":"62397","inReplyTo":"49619b52-0fea-4179-a829-7ec4a6945055@kdbg.org","subject":"Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-27T17:27:35Z","receivedAt":"2024-10-27T17:27:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Oct 27, 2024 at 05:31:00PM +0100, Johannes Sixt wrote:\n> Am 27.10.24 um 16:38 schrieb Patrick Steinhardt:\n> > On Sun, Oct 27, 2024 at 02:23:28PM +0100, Johannes Sixt wrote:\n> >> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:\n> >>> Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag\n> >>> that allows us to fix this usecase [1]. When set, it is possible to\n> >>> rename a file over a preexisting file even when the target file still\n> >>> has handles open. Those handles must have been opened with the\n> >>> `FILE_SHARE_DELETE` flag, which we have ensured in the preceding\n> >>> commits.\n> >>>> Careful readers might have noticed that [1] does not mention the above\n> >>> flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\n> >>> not for use with `SetFileInformationByHandle()` though, which is what we\n> >>> use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\n> >>> not documented on [2] or anywhere else as far as I can tell.\n> >>\n> >> The Windows 10 SDK defines FILE_RENAME_FLAG_REPLACE_IF_EXISTS and\n> >> FILE_RENAME_FLAG_POSIX_SEMANTICS for SetFileInformationByHandle(). That\n> >> the documentation lacks \"_FLAG_\" in the names must be an error in the\n> >> documentation.\n> >>\n> >> I found the mention of FILE_RENAME_POSIX_SEMANTICS quite distracting,\n> >> because it is a flag to be used with CreateFileW() and basically only\n> >> has to do with case-sensitivity, but nothing with POSIX semantics of\n> >> renaming.\n> > \n> > I'd still prefer to mention this, because otherwise an astute reader\n> > might notice that I'm using a different flag name than what is\n> > documented in the docs and figure out that I defined the wrong flag\n> > name.\n> \n> Ah, I was confused twice here. First, the documentation that you cite[*]\n> mentions FILE_RENAME_POSIX_SEMANTICS, but the name does not exist at\n> all. There does exist FILE_RENAME_FLAG_POSIX_SEMANTICS. The\n> documentation is just wrong. And in my earlier comment I copied the\n> inexistent flag name.\n> \n> But I meant to cite this flag: FILE_FLAG_POSIX_SEMANTICS (no \"RENAME\").\n> It exists and is for CreateFileW().\n> \n> Perhaps you also meant cite the latter one as the flag that \"is not for\n> use with `SetFileInformationByHandle()`\"?\n> \n> At any rate, the paragraph as written isn't correct.\n\nI think I'm missing something. That's what the paragraph says:\n\n Careful readers might have noticed that [1] does not mention the above\n flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\n not for use with `SetFileInformationByHandle()` though, which is what we\n use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\n not documented on [2] or anywhere else as far as I can tell.\n\nAnd I'd claim it is correct.\n\n`FILE_RENAME_POSIX_SEMANTICS` exists, this it is not a documentation\nerror. It is at a lower level than `FILE_RENAME_FLAG_POSIX_SEMANTICS`,\nthe documentation at [1] refers to \"ntifs.h\", which is part of the\nWindows Driver Kit interfaces. So it is not supposed to be used with\n`SetFileInformationByHandle()`, but with `FtlSetInformationFile()` [2],\nwhich _also_ has a separate `FILE_RENAME_INFO` structure that looks the\nsame as `FILE_RENAME_INFO` defined for `SetFileInformationByHandle()`.\nThe only difference as far as I can tell is that the flags used for\nthese structures have slightly different names.\n\nNow I totally get your confusion -- I have been extremely confused by\nall of this, as well. It certainly is a documentation error that the\nrespective `FILE_RENAME_FLAG_POSIX_SEMANTICS` is undocumented, but\nhaving proper docs for this is rather important such that the reader\nknows what its behaviour is. So I have no other choice than to link to\nthe ntifs interfaces, as it documents the actual behaviour, even though\nit lives in a different part of the Windows APIs.\n\nPatrick\n\n[1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information\n[2]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/fltkernel/nf-fltkernel-fltsetinformationfile\n"},{"id":"506164","messageId":"c044ea3c-2f8c-45c1-99cf-17d9dad5b55f@kdbg.org","threadId":"62397","inReplyTo":"Zx54B6FdGprtH0HV@pks.im","subject":"Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-10-27T21:36:55Z","receivedAt":"2024-10-27T21:37:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 27.10.24 um 18:27 schrieb Patrick Steinhardt:\n> On Sun, Oct 27, 2024 at 05:31:00PM +0100, Johannes Sixt wrote:\n>> Am 27.10.24 um 16:38 schrieb Patrick Steinhardt:\n>>> On Sun, Oct 27, 2024 at 02:23:28PM +0100, Johannes Sixt wrote:\n>>>> Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:\n>>>>> Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag\n>>>>> that allows us to fix this usecase [1]. When set, it is possible to\n>>>>> rename a file over a preexisting file even when the target file still\n>>>>> has handles open. Those handles must have been opened with the\n>>>>> `FILE_SHARE_DELETE` flag, which we have ensured in the preceding\n>>>>> commits.\n>>>>>> Careful readers might have noticed that [1] does not mention the above\n>>>>> flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\n>>>>> not for use with `SetFileInformationByHandle()` though, which is what we\n>>>>> use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\n>>>>> not documented on [2] or anywhere else as far as I can tell.\n>>>>\n>>>> The Windows 10 SDK defines FILE_RENAME_FLAG_REPLACE_IF_EXISTS and\n>>>> FILE_RENAME_FLAG_POSIX_SEMANTICS for SetFileInformationByHandle(). That\n>>>> the documentation lacks \"_FLAG_\" in the names must be an error in the\n>>>> documentation.\n>>>>\n>>>> I found the mention of FILE_RENAME_POSIX_SEMANTICS quite distracting,\n>>>> because it is a flag to be used with CreateFileW() and basically only\n>>>> has to do with case-sensitivity, but nothing with POSIX semantics of\n>>>> renaming.\n>>>\n>>> I'd still prefer to mention this, because otherwise an astute reader\n>>> might notice that I'm using a different flag name than what is\n>>> documented in the docs and figure out that I defined the wrong flag\n>>> name.\n>>\n>> Ah, I was confused twice here. First, the documentation that you cite[*]\n>> mentions FILE_RENAME_POSIX_SEMANTICS, but the name does not exist at\n>> all. There does exist FILE_RENAME_FLAG_POSIX_SEMANTICS. The\n>> documentation is just wrong. And in my earlier comment I copied the\n>> inexistent flag name.\n>>\n>> But I meant to cite this flag: FILE_FLAG_POSIX_SEMANTICS (no \"RENAME\").\n>> It exists and is for CreateFileW().\n>>\n>> Perhaps you also meant cite the latter one as the flag that \"is not for\n>> use with `SetFileInformationByHandle()`\"?\n>>\n>> At any rate, the paragraph as written isn't correct.\n> \n> I think I'm missing something. That's what the paragraph says:\n> \n> Careful readers might have noticed that [1] does not mention the above\n> flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\n> not for use with `SetFileInformationByHandle()` though, which is what we\n> use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\n> not documented on [2] or anywhere else as far as I can tell.\n> \n> And I'd claim it is correct.\n> \n> `FILE_RENAME_POSIX_SEMANTICS` exists, this it is not a documentation\n> error. It is at a lower level than `FILE_RENAME_FLAG_POSIX_SEMANTICS`,\n> the documentation at [1] refers to \"ntifs.h\", which is part of the\n> Windows Driver Kit interfaces. So it is not supposed to be used with\n> `SetFileInformationByHandle()`, but with `FtlSetInformationFile()` [2],\n> which _also_ has a separate `FILE_RENAME_INFO` structure that looks the\n> same as `FILE_RENAME_INFO` defined for `SetFileInformationByHandle()`.\n> The only difference as far as I can tell is that the flags used for\n> these structures have slightly different names.\n> \n> Now I totally get your confusion -- I have been extremely confused by\n> all of this, as well. It certainly is a documentation error that the\n> respective `FILE_RENAME_FLAG_POSIX_SEMANTICS` is undocumented, but\n> having proper docs for this is rather important such that the reader\n> knows what its behaviour is. So I have no other choice than to link to\n> the ntifs interfaces, as it documents the actual behaviour, even though\n> it lives in a different part of the Windows APIs.\n> > Patrick\n> \n> [1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information\n> [2]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/fltkernel/nf-fltkernel-fltsetinformationfile\n\nOK, then let's leave the text as it is.\n\nThanks,\n-- Hannes\n\n"},{"id":"506172","messageId":"Zx7Q8L6/cPId1ZMu@nand.local","threadId":"62397","inReplyTo":"bc0c9853-7dd4-4b1d-9985-25f75815b0a9@kdbg.org","subject":"Re: [PATCH 1/3] compat/mingw: share file handles created via `CreateFileW()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-27T23:46:56Z","receivedAt":"2024-10-27T23:46:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Oct 27, 2024 at 02:14:30PM +0100, Johannes Sixt wrote:\n> Am 23.10.24 um 19:23 schrieb Taylor Blau:\n> > On Wed, Oct 23, 2024 at 05:04:58PM +0200, Patrick Steinhardt wrote:\n> >> Unless told otherwise, Windows will keep other processes from reading,\n> >> writing and deleting files when one has an open handle that was created\n> >> via `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*`\n> >> flags such that other processes _can_ read and/or modify such a file.\n> >> This sharing mechanism is quite important in the context of Git, as we\n> >> assume POSIX semantics all over the place.\n> >>\n> >> There are two calls where we don't set up those flags though:\n> >>\n> >> - We don't set `FILE_SHARE_DELETE` when creating a file for appending\n> >> via `mingw_open_append()`. This makes it impossible to delete the\n> >> file from another process or to replace it via an atomic rename.\n> >>\n> >> - When opening a file such that we can change its access/modification\n> >> times. This makes it impossible to perform any kind of operation\n> >> on this file at all from another process. While we only open the\n> >> file for a short amount of time to update its timestamps, this still\n> >> opens us up for a race condition with another process.\n> >>\n> >> Adapt both of these callsites to pass all three sharing flags.\n> >\n> > Interesting, and especially so noting that we *do* call CreateFileW()\n> > with the FILE_SHARE_DELETE flag in other functions like create_watch(),\n> > mingw_open_existing(), mingw_getcwd(), etc.\n> >\n> > Why didn't we call CreateFileW() with FILE_SHARE_DELETE in the below two\n> > functions? I assume Johannes Schindelin and/or Johannes Sixt (both CC'd)\n> > would know the answer. Regardless, it would be interesting and useful\n> > (IMHO) to include such a detail in the commit message.\n>\n> My attitude in the past was that deleting a file that is open elsewhere\n> is a bug, so FILE_SHARE_DELETE would not be needed, but its absence\n> could point to a bug elsewhere. Now that we have a reftable\n> implementation, it looks like I can't uphold this attitude anymore.\n\nMakes sense.\n\nThanks,\nTaylor\n"},{"id":"506173","messageId":"Zx7RU6sgIJjNSutE@nand.local","threadId":"62397","inReplyTo":"Zx5eipv_Un3x1j-4@pks.im","subject":"Re: [PATCH v2 2/3] compat/mingw: allow deletion of most opened files","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-27T23:48:35Z","receivedAt":"2024-10-27T23:48:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Oct 27, 2024 at 04:38:50PM +0100, Patrick Steinhardt wrote:\n> > > +\tint fd;\n> > > +\n> > > +\t/* We only support basic flags. */\n> > > +\tif (oflags & ~(O_ACCMODE | O_NOINHERIT)) {\n> > > +\t\terrno = ENOSYS;\n> > > +\t\treturn -1;\n> > > +\t}\n> > > +\n> > > +\tif (oflags & O_RDWR)\n> > > +\t\taccess = GENERIC_READ | GENERIC_WRITE;\n> > > +\telse if (oflags & O_WRONLY)\n> > > +\t\taccess = GENERIC_WRITE;\n> > > +\telse\n> > > +\t\taccess = GENERIC_READ;\n> >\n> > O_RDWR, O_WRONLY and O_RDONLY are not flags, but values occupying two\n> > bits of oflags. This must be:\n> >\n> > \tif ((oflags & O_ACCMODE) == O_RDWR)\n> > \t\taccess = GENERIC_READ | GENERIC_WRITE;\n> > \telse if ((oflags & O_ACCMODE) == O_WRONLY)\n> > \t\taccess = GENERIC_WRITE;\n> > \telse\n> > \t\taccess = GENERIC_READ;\n> >\n> > or similar.\n>\n> Ah, that makes sense indeed. Will fix.\n\nIt may be nice to write this as a switch statement, since we're always\ncomparing the value of oflags & O_ACCMODE, like so:\n\n switch (oflags & O_ACCMODE) {\n case O_RDWR:\n access = GENERIC_READ | GENERIC_WRITE;\n break;\n case O_WRONLY:\n access = GENERIC_WRITE;\n break;\n default:\n access = GENERIC_READ;\n break;\n }\n\n, but it is a minor point and I certainly do not have very strong\nfeelings here.\n\nThanks,\nTaylor\n"},{"id":"506174","messageId":"Zx7Rxz0ZG+Gqwf03@nand.local","threadId":"62397","inReplyTo":"c044ea3c-2f8c-45c1-99cf-17d9dad5b55f@kdbg.org","subject":"Re: [PATCH v2 3/3] compat/mingw: support POSIX semantics for atomic renames","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-27T23:50:31Z","receivedAt":"2024-10-27T23:50:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Oct 27, 2024 at 10:36:55PM +0100, Johannes Sixt wrote:\n> >> Ah, I was confused twice here. First, the documentation that you cite[*]\n> >> mentions FILE_RENAME_POSIX_SEMANTICS, but the name does not exist at\n> >> all. There does exist FILE_RENAME_FLAG_POSIX_SEMANTICS. The\n> >> documentation is just wrong. And in my earlier comment I copied the\n> >> inexistent flag name.\n> >>\n> >> But I meant to cite this flag: FILE_FLAG_POSIX_SEMANTICS (no \"RENAME\").\n> >> It exists and is for CreateFileW().\n> >>\n> >> Perhaps you also meant cite the latter one as the flag that \"is not for\n> >> use with `SetFileInformationByHandle()`\"?\n> >>\n> >> At any rate, the paragraph as written isn't correct.\n> >\n> > I think I'm missing something. That's what the paragraph says:\n> >\n> > Careful readers might have noticed that [1] does not mention the above\n> > flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is\n> > not for use with `SetFileInformationByHandle()` though, which is what we\n> > use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is\n> > not documented on [2] or anywhere else as far as I can tell.\n> >\n> > And I'd claim it is correct.\n> >\n> > [snip]\n> >\n> > [1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information\n> > [2]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/fltkernel/nf-fltkernel-fltsetinformationfile\n>\n> OK, then let's leave the text as it is.\n\nThanks, both, for vetting it carefully.\n\nThanks,\nTaylor\n"},{"id":"506175","messageId":"Zx7R7R2xDXX0m0kU@nand.local","threadId":"62397","inReplyTo":"Zx7RU6sgIJjNSutE@nand.local","subject":"Re: [PATCH v2 2/3] compat/mingw: allow deletion of most opened files","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-27T23:51:09Z","receivedAt":"2024-10-27T23:51:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Oct 27, 2024 at 07:48:35PM -0400, Taylor Blau wrote:\n> It may be nice to write this as a switch statement, since we're always\n> comparing the value of oflags & O_ACCMODE, like so:\n>\n> switch (oflags & O_ACCMODE) {\n> case O_RDWR:\n> access = GENERIC_READ | GENERIC_WRITE;\n> break;\n> case O_WRONLY:\n> access = GENERIC_WRITE;\n> break;\n> default:\n> access = GENERIC_READ;\n> break;\n> }\n>\n> , but it is a minor point and I certainly do not have very strong\n> feelings here.\n\n...and I see that's exactly what you went with in the subsequent round\n;-).\n\nThanks,\nTaylor\n"},{"id":"506698","messageId":"xmqqwmhhow3j.fsf@gitster.g","threadId":"62397","inReplyTo":"cover.1730042775.git.ps@pks.im","subject":"Re: [PATCH v3 0/3] compat/mingw: implement POSIX-style atomic renames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-06T03:54:40Z","receivedAt":"2024-11-06T03:54:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> this is third version of my patch series that introduces POSIX-style\n> atomic renames on Windows to fix concurrent writes with the reftable\n> backend.\n>\n> Changes compared to v2:\n>\n> - Use `DWORD` instead of `int` to store the `access` flags.\n>\n> - Fix handling of `oflags`.\n>\n> - Add a comment referring to `mingw_open_append()` for why convert\n> `ERROR_INVALID_PARAMETER` to `ERROR_PATH_NOT_FOUND`.\n>\n> Thanks!\n>\n> Patrick\n\nThanks, Patrick, for listing those who gave reviews on the previous\niterations of these patches.\n\nReviewers, how does this round look to you? Is this a version you\nfolks are comfortable to have in 'next' and later in 'master'?\n\nThanks.\n"},{"id":"506702","messageId":"1483501d-d798-46d6-93f5-f5edaf915e99@kdbg.org","threadId":"62397","inReplyTo":"xmqqwmhhow3j.fsf@gitster.g","subject":"Re: [PATCH v3 0/3] compat/mingw: implement POSIX-style atomic renames","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-11-06T06:44:11Z","receivedAt":"2024-11-06T06:44:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.11.24 um 04:54 schrieb Junio C Hamano:\n> Reviewers, how does this round look to you? Is this a version you\n> folks are comfortable to have in 'next' and later in 'master'?\n\nThis round looks fine!\n\nReviewed-by: Johannes Sixt \n\n-- Hannes\n\n"},{"id":"506712","messageId":"xmqqmsicpnrq.fsf@gitster.g","threadId":"62397","inReplyTo":"1483501d-d798-46d6-93f5-f5edaf915e99@kdbg.org","subject":"Re: [PATCH v3 0/3] compat/mingw: implement POSIX-style atomic renames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-06T12:09:13Z","receivedAt":"2024-11-06T12:09:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 06.11.24 um 04:54 schrieb Junio C Hamano:\n>> Reviewers, how does this round look to you? Is this a version you\n>> folks are comfortable to have in 'next' and later in 'master'?\n>\n> This round looks fine!\n>\n> Reviewed-by: Johannes Sixt \n\nThanks.\n"}]}