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

How to correctly get the right file descriptor and execute in wasmtime? #6505

Closed
orangeC23 opened this issue Jun 2, 2023 · 8 comments
Closed

Comments

@orangeC23
Copy link

orangeC23 commented Jun 2, 2023

Steps to reproduce

(1)'cargo new fd_advise'
(2)cd fd_advise and modify the main.rs :

fn main() {
    unsafe {
        let p1:wasi::Fd = 3; 
        let p2:wasi::Filesize = 10;
        let p3:wasi::Filesize = 20;
        let p4:wasi::Advice = wasi::ADVICE_NORMAL;
    
        println!("===[Result]===");
        println!("{:?}", wasi::fd_advise(p1, p2, p3, p4).unwrap());
    }
}

And add dependency in Cargo.toml:

[dependencies]
wasi = "0.11.0"

(3) cargo build --target wasm32-wasi
(4) execute it by wasmtime:wasmtime run --dir=/Users/name/filetest ./target/wasm32-wasi/debug/fd_advise.wasm and it prints
image

(5) execute it by wasmer:wasmer --dir=/Users/name/filetest ./target/wasm32-wasi/debug/fd_advise.wasm and it prints:
image

without panic

Maybe I use wasmtime with wrong code ?

@bjorn3
Copy link
Contributor

bjorn3 commented Jun 2, 2023

I don't think fd_advise is meant to be used on directories. --dir opens a directory, not a file. If /Users/name/filetest is a file, I'm surprised you don't get an error earlier when trying to open it as directory.

@pchickey
Copy link
Collaborator

pchickey commented Jun 2, 2023

@bjorn3 is correct: wasmtime's wasi implementation fails BADF on directories because fd_advise does not work on directories. In your example, fd 3 is a preopened directory.

@pchickey
Copy link
Collaborator

pchickey commented Jun 2, 2023

Since I believe what you described is the intended behavior, I'm going to close this issue, but please re-open if my understanding of the problem is incorrect.

@pchickey pchickey closed this as completed Jun 2, 2023
@bjorn3
Copy link
Contributor

bjorn3 commented Jun 2, 2023

Would it make sense to add a test that the wasi implementation will return EBADF for fd_advise on directories?

@pchickey
Copy link
Collaborator

pchickey commented Jun 2, 2023

We already do, along with a note that it differs from the Linux behavior. https://github.com/bytecodealliance/wasmtime/blob/main/crates/test-programs/wasi-tests/src/bin/dir_fd_op_failures.rs#L64

@orangeC23
Copy link
Author

Thanks a lot !

wenyongh pushed a commit to bytecodealliance/wasm-micro-runtime that referenced this issue Dec 7, 2023
This commit adds a check to `fd_advise`.  If the fd is a directory,
return `ebadf`.  This brings iwasm in line with Wasmtime's behavior.
WASI folks have stated that fd_advise should not work on directories
as this is a Linux-specific behavior:
bytecodealliance/wasmtime#6505 (comment)
@yamt
Copy link

yamt commented Dec 7, 2023

it sounds like a bug (or a restriction) in wasmtime to me.

in posix environments, it's common to ignore fadvice on directory.
wasi doesn't seem to have a word about the case. (so i expect it to follow posix equivalent.)

besides that, if you want to make it fail for some reasons, i feel it's more natural to use EISDIR than EBADF.

@pchickey
Copy link
Collaborator

pchickey commented Dec 7, 2023

OK. This is the least interesting topic possible to debate, so we will change wasmtime's behavior to ignore and return success if you send a PR to change our implementations and tests.

victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this issue May 27, 2024
This commit adds a check to `fd_advise`.  If the fd is a directory,
return `ebadf`.  This brings iwasm in line with Wasmtime's behavior.
WASI folks have stated that fd_advise should not work on directories
as this is a Linux-specific behavior:
bytecodealliance/wasmtime#6505 (comment)
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

No branches or pull requests

4 participants