Review safety of all functions

[newpavlov]

Are we sure what all Core API functions can be wrapped in a safe interface?

[sunfishcode]

We haven't done any formal proofs or fuzzing, but other than possible implementation bugs, I'm not aware of anything in WASI which could cause unsafety in the Rust sense.

[newpavlov]

I have this question because from_raw_fd is an unsafe function. For example what will happen if user will call fd_close(25) and it is a valid file descriptor? Will we get an IO error if we'll try to do something with this FD? Is it guaranteed that FDs are not getting reused?

[sunfishcode]

It's a good question. Yes, you can fd_close an arbitrary file descriptor, even one held by libstd. And file descriptors can get reused. It could cause I/O errors or potentially lost data. It could route data intended for one stream into another. But it shouldn't cause undefined behavior. Does that justify unsafe?

It justifies documentation, in any case :-).

[newpavlov]

Does that justify unsafe?

I don't know. :)

@RalfJung
Can you help us here?

[RalfJung]

That's kind of an arbitrary call that the ecosystem as a whole has to agree on -- both options work, as long as they are done consistently.

But judging from things like rust-lang/rust#39575, Rust seems to be on the side of "messing with other people's file descriptors is unsafe".

[RalfJung]

But it shouldn't cause undefined behavior.

It doesn't cause UB on its own, but if other code relies on privacy of its file descriptors, that could then in combination lead to UB. So the question is if we allow other code to assume that its file descriptors are private. And I think we do, with the issue mentioned above (and unsafe from_raw_fd) being an indication, but I don't think this has been officially resolved.

[sunfishcode]

That's a good point. And if we let code assume file descriptors are private, one could argue that even non-mutating functions, like fd_fdstat_get (WASI's fstat), should be unsafe, because they could leak potentially sensitive information. So maybe everything that takes a file descriptor argument (which is most of the API) should be unsafe.

That's inconvenient, but ideally, we shouldn't have lots of people using the wasi crate directly anyway, as people should usually prefer libstd and libc. So I think I could be ok making most things in this repo unsafe, to err on the side of caution.

[newpavlov]

@sunfishcode
I think we can be conservative and mark all functions unsafe except the "obviously" safe: clock_res_get, clock_time_get, proc_exit, random_get, sched_yield. Other functions can be made safe in a backwards compatible manner after a better model of safety around file descriptors will get worked out.

If you want, I can add these changes to #5.

[sunfishcode]

Yes, that'd be great.

Looking forward, clock and random APIs at least will likely move to requiring file descriptors, but until they do, it's fine to leave them safe. It's interesting that access to ambient authorities is aligning with unsafe.

[marmistrz]

That's kind of an arbitrary call that the ecosystem as a whole has to agree on -- both options work, as long as they are done consistently.

But judging from things like rust-lang/rust#39575, Rust seems to be on the side of "messing with other people's file descriptors is unsafe".

Related: https://users.rust-lang.org/t/when-should-we-mark-our-functions-unsafe/11834/3

[newpavlov]

BTW I think it should be fine to remove unsafe for "read-only" functions like fd_prestat_get, fd_prestat_dir_name and probably others. So it may be worth to reopen this issue or create a new one.

[RalfJung]

Even read access can still be abstraction-breaking, and abstraction is what we use to tame unsafe in Rust.

We require unsafe code to read from private Copy fields of structs in other modules, even though this could be made a safe operation. I think we should follow the same line for read access to other potentially private state.

[newpavlov]

In v0.8 all functions are now safe, is it intentional? Even if we'll leave FD stuff out, signatures like this are definitely smell trouble in my opinion.

[sunfishcode]

@newpavlov Thanks for spotting that -- I've now opened #24 to reintroduce the unsafe annotations.