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

[Patrick Steinhardt <ps@pks.im>]

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

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

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

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

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

Yup.

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

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

Patrick