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

[WASI] Should we overlook or restrict the access modes for stdin/stdout/stderr? #3686

Closed
lum1n0us opened this issue Aug 5, 2024 · 4 comments

Comments

@lum1n0us
Copy link
Collaborator

lum1n0us commented Aug 5, 2024

We've observed that in certain situations, the access modes for STDIN, STDOUT and STDERR do not behave as anticipated. For example, STDOUT may be read-only, while STDIN may be write-only.

It's important that we look into why these unexpected access modes are occurring. However, I'm wondering if we need to take steps to block these incorrect settings in core/iwasm/libraries/libc-wasi

Specifically, when determining file descriptor rights in the function fd_determine_type_rights(), should we assign fixed and unchangeable rights to STDIN, STDOUT and STDERR?

@wenyongh
Copy link
Contributor

wenyongh commented Aug 8, 2024

I saw the PR #3694 sets the STDIN rights with:

(__WASI_RIGHT_FD_ADVISE | __WASI_RIGHT_FD_FILESTAT_GET | \
  __WASI_RIGHT_FD_READ | __WASI_RIGHT_POLL_FD_READWRITE)

not sure will it disable the write right for STDIN and disable the read right for STDOUT? It seems the similar issues were reported before, but after discussion with @yamt, we decided not to change the code:
#2645 (comment)
#2606

@yamt
Copy link
Collaborator

yamt commented Aug 8, 2024

We've observed that in certain situations, the access modes for STDIN, STDOUT and STDERR do not behave as anticipated. For example, STDOUT may be read-only, while STDIN may be write-only.

It's important that we look into why these unexpected access modes are occurring. However, I'm wondering if we need to take steps to block these incorrect settings in core/iwasm/libraries/libc-wasi

yes, please investigate the situation further before making changes to libc-wasi.

@lum1n0us
Copy link
Collaborator Author

lum1n0us commented Aug 8, 2024

The problem originated from integrating WAMR with runwasi, which aims to run a WebAssembly runtime within containerd. I haven't pinpointed the exact issue yet, but there are some unanticipated behaviors occurring within their sandbox environment. Like, fcntl(STDOUT, F_GETFL, 0) returns O_RDONLY. The unusual result will strip off write bits.

Then I made an trial on linux.

$ cat demo.c
int main() {
    int ret;

    ret = fcntl(STDOUT_FILENO, F_GETFL);
    printf("fcntl(stdout, F_GETFL)=%d\n", ret);

    ret = fcntl(STDOUT_FILENO, F_SETFL, O_RDONLY);
    perror("fcntl(stdout, F_SETFL, O_RDONLY)");

    ret = fcntl(STDOUT_FILENO, F_GETFL);
    printf("fcntl(stdout, F_GETFL)=%d\n", ret);

    return EXIT_SUCCESS;
}
$ clang -o demo demo.c
$ demo
fcntl(stdout, F_GETFL)=2
fcntl(stdout, F_SETFL, O_RDONLY): Success
fcntl(stdout, F_GETFL)=2

IIUC, this suggests that the access mode for STDOUT cannot be altered. As a result, it appears that the access mode for STDOUT ought to remain fixed, making any previous removal of write permissions unnecessary.

If everyone is in consensus that the access modes for STDIN, STDOUT, and STDERR are fixed and cannot be altered, then we can begin to discuss the suitable combinations of permissions for these three streams.

@wenyongh
Copy link
Contributor

wenyongh commented Aug 9, 2024

Agree that the access modes for STDIN, STDOUT and STDERR should be fixed, and suggest adding __WASI_RIGHT_FD_READ | __WASI_RIGHT_FD_WRITE to the rights of them all.

wenyongh pushed a commit that referenced this issue Aug 13, 2024
…r access modes (#3694)

When determining the file descriptor rights in the function fd_determine_type_rights(),
we assign fixed and unchangeable rights to STDIN, STDOUT and STDERR.

ps.
#3686
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants