Re: [PATCH 2/3] compat/mingw: allow deletion of most opened files

From: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Date: Oct 23, 2024, 16:17 UTC
Message-ID: <bc829d84-6696-4e16-9687-8ba611af29ff@app.fastmail.com>
In-Reply-To: <3552feddb332b31744c2ab723b112b9b08926436.1729695349.git.ps@pks.im>

On Wed, Oct 23, 2024, at 17:05, Patrick Steinhardt wrote:

> On Windows, we emulate open(3p) via `mingw_open()`. This function
> implements handling of some platform- specific quirks that are required

s/platform- specific/platform-specific/

Linewrapping artifact?

Show 16 quoted lines

> 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`,

s/O_CREAT/O_CREATE/ ?

Show 37 quoted lines

> 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 <ps@pks.im>
> ---
>  compat/mingw.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index e326c6fcd2d..ce95f3a5968 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -532,6 +532,59 @@ 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.
> + */

This is above my technical level but the comment reads nicely.

Show 5 quoted lines

> +static int mingw_open_existing(const wchar_t *filename, int oflags, ...)
> +{
> +	SECURITY_ATTRIBUTES security_attributes = {
> +	    .nLength = sizeof(security_attributes),
> +	    .bInheritHandle = !(oflags & O_NOINHERIT),

`clang-format` thinks that these two lines should be indented with tabs instead (so two tabs in total).

(Ubuntu clang-format version 14.0.0-1ubuntu1.1)

Show 8 quoted lines

> +	};
> +	HANDLE handle;
> +	int access;
> +	int fd;
> +
> +	/* We only support basic flags. */
> +	if (oflags & ~(O_ACCMODE | O_NOINHERIT))
> +		return errno = ENOSYS, -1;

This use of the comma operator is maybe an idiom to save space and avoid a brace around the `if`? This pattern is already in use in `mingw_open_append`. I see in `mingw.h` that it uses:

``` static inline int symlink(const char *oldpath UNUSED, const char *newpath UNUSED) { errno = ENOSYS; return -1; } ```

Show 10 quoted lines

> +	if (oflags & O_RDWR)
> +		access = GENERIC_READ | GENERIC_WRITE;
> +	else if (oflags & O_WRONLY)
> +		access = GENERIC_WRITE;
> +	else
> +		access = GENERIC_READ;
> +
> +	handle = CreateFileW(filename, access,
> +			     FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE,
> +			     &security_attributes, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

`clang-format` says that these two lines are too long.

Show 6 quoted lines

> +	if (handle == INVALID_HANDLE_VALUE) {
> +		DWORD err = GetLastError();
> +		if (err == ERROR_INVALID_PARAMETER)
> +			err = ERROR_PATH_NOT_FOUND;
> +		errno = err_win_to_posix(err);
> +		return -1;

Here you don’t use the comma operator, presumably because it wouldn’t turn the `if` into a one-statement block.

Show 22 quoted lines

> +	}
> +
> +	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 +620,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;
>
> --
> 2.47.0.118.gfd3785337b.dirty