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

Overlapping preopens #111

Open
vshymanskyy opened this issue Apr 28, 2020 · 5 comments
Open

Overlapping preopens #111

vshymanskyy opened this issue Apr 28, 2020 · 5 comments

Comments

@vshymanskyy
Copy link
Contributor

vshymanskyy commented Apr 28, 2020

While working on VFS support, I run into this problem.
A temporary directory is created and I'd like to mount it to the root fs:

    uvwasi_preopen_t preopens[3];
    preopens[0].mapped_path = "/";
    preopens[0].real_path = ".";
    preopens[1].mapped_path = ".";
    preopens[1].real_path = ".";
    preopens[2].mapped_path = "/_wasmer";
    preopens[2].real_path = "/tmp/wasm3_temp/1234/_wasmer";

This conflicts with the first (root) entry, and I cannot access _wasmer.
If I reorder preopens, I can access _wasmer, but not the root content.

How should this be handled?

@cjihrig
Copy link
Collaborator

cjihrig commented Apr 28, 2020

So I stumbled upon this issue recently. It doesn't have a resolution, but implies that the ordering of preopens might matter in the wasi-libc. It's also not clear to me (and I haven't been able to find any clarifying documentation) if this is something that is supposed to be handled by the WASI syscall provider vs. the wasi-libc. I had assumed that given a collection of preopens, wasi-libc would sort things out - otherwise, every WASI implementor needs to reimplement the same bug prone logic.

When an application starts up, the preopens are surveyed via fd_prestat_get() and prestat_dir_name(). Then, the WASI runtime (not uvwasi) matches paths to existing file descriptors (preopened or not). After it has a picture of all of the preopens, wasi-libc should be able to account for nested paths.

I'm sorry I don't know the definitive answer to this. It would be good to have some spec clarification on that. If it's something that uvwasi is required to implement, I could work on that.


As a workaround for your immediate issue, would it be possible to preopen '.' and '/' in uvwasi_init() and then make a call to uvwasi_path_open() to open '/_wasmer' in the context of one of the preopens? That's not a proper long term solution, but it might get you unstuck until we can get clarification on how to handle this.

@vshymanskyy
Copy link
Contributor Author

Yes, it looks like this would require a complete vfs implementation on uvwasi level.
I'm currently unblocked using some other hacks (substituting some file descriptors).

@syrusakbary
Copy link

Adding @MarkMcCaskey to the thread, he worked on the WASI implementation for Wasmer and he might have some ideas on useful approaches for the uvwasi implementation :)

@MarkMcCaskey
Copy link

MarkMcCaskey commented May 18, 2020

Yeah we definitely ran into issues with order and nesting of pre-opens. I think we ended up just trying to simplify things as much as possible. Anyways here's an info-dump of what I remember:

  • The order of preopens is an implementation-specific bug as far as I remember and not a property of the compiled Wasm module (so not a libc issue) and I thought wasmer was not affected by it (though we did change the logic not too long ago for recursively creating virtual parent directories and it would not be surprising if that broke something...)
  • A lot of this logic is underdocumented, both on the WASI spec side and the runtime side. For example, in Wasmer / and . (and probably _wasmer in the root in certain circumstances) are sometimes treated as special values and so using them directly is definitely putting yourself at higher risk of hitting some obscure, accidentally untested edge cases.
  • The virtual root, /, is not actually part of the WASI spec, but it's quite useful and I've written about some of its merits in issues on the WASI repo in the past.
  • What I currently remember about Wasmer's implementation of nested preopens is that:
    • It allows preopens to override real files (e.g. a real filesystem of dir a, file b in a and preopens of a/b will make the real file b not accessible and the virtual preopen b will take its place.
    • No attempt is made to make the permissions make sense together in the context of nesting and I don't remember if we force using the most specific preopen when operating on a file that could be accessed from either preopen but it's something I spent a while thinking about in the past. This is also undefined by WASI so 🤷
    • The implementation of _wasmer I believe requires a bit of a hack in path_open. It was originally just a temporary work-around but it ended up shipping because I couldn't find a better way to make it work. The way I remember it working is that these types of host-controlled pre-opens have an extra flag set that allows them to be opened even though they're already opened and closing them doesn't actually close them.

Anyways, if the order of preopens matters in wasmer in general then that's a bug as far as I'm concerned. It's important to be aware that some implementations of WASI, such as AssemblyScript's are very much order dependent given their current implementation (Wasmer's virtual root usually makes this bug harder to see in practice).

@cjihrig
Copy link
Collaborator

cjihrig commented May 19, 2020

Thanks @MarkMcCaskey, I appreciate the detailed feedback. This seems like something that needs to be defined in the spec. Otherwise, there are multiple ways for runtimes to approach this, and there will be no consistency across the ecosystem (based on your comments, that seems to be the case already).

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

4 participants