Skip to content
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

Fix opening directories failing on Windows when fdFlags contains NONBLOCK #6348

Merged
merged 1 commit into from
May 8, 2023

Conversation

juamedgod
Copy link
Contributor

Description

We were testing the new wasmtime 8.0.1 on Windows and are getting "Permission denied" errors opening directories, while reading files under those directories work fine.

Test Case

The issue is reproducible using the below rust program (built as lister.wasm in the snippets) that directly calls opendir:

extern crate libc;
use std::ffi::CString;
use libc::{opendir, closedir, c_char};
use std::env;

fn main() {
    let args: Vec<String> = env::args().collect();
    if args.len() != 2 {
        println!("Usage: listdir <directory>");
        return;
    }
    let dir_name_cstring = CString::new(args[1].clone()).unwrap();
    unsafe {
        let dir_ptr = opendir(dir_name_cstring.as_ptr() as *const c_char);
        if dir_ptr.is_null() {
            println!("Error opening directory");
            return;
        } else {
            closedir(dir_ptr);
            println!("Opening directory worked");
        }
    }
}

Steps to Reproduce

Build a wasm module using the test case snippet, and invoke it pointing to a directory using wasmtime-8.0.1 on Windows:

$> c:\tests\wasmtime-v8.0.1-x86_64-windows\wasmtime.exe --mapdir /app::c:/tests/ c:\tests\lister.wasm -- /app
Error opening directory

The same command works when using 8.0.0:

$> c:\tests\wasmtime-v8.0.0-x86_64-windows\wasmtime.exe --mapdir /app::c:/tests/ c:\tests\lister.wasm -- /app
Opening directory worked

Versions and Environment

Wasmtime version or commit: wasmtime 8.0.1
Operating system: Windows 10
Architecture: x86_64

Extra details

The bug seems to be related to the changes in #6163. The code is failing here:

https://github.com/bytecodealliance/wasmtime/blob/main/crates/wasi-common/cap-std-sync/src/dir.rs#L93

Which was not previously applied to directories. That results on the directory being reopened which ends up calling Windows ReOpenFile, and that returns a "Access denied".

We have "fixed" the issue by moving the code to be executed only for files, but that may not be the best solution. It would be great of you could give us some tips if that is not the proper solution to improve the patch.

@juamedgod juamedgod requested a review from a team as a code owner May 5, 2023 16:25
@juamedgod juamedgod requested review from jameysharp and removed request for a team May 5, 2023 16:25
@github-actions github-actions bot added the wasi Issues pertaining to WASI label May 5, 2023
@github-actions
Copy link

github-actions bot commented May 5, 2023

Subscribe to Label Action

cc @kubkon

This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

  • kubkon: wasi

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

Thank you for finding and fixing this bug!

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

This fix makes sense, and thank you for the new tests as well!

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