From patchwork Sun Oct 27 15:39:37 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 13852697 Received: from fhigh-b6-smtp.messagingengine.com (fhigh-b6-smtp.messagingengine.com [202.12.124.157]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 90A402570 for ; Sun, 27 Oct 2024 15:39:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=202.12.124.157 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730043560; cv=none; b=OG44ZIhNiORuBafomMjnmjhCF5lfeq6sDnnJjREJ8KTN3T3zvSrPXTgHnakkMfXb5p6SiIIEu24zsfERQYnCSboD0rlnpJDbkBmxrkJLf4qWoJgUoHE0jYVpgE9GIjNw64cFnPzASd3+g2ZsQ5EPN8GhmCsPnYyVn82toJ3xefs= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730043560; c=relaxed/simple; bh=YqXW1DFZbq/8XaUQc83doprlyCXcqO0C2mKsYzs3Fh0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=fjeRlzMyvNjCkDlk0ows4xYDgcD+8yXihtd3BLXYf0qa1ME/FjBu9fNrl2jcpparhewCMp7FtyPtVsy/hdt+LNIGTdBO54TrPG6lupLKdBn6Q6PLEe5i6recDJKoaZHXmfL6GVdcID5DAq7o/OelvYALtRJYyuEYn3vuATvLoao= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im; spf=pass smtp.mailfrom=pks.im; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b=fXOwiBvj; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=VAPy/xst; arc=none smtp.client-ip=202.12.124.157 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pks.im Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b="fXOwiBvj"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="VAPy/xst" Received: from phl-compute-07.internal (phl-compute-07.phl.internal [10.202.2.47]) by mailfhigh.stl.internal (Postfix) with ESMTP id 625E425400D7; Sun, 27 Oct 2024 11:39:17 -0400 (EDT) Received: from phl-mailfrontend-02 ([10.202.2.163]) by phl-compute-07.internal (MEProxy); Sun, 27 Oct 2024 11:39:17 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm3; t=1730043557; x=1730129957; bh=MTRkXvw6t0 FSqOMZZJW27DpDqfh6nWVWSPW17/JpGEk=; b=fXOwiBvjWEZy4OPZsKDj9AGpqg RtVSJ8nQ+CvVNoMn+DGeGtnF22TEQTzw5m6UQDn2bxs6QQwp3uIKPv7IoAMBlW03 LKzPSm/J8wNfc5vGI5R4R+cE7bhOY4czaV1rYHdaD9AF3QYzfIH/weF8+FGHzlJa gSDy5rTlcXM25j4IEvjrmKVcWeUrZiXFhlKQE2G6+vMLL2YJncVnpN3BeM26ja71 3vaZJQpRHJOiTOSP8roryXg3zToNzKTk2wpsX5UcMQpZwnM9szsiLaAfrA5iUzwO nGG6oTmDykXqXcv6Q9dA1fxlgxZ8R9v4ljE1umBoTpb9kYbIH5iTgr/yLEMA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1730043557; x=1730129957; bh=MTRkXvw6t0FSqOMZZJW27DpDqfh6 nWVWSPW17/JpGEk=; b=VAPy/xstPTQIIDUtahFL0EsGr+O13RAWxH+cW/Jdbe2y uyKCH+7exsgQ6WoVEFG2ZHyFMcEzR0Dsqarvc/XK80mlOiRNqJ3AybAMQTYxuBac 7Jop5SGSzQHBhes5hdcDIJyZazjSLdHNX6I2xVOgyYBrG02hG8/UEu0KO0mJv+Pn 9OgOXsS9YXjZTa0veVeu4BxqS7g5bcs2e7/RYmGma+fxJrceBYjCFe/wHLHgzj6s DlQMTL6Iw9McjJo64pQqih+p6FFRzPi22DHbDRj/dfOhmJ1NJs7RntNjR/t7PJXp ineFw3cxMZHY7Y7J0x8KAfcBXslNVl8CEjPXbWhNYw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeftddrvdejiedgjeelucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnecujfgurhepfffhvfevuffkfhggtggujgesthdtredttddtvden ucfhrhhomheprfgrthhrihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimh eqnecuggftrfgrthhtvghrnhepveekkeffhfeitdeludeigfejtdetvdelvdduhefgueeg udfghfeukefhjedvkedtnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrg hilhhfrhhomhepphhssehpkhhsrdhimhdpnhgspghrtghpthhtohephedpmhhouggvpehs mhhtphhouhhtpdhrtghpthhtohepmhgvsehtthgrhihlohhrrhdrtghomhdprhgtphhtth hopehjohhhrghnnhgvshdrshgthhhinhguvghlihhnsehgmhigrdguvgdprhgtphhtthho pehjiehtsehkuggsghdrohhrghdprhgtphhtthhopehkrhhishhtohhffhgvrhhhrghugh hssggrkhhksehfrghsthhmrghilhdrtghomhdprhgtphhtthhopehgihhtsehvghgvrhdr khgvrhhnvghlrdhorhhg X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 27 Oct 2024 11:39:15 -0400 (EDT) Received: by vm-mail (OpenSMTPD) with ESMTPSA id 5054d85f (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 27 Oct 2024 15:39:11 +0000 (UTC) Date: Sun, 27 Oct 2024 16:39:37 +0100 From: Patrick Steinhardt To: git@vger.kernel.org Cc: Johannes Schindelin , Taylor Blau , Kristoffer Haugsbakk , Johannes Sixt Subject: [PATCH v3 1/3] compat/mingw: share file handles created via `CreateFileW()` Message-ID: <63c2cfa87a4a3c44367acdc1f7cdb57289b4b761.1730042775.git.ps@pks.im> References: Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-TUID: eEfurap1rNPL Unless told otherwise, Windows will keep other processes from reading, writing and deleting files when one has an open handle that was created via `CreateFileW()`. This behaviour can be altered via `FILE_SHARE_*` flags: - `FILE_SHARE_READ` allows a concurrent process to open the file for reading. - `FILE_SHARE_WRITE` allows a concurrent process to open the file for writing. - `FILE_SHARE_DELETE` allows a concurrent process to delete the file or to replace it via an atomic rename. This sharing mechanism is quite important in the context of Git, as we assume POSIX semantics all over the place. But there are two callsites where we don't pass all three of these flags: - We don't set `FILE_SHARE_DELETE` when creating a file for appending via `mingw_open_append()`. This makes it impossible to delete the file from another process or to replace it via an atomic rename. The function was introduced via d641097589 (mingw: enable atomic O_APPEND, 2018-08-13) and has been using `FILE_SHARE_READ | FILE_SHARE_WRITE` since the inception. There aren't any indicators that the omission of `FILE_SHARE_DELETE` was intentional. - We don't set any sharing flags in `mingw_utime()`, which changes the access and modification of a file. This makes it impossible to perform any kind of operation on this file at all from another process. While we only open the file for a short amount of time to update its timestamps, this still opens us up for a race condition with another process. `mingw_utime()` was originally implemented via `_wopen()`, which doesn't give you full control over the sharing mode. Instead, it calls `_wsopen()` with `_SH_DENYNO`, which ultimately translates to `FILE_SHARE_READ | FILE_SHARE_WRITE`. It was then refactored via 090a3085bc (t/helper/test-chmtime: update mingw to support chmtime on directories, 2022-03-02) to use `CreateFileW()`, but we stopped setting any sharing flags at all, which seems like an unintentional side effect. By restoring `FILE_SHARE_READ | FILE_SHARE_WRITE` we thus fix this and get back the old behaviour of `_wopen()`. The fact that we didn't set the equivalent of `FILE_SHARE_DELETE` can be explained, as well: neither `_wopen()` nor `_wsopen()` allow you to do so. So overall, it doesn't seem intentional that we didn't allow deletions here, either. Adapt both of these callsites to pass all three sharing flags. Signed-off-by: Patrick Steinhardt --- compat/mingw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index 0e851ecae29..e326c6fcd2d 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -502,7 +502,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) * to append to the file. */ handle = CreateFileW(wfilename, FILE_APPEND_DATA, - FILE_SHARE_WRITE | FILE_SHARE_READ, + FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE, NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); if (handle == INVALID_HANDLE_VALUE) { DWORD err = GetLastError(); @@ -1006,7 +1006,7 @@ int mingw_utime (const char *file_name, const struct utimbuf *times) osfilehandle = CreateFileW(wfilename, FILE_WRITE_ATTRIBUTES, - 0 /*FileShare.None*/, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, OPEN_EXISTING, (attrs != INVALID_FILE_ATTRIBUTES && From patchwork Sun Oct 27 15:39:40 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 13852698 Received: from fhigh-b6-smtp.messagingengine.com (fhigh-b6-smtp.messagingengine.com [202.12.124.157]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1EB7F17798F for ; Sun, 27 Oct 2024 15:39:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=202.12.124.157 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730043564; cv=none; b=tKI/v/X/uno2XNXxTpbaeqZLzC3Trf3ekpoPYxuQzopQct4mml0+iySAF5tNoBuhTC+1hFnx2oW8wnD2aS5A7NJfO0QnufSCYnM2zarpNJNP8IrZqZxauKP7GZLYcCWGY34FhXVZRneT6Tv+pL3x0przsLyoJDkb6r3kKLZVmSY= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730043564; c=relaxed/simple; bh=bebsAzHb5qzC4brNAVmxVTg+CQAn7mKgBbjaeiuP12c=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ksNEpo4KhGVu9GvntyNOVug6+jUukoxE9sj06Bn8R6WZaBYxa8Ebe36zG8KkQwWjoCDobPKdTsfmD8pjqgFwunn4M/K010eulMap74WQmDmlrRBB3FSxBHSMXIfxlCd5WDw+e86LK8tbIKqtoRMLYGXMxVqik877Yzn5Xzi2gs4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im; spf=pass smtp.mailfrom=pks.im; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b=edqi7XB5; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=fLPZGm4X; arc=none smtp.client-ip=202.12.124.157 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pks.im Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b="edqi7XB5"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="fLPZGm4X" Received: from phl-compute-01.internal (phl-compute-01.phl.internal [10.202.2.41]) by mailfhigh.stl.internal (Postfix) with ESMTP id 401AD25400AB; Sun, 27 Oct 2024 11:39:21 -0400 (EDT) Received: from phl-mailfrontend-02 ([10.202.2.163]) by phl-compute-01.internal (MEProxy); Sun, 27 Oct 2024 11:39:21 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm3; t=1730043561; x=1730129961; bh=7Lc3bD0flU 4mOEXuSAZqpifraRo+wtlpPLkBbsqoFN8=; b=edqi7XB58Z/riIaNpY9sxFECXi hrSJbzuBUNqiT9OHyKV9ZhWk6Oo8G/KPOdhbcN1xHyH7B2WjkXX6dIxWi17hllfH vSelr/QUc8/9qJaT3VbWfj7aK8wzJcy06hoJNE7Nb5A3XgLXjcRceJXBXvJIFzae Js0LXhL1nDcQG2n19Vf6hoz21UQgt+t4MLSGutBTOYaiEOeQN95m+mUBqymNiZnI 8DtiWvBSSLnU43jS/GIHCYkN5AnnuWVvTfNB/FLg+qlifYouJccROw9o9Sn6STRT RCKSmyfvNLUjj/S0YTGvyZ8v9vlQsrLlZVQptXSWMePkFK4FBU3Zd/tjFQtg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1730043561; x=1730129961; bh=7Lc3bD0flU4mOEXuSAZqpifraRo+ wtlpPLkBbsqoFN8=; b=fLPZGm4XwYBpNUf4Dn4H5Zu6K3/ecAfi8DbI5xQp4b/O 24Qft/0YfNfI8qhXXUjjSm0fwbyLAE6j4PFHR3PMdqODptfjQ+bSIdWH9iNhQJWo eZ0ihLclPGzmQ0Ittcrh+nj0wV+OdtAtcfbf5tsRk0xuN3Rlehno18HQ61l2Qvpl Pzt86/3RF4cBgXzdfJVU+Re4TIMWAPMIogyVozHXxB7jAhtdpz22avtn4jHLUGKy TLKF50td0zd8xGvvA74UsobKw0MnBrSDjykrsigz0oOJBZoAe0s655VwOqCrGXwK QcuB0CNn/jBYSAgr2I26sJ+G1BEwXZv1qlWNiD7QpQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeftddrvdejiedgkedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnecujfgurhepfffhvfevuffkfhggtggujgesthdtredttddtvden ucfhrhhomheprfgrthhrihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimh eqnecuggftrfgrthhtvghrnhepveekkeffhfeitdeludeigfejtdetvdelvdduhefgueeg udfghfeukefhjedvkedtnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrg hilhhfrhhomhepphhssehpkhhsrdhimhdpnhgspghrtghpthhtohephedpmhhouggvpehs mhhtphhouhhtpdhrtghpthhtohepjheitheskhgusghgrdhorhhgpdhrtghpthhtohepkh hrihhsthhofhhfvghrhhgruhhgshgsrghkkhesfhgrshhtmhgrihhlrdgtohhmpdhrtghp thhtohepghhithesvhhgvghrrdhkvghrnhgvlhdrohhrghdprhgtphhtthhopehjohhhrg hnnhgvshdrshgthhhinhguvghlihhnsehgmhigrdguvgdprhgtphhtthhopehmvgesthht rgihlhhorhhrrdgtohhm X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 27 Oct 2024 11:39:19 -0400 (EDT) Received: by vm-mail (OpenSMTPD) with ESMTPSA id f6792dde (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 27 Oct 2024 15:39:15 +0000 (UTC) Date: Sun, 27 Oct 2024 16:39:40 +0100 From: Patrick Steinhardt To: git@vger.kernel.org Cc: Johannes Schindelin , Taylor Blau , Kristoffer Haugsbakk , Johannes Sixt Subject: [PATCH v3 2/3] compat/mingw: allow deletion of most opened files Message-ID: <91d434116f3eec4444316804dfb3d955c9750a6b.1730042775.git.ps@pks.im> References: Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-TUID: vI8jGBB/8bs6 On Windows, we emulate open(3p) via `mingw_open()`. This function implements handling of some platform-specific quirks that are required to make it behave as closely as possible like open(3p) would, but for most cases we just call the Windows-specific `_wopen()` function. This function has a major downside though: it does not allow us to specify the sharing mode. While there is `_wsopen()` that allows us to pass sharing flags, those sharing flags are not the same `FILE_SHARE_*` flags as `CreateFileW()` accepts. Instead, `_wsopen()` only allows concurrent read- and write-access, but does not allow for concurrent deletions. Unfortunately though, we have to allow concurrent deletions if we want to have POSIX-style atomic renames on top of an existing file that has open file handles. Implement a new function that emulates open(3p) for existing files via `CreateFileW()` such that we can set the required sharing flags. While we have the same issue when calling open(3p) with `O_CREAT`, implementing that mode would be more complex due to the required permission handling. Furthermore, atomic updates via renames typically write to exclusive lockfile and then perform the rename, and thus we don't have to handle the case where the locked path has been created with `O_CREATE`. So while it would be nice to have proper POSIX semantics in all paths, we instead aim for a minimum viable fix here. Signed-off-by: Patrick Steinhardt --- compat/mingw.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/compat/mingw.c b/compat/mingw.c index e326c6fcd2d..0d9600543cb 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -532,6 +532,70 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) return fd; } +/* + * Ideally, we'd use `_wopen()` to implement this functionality so that we + * don't have to reimplement it, but unfortunately we do not have tight control + * over the share mode there. And while `_wsopen()` and friends exist that give + * us _some_ control over the share mode, this family of functions doesn't give + * us the ability to enable FILE_SHARE_DELETE, either. But this is a strict + * requirement for us though so that we can unlink or rename over files that + * are held open by another process. + * + * We are thus forced to implement our own emulation of `open()`. To make our + * life simpler we only implement basic support for this, namely opening + * existing files for reading and/or writing. This means that newly created + * files won't have their sharing mode set up correctly, but for now I couldn't + * find any case where this matters. We may have to revisit that in the future + * though based on our needs. + */ +static int mingw_open_existing(const wchar_t *filename, int oflags, ...) +{ + SECURITY_ATTRIBUTES security_attributes = { + .nLength = sizeof(security_attributes), + .bInheritHandle = !(oflags & O_NOINHERIT), + }; + HANDLE handle; + DWORD access; + int fd; + + /* We only support basic flags. */ + if (oflags & ~(O_ACCMODE | O_NOINHERIT)) { + errno = ENOSYS; + return -1; + } + + switch (oflags & O_ACCMODE) { + case O_RDWR: + access = GENERIC_READ | GENERIC_WRITE; + break; + case O_WRONLY: + access = GENERIC_WRITE; + break; + default: + access = GENERIC_READ; + break; + } + + handle = CreateFileW(filename, access, + FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE, + &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); + if (handle == INVALID_HANDLE_VALUE) { + DWORD err = GetLastError(); + + /* See `mingw_open_append()` for why we have this conversion. */ + if (err == ERROR_INVALID_PARAMETER) + err = ERROR_PATH_NOT_FOUND; + + errno = err_win_to_posix(err); + return -1; + } + + fd = _open_osfhandle((intptr_t)handle, oflags | O_BINARY); + if (fd < 0) + CloseHandle(handle); + return fd; +} + /* * Does the pathname map to the local named pipe filesystem? * That is, does it have a "//./pipe/" prefix? @@ -567,6 +631,8 @@ int mingw_open (const char *filename, int oflags, ...) if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename)) open_fn = mingw_open_append; + else if (!(oflags & ~(O_ACCMODE | O_NOINHERIT))) + open_fn = mingw_open_existing; else open_fn = _wopen; From patchwork Sun Oct 27 15:39:43 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 13852699 Received: from fhigh-b6-smtp.messagingengine.com (fhigh-b6-smtp.messagingengine.com [202.12.124.157]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0915D1714A0 for ; Sun, 27 Oct 2024 15:39:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=202.12.124.157 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730043567; cv=none; b=YB0if5E/D5oe2upYxU31+PVzOvDICxLvkhDKzUPnmybRMQNTjGkXqsqL5xc7re0IpSJkN7tcE9i94Cd5hxNhWUkBtL9VN/5scK98FjUoy1MuWnLKHDAdSBRpP2afUA4e8QW1pt4KEIMg/8g9xc8FcTHnXK6cg8J0szw4CzQNPRM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730043567; c=relaxed/simple; bh=fW8TzdqCozzMrwAnBzNHmbmS5S72+f4rfuF+XB0o2Fc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=kR50/pyIAfdAdofTwx4iH0J/OfrCP/K+7aL8sayv/nSvLgtAjklG1CcYKscCpyk8ZIylhf4ldfo9PRje+tKlL8D5u2r9JoexBuHo9vteAnKlg4SjnRfaeoiJtVZzUmdpppfj4VPe/hlCXvGCYKCwL+o34kz7AG+1zIGcy6PLBgQ= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im; spf=pass smtp.mailfrom=pks.im; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b=wLaD6z27; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=Z6CfD2xD; arc=none smtp.client-ip=202.12.124.157 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pks.im Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b="wLaD6z27"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="Z6CfD2xD" Received: from phl-compute-02.internal (phl-compute-02.phl.internal [10.202.2.42]) by mailfhigh.stl.internal (Postfix) with ESMTP id 1898725400CA; Sun, 27 Oct 2024 11:39:24 -0400 (EDT) Received: from phl-mailfrontend-02 ([10.202.2.163]) by phl-compute-02.internal (MEProxy); Sun, 27 Oct 2024 11:39:24 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm3; t=1730043563; x=1730129963; bh=3lq/fAD2A2 Bir0VqUARmjJCkaQhoU1Oczp1QdRonFAY=; b=wLaD6z27DQuJF12jNUzNIMxjN3 qNnON1n3Qe2somTpqlWxEIsJatdZ+x/jDpgahGlXOvIeELMstqEUYQfluKb4ib4P WqzQxrvZAHW43HtWZSQFXUv8GdASmTP8JKb27jA6lsYQZDqADkdUTEC1RzJ49yst Q+SnmN0UGfG3y6ypiwvA6JO4BOudCLPyijB/nUH++GMy/izSi+8y/Vdr9frRQ2/j 6J1vEcgCcIXXGAS+uzKvBjZm0+cuF1LUUX51KmeVVwgawpAh16FAcDMDPo/BPgvy 6tlToKwg18Z7sx2PS2GklMxIuuLwD0IxJQYQu0of/mC2AccYhJjms2p4Cdnw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1730043563; x=1730129963; bh=3lq/fAD2A2Bir0VqUARmjJCkaQho U1Oczp1QdRonFAY=; b=Z6CfD2xDohcZukO3teyxQ/V81RFqhG7yL7maP8MRXiyN +617UpIjtE1DecGLYPGuWrSnj7YTJkNn7u2G75XVAwFwH/PagHnSGpvcCgS5/ZHi vKWWVBzf2+f8lwXEAJAMh3ZVZHyWYJtt9+vR/Uw/cFdwxKVmniiMmdvm8LRtIyKC IJmvKexuyI8UNmPOoUG82/mtBbFGG9vaJJ9vB5+KpeF1bBAzDpZuBRc9A9pCkRq2 XiVL3AnAxdNI5YltzLifERXF4nUg2EihWxZWJKYZf7Ed0D52K6gZnuu3AWqZCelu SnQuNs7sehD/jluH3hTqP7/sghGlIwEhnOK8U5V64w== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeftddrvdejiedgjeelucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnecujfgurhepfffhvfevuffkfhggtggujgesthdtredttddtvden ucfhrhhomheprfgrthhrihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimh eqnecuggftrfgrthhtvghrnhepjeehffduffeufeeludelleeivedvffeludeuheeuuddt udelgfetgeffffdukeegnecuffhomhgrihhnpehmihgtrhhoshhofhhtrdgtohhmnecuve hluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkhhs rdhimhdpnhgspghrtghpthhtohephedpmhhouggvpehsmhhtphhouhhtpdhrtghpthhtoh epmhgvsehtthgrhihlohhrrhdrtghomhdprhgtphhtthhopehjohhhrghnnhgvshdrshgt hhhinhguvghlihhnsehgmhigrdguvgdprhgtphhtthhopehkrhhishhtohhffhgvrhhhrg hughhssggrkhhksehfrghsthhmrghilhdrtghomhdprhgtphhtthhopehgihhtsehvghgv rhdrkhgvrhhnvghlrdhorhhgpdhrtghpthhtohepjheitheskhgusghgrdhorhhg X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 27 Oct 2024 11:39:22 -0400 (EDT) Received: by vm-mail (OpenSMTPD) with ESMTPSA id c4d25518 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 27 Oct 2024 15:39:18 +0000 (UTC) Date: Sun, 27 Oct 2024 16:39:43 +0100 From: Patrick Steinhardt To: git@vger.kernel.org Cc: Johannes Schindelin , Taylor Blau , Kristoffer Haugsbakk , Johannes Sixt Subject: [PATCH v3 3/3] compat/mingw: support POSIX semantics for atomic renames Message-ID: <2447d332a851e41e613dec181c347d5a15c310ab.1730042775.git.ps@pks.im> References: Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-TUID: ibLLLeK8jhGQ By default, Windows restricts access to files when those files have been opened by another process. As explained in the preceding commits, these restrictions can be loosened such that reads, writes and/or deletes of files with open handles _are_ allowed. While we set up those sharing flags in most relevant code paths now, we still don't properly handle POSIX-style atomic renames in case the target path is open. This is failure demonstrated by t0610, where one of our tests spawns concurrent writes in a reftable-enabled repository and expects all of them to succeed. This test fails most of the time because the process that has acquired the "tables.list" lock is unable to rename it into place while other processes are busy reading that file. Windows 10 has introduced the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag that allows us to fix this usecase [1]. When set, it is possible to rename a file over a preexisting file even when the target file still has handles open. Those handles must have been opened with the `FILE_SHARE_DELETE` flag, which we have ensured in the preceding commits. Careful readers might have noticed that [1] does not mention the above flag, but instead mentions `FILE_RENAME_POSIX_SEMANTICS`. This flag is not for use with `SetFileInformationByHandle()` though, which is what we use. And while the `FILE_RENAME_FLAG_POSIX_SEMANTICS` flag exists, it is not documented on [2] or anywhere else as far as I can tell. Unfortunately, we still support Windows systems older than Windows 10 that do not yet have this new flag. Our `_WIN32_WINNT` SDK version still targets 0x0600, which is Windows Vista and later. And even though that Windows version is out-of-support, bumping the SDK version all the way to 0x0A00, which is Windows 10 and later, is not an option as it would make it impossible to compile on Windows 8.1, which is still supported. Instead, we have to manually declare the relevant infrastructure to make this feature available and have fallback logic in place in case we run on a Windows version that does not yet have this flag. On another note: `mingw_rename()` has a retry loop that is used in case deleting a file failed because it's still open in another process. One might be pressed to not use this loop anymore when we can use POSIX semantics. But unfortunately, we have to keep it around due to our dependence on the `FILE_SHARE_DELETE` flag. While we know to set that sharing flag now, other applications may not do so and may thus still cause sharing violations when we try to rename a file. This fixes concurrent writes in the reftable backend as demonstrated in t0610, but may also end up fixing other usecases where Git wants to perform renames. [1]: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information [2]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_rename_info Signed-off-by: Patrick Steinhardt --- compat/mingw.c | 87 ++++++++++++++++++++++++++++++++++++-- t/t0610-reftable-basics.sh | 8 ++-- 2 files changed, 88 insertions(+), 7 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index 0d9600543cb..c4320769db6 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2217,10 +2217,16 @@ int mingw_accept(int sockfd1, struct sockaddr *sa, socklen_t *sz) #undef rename int mingw_rename(const char *pold, const char *pnew) { + static int supports_file_rename_info_ex = 1; DWORD attrs, gle; int tries = 0; wchar_t wpold[MAX_PATH], wpnew[MAX_PATH]; - if (xutftowcs_path(wpold, pold) < 0 || xutftowcs_path(wpnew, pnew) < 0) + int wpnew_len; + + if (xutftowcs_path(wpold, pold) < 0) + return -1; + wpnew_len = xutftowcs_path(wpnew, pnew); + if (wpnew_len < 0) return -1; /* @@ -2231,11 +2237,84 @@ int mingw_rename(const char *pold, const char *pnew) return 0; if (errno != EEXIST) return -1; + repeat: - if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING)) - return 0; + if (supports_file_rename_info_ex) { + /* + * Our minimum required Windows version is still set to Windows + * Vista. We thus have to declare required infrastructure for + * FileRenameInfoEx ourselves until we bump _WIN32_WINNT to + * 0x0A00. Furthermore, we have to handle cases where the + * FileRenameInfoEx call isn't supported yet. + */ +#define FILE_RENAME_FLAG_REPLACE_IF_EXISTS 0x00000001 +#define FILE_RENAME_FLAG_POSIX_SEMANTICS 0x00000002 + FILE_INFO_BY_HANDLE_CLASS FileRenameInfoEx = 22; + struct { + /* + * This is usually an unnamed union, but that is not + * part of ISO C99. We thus inline the field, as we + * really only care for the Flags field anyway. + */ + DWORD Flags; + HANDLE RootDirectory; + DWORD FileNameLength; + /* + * The actual structure is defined with a single-character + * flex array so that the structure has to be allocated on + * the heap. As we declare this structure ourselves though + * we can avoid the allocation and define FileName to have + * MAX_PATH bytes. + */ + WCHAR FileName[MAX_PATH]; + } rename_info = { 0 }; + HANDLE old_handle = INVALID_HANDLE_VALUE; + BOOL success; + + old_handle = CreateFileW(wpold, DELETE, + FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE, + NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); + if (old_handle == INVALID_HANDLE_VALUE) { + errno = err_win_to_posix(GetLastError()); + return -1; + } + + rename_info.Flags = FILE_RENAME_FLAG_REPLACE_IF_EXISTS | + FILE_RENAME_FLAG_POSIX_SEMANTICS; + rename_info.FileNameLength = wpnew_len * sizeof(WCHAR); + memcpy(rename_info.FileName, wpnew, wpnew_len * sizeof(WCHAR)); + + success = SetFileInformationByHandle(old_handle, FileRenameInfoEx, + &rename_info, sizeof(rename_info)); + gle = GetLastError(); + CloseHandle(old_handle); + if (success) + return 0; + + /* + * When we see ERROR_INVALID_PARAMETER we can assume that the + * current system doesn't support FileRenameInfoEx. Keep us + * from using it in future calls and retry. + */ + if (gle == ERROR_INVALID_PARAMETER) { + supports_file_rename_info_ex = 0; + goto repeat; + } + + /* + * In theory, we shouldn't get ERROR_ACCESS_DENIED because we + * always open files with FILE_SHARE_DELETE But in practice we + * cannot assume that Git is the only one accessing files, and + * other applications may not set FILE_SHARE_DELETE. So we have + * to retry. + */ + } else { + if (MoveFileExW(wpold, wpnew, MOVEFILE_REPLACE_EXISTING)) + return 0; + gle = GetLastError(); + } + /* TODO: translate more errors */ - gle = GetLastError(); if (gle == ERROR_ACCESS_DENIED && (attrs = GetFileAttributesW(wpnew)) != INVALID_FILE_ATTRIBUTES) { if (attrs & FILE_ATTRIBUTE_DIRECTORY) { diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh index babec7993e3..eaf6fab6d29 100755 --- a/t/t0610-reftable-basics.sh +++ b/t/t0610-reftable-basics.sh @@ -450,10 +450,12 @@ test_expect_success 'ref transaction: retry acquiring tables.list lock' ' ) ' -# This test fails most of the time on Windows systems. The root cause is +# This test fails most of the time on Cygwin systems. The root cause is # that Windows does not allow us to rename the "tables.list.lock" file into -# place when "tables.list" is open for reading by a concurrent process. -test_expect_success !WINDOWS 'ref transaction: many concurrent writers' ' +# place when "tables.list" is open for reading by a concurrent process. We have +# worked around that in our MinGW-based rename emulation, but the Cygwin +# emulation seems to be insufficient. +test_expect_success !CYGWIN 'ref transaction: many concurrent writers' ' test_when_finished "rm -rf repo" && git init repo && (