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

fopen/openat does not seem to preserve access flags #415

Open
squeek502 opened this issue May 24, 2023 · 11 comments
Open

fopen/openat does not seem to preserve access flags #415

squeek502 opened this issue May 24, 2023 · 11 comments

Comments

@squeek502
Copy link

squeek502 commented May 24, 2023

Tested on Linux with wasmtime v9.0.1 and wasi-sdk-20.0.

The following code (saved as open-rw.c) tries to open a file (that exists) for reading and writing:

#include <stdio.h>
#include <stdlib.h>

int main() {
    // Note: using an `openat` call directly here with O_WRONLY or O_RDWR would have the same result
    FILE* file = fopen("testfile", "w");
    if (!file) return 1;
    fclose(file);
    return 0;
}

EDIT: The above code originally used the invalid flags "rw", it has been corrected to use "w"

However, when using wasi-libc, the actual syscall (for me it's openat2) is being passed O_RDONLY instead of the expected O_WRONLY. Note: same applies to O_RDWR, O_RDONLY is still passed instead.

Compiled with wasi-sdk-20.0:

$ ls testfile
testfile
$ $WASI_SDK/bin/clang --sysroot=$WASI_SDK/share/wasi-sysroot open-rw.c -o open-rw-sdk.wasm
$ strace -e trace=openat2 wasmtime --dir=. open-rw-sdk.wasm
openat2(3, "testfile", {flags=O_RDONLY|O_CLOEXEC, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 5
+++ exited with 0 +++

Note that access flags are preserved in the equivalent Zig code (unless -lc to link libc is used in which case wasi-libc is built/linked and the O_RDONLY behavior occurs)

const std = @import("std");

pub fn main() !void {
    var file = try std.fs.cwd().openFile("testfile", .{ .mode = .read_write });
    defer file.close();
}
$ zig build-exe -target wasm32-wasi open-rw.zig -femit-bin=open-rw-zig.wasm
$ strace -e trace=openat2 wasmtime --dir=. open-rw-zig.wasm
openat2(3, "testfile", {flags=O_RDWR|O_APPEND|O_NOFOLLOW|O_CLOEXEC, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 5
+++ exited with 0 +++

And are also preserved in the equivalent Rust code:

use std::fs::OpenOptions;

fn main() {
  let mut _file = OpenOptions::new().read(true).write(true).open("testfile").unwrap();
}
$ strace -e trace=openat2 wasmtime --dir=. open-rw-rust.wasm
openat2(3, "testfile", {flags=O_RDWR|O_CLOEXEC, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 5
+++ exited with 0 +++
@pchickey
Copy link
Collaborator

Can you please run wasmtime with the environment variable RUST_LOG=wasi_common=trace to provide additional debug information? That will help us determine whether this is a wasmtime or libc issue.

@jedisct1
Copy link
Member

@jedisct1
Copy link
Member

wasmedge, wasm3 ,node and wasmer return an error, even when the libc is linked.

@jedisct1
Copy link
Member

wasmtime 8.0.0 and all previous versions print error: IsDir

The behavior changed in wasmtime 9.0.0.

Log from wasmtime 8.0.0: wasmtime8.txt

@jedisct1
Copy link
Member

diff between wasmtime 8 and wasmtime 9, with the same wasm file.

@pchickey
Copy link
Collaborator

Thanks. Can you please move this bug over to the wasmtime repository? This appears to be a bug over there, not in wasi-libc.

@jedisct1
Copy link
Member

jedisct1 commented May 24, 2023

Something changed in wasmtime, but I'm not sure wasi-libc is not involved here.

Looking at the traces, for the same code, this is what we see without wasi-libc:

TRACE wasi_common::snapshots::preview_1::wasi_snapshot_preview1 > wiggle abi; module="wasi_snapshot_preview1" function="path_open"

TRACE wasi_common::snapshots::preview_1::wasi_snapshot_preview1 >
fd=Fd(3) dirflags=(empty) path=*guest 0x100290/8 oflags=(empty)
fs_rights_base=FD_DATASYNC | FD_READ | FD_SEEK | FD_FDSTAT_SET_FLAGS |
FD_SYNC | FD_TELL | FD_WRITE | FD_ADVISE | FD_ALLOCATE |
FD_FILESTAT_GET | FD_FILESTAT_SET_SIZE | FD_FILESTAT_SET_TIMES
fs_rights_inheriting=(empty) fdflags=APPEND

TRACE wasi_common::snapshots::preview_1::wasi_snapshot_preview1 > result=Err(Error { inner: Isdir })

and with wasi-libc:

 TRACE wasi_common::snapshots::preview_1::wasi_snapshot_preview1 >
 result=Ok(Fdstat { fs_filetype: Directory, fs_flags: (empty),
 fs_rights_base: (empty), fs_rights_inheriting: (empty) })

 TRACE wasi_common::snapshots::preview_1::wasi_snapshot_preview1 > wiggle abi; module="wasi_snapshot_preview1" function="path_open"

 TRACE wasi_common::snapshots::preview_1::wasi_snapshot_preview1 >
 fd=Fd(3) dirflags=SYMLINK_FOLLOW path=*guest 0xfef9c/8 oflags=(empty)
 fs_rights_base=(empty) fs_rights_inheriting=(empty) fdflags=(empty)

 TRACE wasi_common::snapshots::preview_1::wasi_snapshot_preview1 > result=Ok(Fd(4))

@jedisct1
Copy link
Member

jedisct1 commented May 24, 2023

This is the wasmtime commit that changed the behavior: liberate wasi-common from Rights

@squeek502
Copy link
Author

squeek502 commented May 24, 2023

Just to make a clarification:

wasmedge, wasm3 ,node and wasmer return an error, even when the libc is linked.

wasmtime 8.0.0 and all previous versions print error: IsDir

This is referring to how the problem was manifesting in the Zig test suite, where it'd try to open a directory with O_RDWR but EISDIR would not be returned. So, in @jedisct1's comments, 'return an error' is the correct behavior.

The reproduction in the OP of this issue does not include that wrinkle and is instead focused on just the flags being passed to the syscall, since I realized the problem is more general than just opening directories for writing.

@pchickey
Copy link
Collaborator

This is a bug in Wasmtime 9.0.{0,1}. I have prepared a fix which we will release as 9.0.2.

The behavior is the same across wasi-sdk 19 and 20, I did not test any further back but I believe it is the same for a long time.

In the original test case given, this line:

    FILE* file = fopen("testfile", "rw");

will actually open a file read-only. The mode string to open read-write is "r+".

I made variants of that test for read-only (r), write after truncating (w), and read-write (r+) access, and am testing them here against the wasmtime executable built from bytecodealliance/wasmtime#6462

[phickey@pch-tower:src/wasi_libc_415]% strace -e trace=openat2 ../wasmtime/target/release/wasmtime --disable-cache --dir . open-ro-sdk19.wasm
openat2(3, "testfile", {flags=O_RDONLY|O_CLOEXEC, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 5
+++ exited with 0 +++
[phickey@pch-tower:src/wasi_libc_415]% strace -e trace=openat2 ../wasmtime/target/release/wasmtime --disable-cache --dir . open-rw-sdk19.wasm
openat2(3, "testfile", {flags=O_RDWR|O_CLOEXEC, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 5
+++ exited with 0 +++
[phickey@pch-tower:src/wasi_libc_415]%
[phickey@pch-tower:src/wasi_libc_415]% strace -e trace=openat2 ../wasmtime/target/release/wasmtime --disable-cache --dir . open-ro-sdk20.wasm
openat2(3, "testfile", {flags=O_RDONLY|O_CLOEXEC, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 5
+++ exited with 0 +++
[phickey@pch-tower:src/wasi_libc_415]% strace -e trace=openat2 ../wasmtime/target/release/wasmtime --disable-cache --dir . open-rw-sdk20.wasm
openat2(3, "testfile", {flags=O_RDWR|O_CLOEXEC, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 5
+++ exited with 0 +++
[phickey@pch-tower:src/wasi_libc_415]% strace -e trace=openat2 ../wasmtime/target/release/wasmtime --disable-cache --dir . open-wo-sdk19.wasm
openat2(3, "testfile", {flags=O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, mode=0666, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 5
+++ exited with 0 +++
[phickey@pch-tower:src/wasi_libc_415]% strace -e trace=openat2 ../wasmtime/target/release/wasmtime --disable-cache --dir . open-wo-sdk20.wasm
openat2(3, "testfile", {flags=O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, mode=0666, resolve=RESOLVE_NO_MAGICLINKS|RESOLVE_BENEATH}, 24) = 5
+++ exited with 0 +++

@jedisct1
Copy link
Member

Awesome!

Thank you Pat!

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

3 participants