wasmerio / wasmer-js

Monorepo for Javascript WebAssembly packages by Wasmer
https://wasmerio.github.io/wasmer-js/
MIT License
924 stars 81 forks source link

fd_renumber implementation is wrong #230

Closed RReverser closed 10 months ago

RReverser commented 4 years ago

While working on my WASI implementation, I was reading fd_renumber docs:

Atomically replace a file descriptor by renumbering another file descriptor. Due to the strong focus on thread safety, this environment does not provide a mechanism to duplicate or renumber a file descriptor to an arbitrary number, like dup2(). This would be prone to race conditions, as an actual file descriptor with the same number could be allocated by a different thread at the same time. This function provides a way to atomically renumber file descriptors, which would disappear if dup2() were to be removed entirely.

I've noticed that Wasmer JS implementation does the following:

https://github.com/wasmerio/wasmer-js/blob/4e660caedba56e50db0da9238a789ab0e2f989be/packages/wasi/src/index.ts#L930-L933

That is, it renumbers to to from, which goes against the naming as well as, upon close reading, against the description in docs.

Then I've checked the upstream implementation of node-wasi by @devsnek that that code was copied from and noticed that, indeed, it used to have same incorrect implementation as well, but it was fixed there a ~year ago to:

https://github.com/devsnek/node-wasi/blob/3624970047bacd19c3fef1447da56ade28f90ef3/src/index.js#L882-L885

which now correctly remaps from descriptor to to.

RReverser commented 4 years ago

Ping @syrusakbary in case this one got missed as well.

Michael-F-Bryan commented 10 months ago

This should be resolved now. All syscalls used by @wasmer/sdk are now implemented in the wasmer-wasix crate.

You can see the implementation in the main wasmerio/wasmer repo, here.