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
