Am 24.10.24 um 13:46 schrieb Patrick Steinhardt:
> 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.
Agreed!
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> compat/mingw.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index e326c6fcd2d..6c75fe36b15 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -532,6 +532,62 @@ 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;
> + int access;
I would have made this variable DWORD or unsigned instead of plain int,
because it receives a bit pattern and would match the parameter type of
CreateFileW() better; but no big deal.
> + int fd;
> +
> + /* We only support basic flags. */
> + if (oflags & ~(O_ACCMODE | O_NOINHERIT)) {
> + errno = ENOSYS;
> + return -1;
> + }
> +
> + if (oflags & O_RDWR)
> + access = GENERIC_READ | GENERIC_WRITE;
> + else if (oflags & O_WRONLY)
> + access = GENERIC_WRITE;
> + else
> + access = GENERIC_READ;
O_RDWR, O_WRONLY and O_RDONLY are not flags, but values occupying two
bits of oflags. This must be:
if ((oflags & O_ACCMODE) == O_RDWR)
access = GENERIC_READ | GENERIC_WRITE;
else if ((oflags & O_ACCMODE) == O_WRONLY)
access = GENERIC_WRITE;
else
access = GENERIC_READ;
or similar.
> +
> + 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();
> + if (err == ERROR_INVALID_PARAMETER)
> + err = ERROR_PATH_NOT_FOUND;
First I wondered what this is about, but then noticed that it is just
copied from the mingw_open_append() implementation catering to some
bogus network filesystems. Good.
> + 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 +623,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;
>
Makes sense!
-- Hannes
