Skip to content

Conversation

@wingo
Copy link
Contributor

@wingo wingo commented Dec 8, 2025

On Windows, it was possible to return a directory descriptor even if WRITE was in the permissions. Fixes wasmtime for
WebAssembly/wasi-testsuite#176.

@wingo wingo requested a review from a team as a code owner December 8, 2025 10:46
@wingo
Copy link
Contributor Author

wingo commented Dec 8, 2025

In all honesty I haven't tested on Windows, it's just that this patch should fix the issue. To repro, check out the prod/testsuite-base branch of https://github.com/WebAssembly/wasi-testsuite and run:

wasmtime -Wcomponent-model-async -Sp3,http --dir 'tests\rust\testsuite\wasm32-wasip3\fs-tests.dir::fs-tests.dir' 'tests\rust\testsuite\wasm32-wasip3\filesystem-flags-and-type.wasm'

@wingo wingo force-pushed the windows-open-dir-with-no-read branch from 2e830ac to 6cd0e89 Compare December 8, 2025 10:52
@wingo
Copy link
Contributor Author

wingo commented Dec 8, 2025

Rebased to fix the rustfmt error

@wingo wingo changed the title filesystem: ErrorCode::IsDirectory when opening director w/o READ filesystem: ErrorCode::IsDirectory when opening directory w/o READ Dec 8, 2025
On Windows, it was possible to return a directory descriptor if READ
wasn't in the permissions.  Fixes wasmtime for
WebAssembly/wasi-testsuite#176.
@wingo wingo force-pushed the windows-open-dir-with-no-read branch from 6cd0e89 to a7831bf Compare December 8, 2025 11:30
@rvolosatovs rvolosatovs enabled auto-merge December 8, 2025 11:38
@rvolosatovs rvolosatovs added this pull request to the merge queue Dec 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 8, 2025
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Dec 8, 2025
@wingo
Copy link
Contributor Author

wingo commented Dec 8, 2025

My goodness, there are many CI checks in this project :) Patch fixes unused-variable warning/error when compiling for Windows.

@rvolosatovs rvolosatovs enabled auto-merge December 8, 2025 13:21
@rvolosatovs
Copy link
Member

My goodness, there are many CI checks in this project :) Patch fixes unused-variable warning/error when compiling for Windows.

You can add prtest:full to commit message description to trigger a full CI run for cases like these

@rvolosatovs rvolosatovs added this pull request to the merge queue Dec 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 8, 2025
@alexcrichton
Copy link
Member

Test-wise this is something that'll be caught when updating to the latest wasi-testsuite, right? If so, I think it's ok to skip a test here. Mind leaving a comment though on this match arm explaining that this is sync'ing windows semantics to unix semantics?

I had misremembered the nature of this incompatibility.
Fixed (hopefully) with a reference to POSIX.

prtest:full
@wingo wingo changed the title filesystem: ErrorCode::IsDirectory when opening directory w/o READ filesystem: ErrorCode::IsDirectory when opening directory with WRITE Dec 9, 2025
@wingo
Copy link
Contributor Author

wingo commented Dec 9, 2025

I had misremembered the nature of this error, my apologies. The problem is not the lack of READ, it's the presence of WRITE; see the open specification. Thank you for your patience.

@wingo wingo requested a review from a team as a code owner December 9, 2025 08:26
@wingo wingo requested review from alexcrichton and removed request for a team December 9, 2025 08:26
wingo added 2 commits December 9, 2025 09:53
path_open_preopen is now working on Windows.
@wingo wingo force-pushed the windows-open-dir-with-no-read branch from c62ec38 to a010fbe Compare December 9, 2025 10:23
@wingo
Copy link
Contributor Author

wingo commented Dec 9, 2025

Lordie me, this PR has been an education for me, thanks all for patience. Ready now (again).

@alexcrichton
Copy link
Member

Anything in particular we could make easier on our end? Or more of a "lots of stuff to discover" kind of education?

@alexcrichton alexcrichton added this pull request to the merge queue Dec 9, 2025
Merged via the queue into bytecodealliance:main with commit 9fd594a Dec 9, 2025
172 checks passed
@wingo
Copy link
Contributor Author

wingo commented Dec 10, 2025

Anything in particular we could make easier on our end? Or more of a "lots of stuff to discover" kind of education?

Oh, I think things are pretty good on the wasmtime side, just that there were a number of considerations that I didn't fully grasp (the history of wasi versions and their implementations, the layers of error codes (std::io vs ErrorCode etc), the specificities of how CI works here...).

So things are good. Marginal improvements can be made tho:

  • I didn't know about prtest:full, many thanks to @rvolosatovs; perhaps worth mentioning this when a merge fails, so as to avoid multiple merge failures
  • Would be nice to be able to manually re-run tests that failed, or choose specific jobs, as in chromium's review system (image attached)
  • It could make sense that if a PR includes a line that removes or adds cfg(), that we select the full build matrix
  • One has to click through a few things to get to build errors (the list of checks, scroll to bad job, click, scroll down in the logs enough so that they are all loaded, search for a term); perhaps some summarization is possible

But like I say things are good, and thank you and Roman for your help & patience <3

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wasi Issues pertaining to WASI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants