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

Cannot open a file with O_TRUNC and O_APPEND #4376

Closed
yagehu opened this issue Dec 27, 2023 · 7 comments · Fixed by #4394
Closed

Cannot open a file with O_TRUNC and O_APPEND #4376

yagehu opened this issue Dec 27, 2023 · 7 comments · Fixed by #4394
Assignees
Labels
bug Something isn't working 📦 lib-wasi About wasmer-wasi priority-medium Medium priority issue
Milestone

Comments

@yagehu
Copy link
Contributor

yagehu commented Dec 27, 2023

Describe the bug

This might be a wontfix but I figure I should report this anyway since out of all the runtimes I've tested only Wasmer and wasmi have this behavior. Native Linux also does not have this behavior.

Opening a file with O_TRUNC and O_APPEND together results in errno 28 inval. This is a result of the host-fs FileSystem uses Rust std::fs::OpenOptions, which performs this check.

I think Wasmtime doesn't have this behavior because it uses rustix which directly invokes the open syscall without this extra check.

$ zwasmer -vV; rustc -vV
wasmer 4.2.4 (0d782f1 2023-12-22)
binary: wasmer-cli
commit-hash: 0d782f18f99ba503f9aff89669c6ffd8e1631f53
commit-date: 2023-12-22
host: x86_64-unknown-linux-gnu
compiler: singlepass,cranelift
rustc 1.74.1 (a28077b28 2023-12-04)
binary: rustc
commit-hash: a28077b28a02b92985b3a3faecf92813155f1ea1
commit-date: 2023-12-04
host: x86_64-unknown-linux-gnu
release: 1.74.1
LLVM version: 17.0.4

Steps to reproduce

Compile the snippet with wasi-sdk and run with wasmer, mounting a directory.

#include <fcntl.h>
#include <stdio.h>

int main(void) {
    int fd = open("tmp/j", O_CREAT | O_RDWR | O_TRUNC | O_APPEND);
    if (fd == -1) {
        perror("open");
        return 1;
    }

    return 0;
}

Expected behavior

It should create the file tmp/j and exit with no error.

Actual behavior

open: Invalid argument

Copy link

linear bot commented Dec 27, 2023

@Arshia001 Arshia001 added bug Something isn't working 📦 lib-wasi About wasmer-wasi priority-medium Medium priority issue labels Jan 2, 2024
@Arshia001 Arshia001 added this to the v4.2.6 milestone Jan 2, 2024
@Arshia001
Copy link
Member

@yagehu thanks for reporting this, it will be fixed in an upcoming release.

@maminrayej
Copy link
Contributor

The combination of O_TRUNC and O_APPEND is considered nonsensical (more context is in the original PR that implemented the functionality described in this RFC). There is also a clippy lint for it that particularly prohibits combining these two flags.

But if it is desired to allow this combination, we have to bypass OpenOptions and call the open syscall directly.

cc @syrusakbary, @Michael-F-Bryan

@Arshia001
Copy link
Member

Or, maybe we could detect the combination and keep only one of the two flags? I assume specifying both flags is undefined behavior in Linux?

@maminrayej
Copy link
Contributor

When you combine O_TRUNC with O_APPEND, the file is first truncated when it's opened, and then all subsequent writes are appended to the end of the file. So, the behavior is well-defined, but it may not match the expectations of the user. So, I think returning an error is the right call here since combining these two doesn't have a practical use case anyway.

@Arshia001
Copy link
Member

Well, yes, that's why rust gives you the error, but we're aiming for compatibility with the behavior of native binaries, be it as wrong or nonsensical as it may, so let's replicate the native behavior here.

@Arshia001
Copy link
Member

@maminrayej keep in mind, Wasmer runs on Windows as well, so make sure any changes you make are also compatible with Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-wasi About wasmer-wasi priority-medium Medium priority issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants