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

Fix preopened current directory not working #4363

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yagehu
Copy link
Contributor

@yagehu yagehu commented Dec 18, 2023

This commit fixes a bug where creating a file with an implicit relative
path returns 0 indicating success, but no file is created on the host.
Here, implicit relative means a relative path that does not begin with
./. For example, calling open("somefile", O_CREAT | O_WRONLY) will
try to create a file called somefile in the current working directory.

The fix is to change the current directory if the user has explicitly
mounted any host directory to . in guest. In this case, the user's
intention is clear: the current directory is the mounted host
directory.

fixes #4361

@yagehu yagehu force-pushed the yagehu/preopen-dir branch 4 times, most recently from 1deef7f to 866d483 Compare December 18, 2023 23:28
@yagehu yagehu changed the title Fix preopened directory mapping to virtual root Fix preopened current directory not working Dec 19, 2023
This commit fixes a bug where creating a file with an implicit relative
path returns `0` indicating success, but no file is created on the host.
Here, implicit relative means a relative path that does not begin with
`./`. For example, calling `open("somefile", O_CREAT | O_WRONLY)` will
try to create a file called `somefile` in the current working directory.

The fix is to change the current directory if the user has explicitly
mounted any host directory to `.` in guest.  In this case, the user's
intention is clear: the current directory *is* the mounted host
directory.

fixes wasmerio#4361
@yagehu
Copy link
Contributor Author

yagehu commented Dec 22, 2023

@theduke Gentle ping. I think this is a reasonable fix for a common use case not working on Wasmer. Specifically, something like:

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

int main(void) {
  int fd = open("somefile", O_CREAT | O_WRONLY);\
  if (fd == -1) {
    perror("open");
    return 1;
  }

  return 0;
}

@syrusakbary
Copy link
Member

This needs a bit more discussion between @theduke and @syrusakbary (me). Stay tuned, and thanks for opening the PR @yagehu (and the patience!)

@yagehu
Copy link
Contributor Author

yagehu commented Apr 12, 2024

This needs a bit more discussion between @theduke and @syrusakbary (me). Stay tuned, and thanks for opening the PR @yagehu (and the patience!)

Hi @syrusakbary, any update? Just to jolt your memory, the motivation for this PR is that a simple path_open() with an implicit relative path (a relative path without a leading ./) does not work with Wasmer while all other runtimes can handle this behavior. In a contrived C example:

open("somefile", O_CREAT | O_RDWR)

should create a file somefile at the current directory.

For context, I'm differential fuzzing various wasm runtimes at the raw WASI level (bypassing wasi-libc). Wasmer is the only runtime where I need to apply this patch so the file system behavior is consistent with other runtimes.

I can update this PR. There are probably much better ways to make Wasmer's behavior make sense both with and without wasi-libc.

@syrusakbary
Copy link
Member

Hey @yagehu , most of the tests are failing on this branch, which is why I'm hesitant to merge atm

@yagehu
Copy link
Contributor Author

yagehu commented Apr 30, 2024

Hey @yagehu , most of the tests are failing on this branch, which is why I'm hesitant to merge atm

To be clear, I was not asking to merge this PR as is since a lot has changed since my initial PR. I'll update this PR if there's interest in aligning Wasmer path_open behavior with other runtimes.

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

Successfully merging this pull request may close these issues.

path_open not working with wasi-sdk
2 participants