bytecodealliance / wasm-micro-runtime

WebAssembly Micro Runtime (WAMR)
Apache License 2.0
4.98k stars 628 forks source link

`fd_pwrite` after setting `fdflags::append` writes from the wrong offset #2828

Closed yagehu closed 7 months ago

yagehu commented 12 months ago

This is a subtle platform-specific WAIS bug. Run the following file compiled with wasi-sdk. It creates a file tmp/a and performs this sequence of actions:

  1. Write 67 bytes to tmp/a.
  2. Set the fdflags::append flag via fcntl().
  3. Write 102 bytes at offset 0 to tmp/a with fd_pwrite.
#include <fcntl.h>
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>

int main() {
    int f = open("tmp/a", O_CREAT | O_WRONLY);
    if (f == -1) {
        perror("open");
    return 1;
    }

    char buf[67];

    int written = write(f, buf, 67);
    printf("written %d\n", written);

    int ret = fcntl(f, F_SETFL, O_APPEND);
    if (ret == -1) {
    perror("fcntl");
    return 1;
    }

    char buf2[102];

    int written2 = pwrite(f, buf2, 102, 0);
    printf("written %d\n", written2);
}

The resulting file should be 102 bytes because fd_pwrite should write from offset 0 regardless of the append flag per specification (WASI specifies fd_pwrite to be similar to POSIX pwritev which specifies this behavior).

Wasmtime behaves correctly, producing a 102-byte file. WAMR produces a 169-byte file.

wenyongh commented 12 months ago

Hi, I tested the result of the gcc native (gcc -O3 -o test main, and then ./test), it also creates a 169-byte file. It is confusing whether we should comply with wasi specification or not, if yes, we may change the behavior of C application. And I found that eventually the libc pwritev (or pwrite) was called with offset 0, but it is strange that it didn't write the data from offset 0.

yagehu commented 12 months ago

@wenyongh Definitely a fair point. man 2 pwrite identifies this as a Linux bug. It was first raised by Micharl Kerrisk. However, you can also argue WASI aims to be platform independent. The Wasmtime team has probably fixed it by using cap-std. I leave it up to you WAMR folks to decide if you wontfix it or not. I'm just reporting this deviation from the specification.

yagehu commented 11 months ago

I did some digging and found that Wasmtime solves it in preview1 by managing the append flag and hiding it from the underlying syscall. It has nothing to do with cap-std, which simply forwards the call to Rust std.

Notably, here and here.

yamt commented 11 months ago

I did some digging and found that Wasmtime solves it in preview1 by managing the append flag and hiding it from the underlying syscall. It has nothing to do with cap-std, which simply forwards the call to Rust std.

Notably, here and here.

such a "fix" would break an important use case of O_APPEND, which is multiple writers to a single file.

yagehu commented 11 months ago

I did some digging and found that Wasmtime solves it in preview1 by managing the append flag and hiding it from the underlying syscall. It has nothing to do with cap-std, which simply forwards the call to Rust std. Notably, here and here.

such a "fix" would break an important use case of O_APPEND, which is multiple writers to a single file.

I don't think it would break the use case if implemented correctly considering you don't have multiple processes in WASI.

yamt commented 11 months ago

I did some digging and found that Wasmtime solves it in preview1 by managing the append flag and hiding it from the underlying syscall. It has nothing to do with cap-std, which simply forwards the call to Rust std. Notably, here and here.

such a "fix" would break an important use case of O_APPEND, which is multiple writers to a single file.

I don't think it would break the use case if implemented correctly considering you don't have multiple processes in WASI.

"multiple writers" can include a host process which is not related to wamr.

yagehu commented 7 months ago

Since this is a Linux behavior, it would be hard to fix without emulating the WASI fs layer. Closing as wontfix.