Skip to content

windows: use permissive file share flags everywhere #19505

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 11, 2024

Conversation

gvilums
Copy link
Contributor

@gvilums gvilums commented Apr 2, 2024

On windows, the zig standard library generally uses pretty restrictive FILE_SHARE_* flags, even though there is no obvious reason to do so.

This PR changes all uses to the most permissive share flags, allowing other processes to read, write and delete files that are opened by the running process, and allowing the running process to open files that may have already been opened with more permissive sharing permissions. This is in line with POSIX behavior, and I see no reason not to allow this, as it is not required for correctness in any of the cases (from my understanding, please correct me if I'm wrong here).

The prompt for opening this PR was a set of issues we have been facing at bun: For example, deleting a directory that has been opened with fs.openDir would result in an error.Unexpected NTSTATUS=0xc0000043 (a STATUS_SHARING_VIOLATION).

For reference, note that the rust standard library uses the most permissive flags for all file opening operations: click, click

@Jarred-Sumner
Copy link
Contributor

Jarred-Sumner commented Apr 2, 2024

@jdalton
Copy link

jdalton commented Apr 2, 2024

We should add a trivial test to validate.

@@ -255,7 +255,7 @@ pub fn CreatePipe(rd: *HANDLE, wr: *HANDLE, sattr: *const SECURITY_ATTRIBUTES) C
GENERIC_READ | FILE_WRITE_ATTRIBUTES | SYNCHRONIZE,
&attrs,
&iosb,
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come you had to revert this change? Do you know why CreateNamedPipeFile is failing with your change?

Copy link

@chawyehsu chawyehsu Apr 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess because NtCreateNamedPipeFile does not support FILE_SHARE_DELETE.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kubkon

CreatePipe() was implemented recently in #19271 as (hopefully) an exact match of the same function in kernel32.
I think all changes to that function should be reverted. There is no extra issue here that could possibly arise with Zig's current flags that the vast majority of other software that just directly calls the kernel32 implementation also wouldn't have.

(The \Device\Null and \??\MountPointManager changes probably deserve a closer look too)


@chawyehsu

Please don't link to leaked source code without a warning (or at all).

Not everyone checks where the link leads before clicking on it, and even if they did, they might not recognize why it's a bad idea in this case.
Even just having this here could make it harder for people to contribute to Windows-related stuff in Zig, especially when it comes to working towards #1840 and similar issues.
ReactOS has had constant issues with this since its inception - I think we better err on the side of caution here. Thanks.

@kubkon kubkon merged commit 084c2cd into ziglang:master May 11, 2024
10 checks passed
SammyJames pushed a commit to SammyJames/zig that referenced this pull request Aug 7, 2024
* use permissive file share flags everywhere

* remove file_share_delete from createnamedpipefile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants