Repository navigation
feat: please turn the WASI returnOnExit option to default true #46923
Description
Activity
@nodejs/wasi
I don't have strong feelings either way.
My only concern about making the default
trueis that the implementation ofreturnOnExitis currently pretty hacky. It monkey patches the WASI imports to throw a JavaScript Symbol that WASM cannot catch. If we were to make the defaulttrue, we should plumb thereturnOnExitoption to the native layer (ideally to here by making it an option onuvwasi_t).EDIT: One thing that occurred to me is that we implemented the current hack because if
proc_exit()did not exit the process, an assertion was raised (not from Node or uvwasi). If that assertion still exists, I'm not sure that we can cleanly return.I brought this up in today's wasi team meeting. It sounds like our hacky approach is essentially the right way to go for now and we should change the default value of
returnOnExittotrue.@cjihrig I'm happy to put together a PR to change the default. It is potentially breaking though so I'm wondering if we should mark it SemVer Major even though the feature is experimental. What do you think. Also my plan would be to put the PR together early next week so it would not make the cutoff for 20.x
How about making it semver-minor and not backporting to 19.x and below?
WASI is still experimental in Node so we can change it at any time. But I do think not backporting it is a good idea.
I like the suggestion as well. So we'll plan to mark semver-minor and then tag for don't backport to 19.x etc.
Reacted by Tobias NießenI'll plan to submit a PR Friday or more likely Monday.
- added a commit that references this issue
on May 2, 2023 I think this can be closed as the PR to switch the default has landed. Let me know if you think that was not the right thing to do.
Feature Request
Overview
node/doc/api/wasi.md
Lines 139 to 142 in c4103c1
node/lib/wasi.js
Lines 112 to 116 in c4103c1
The current behaviour is for the WASI/WASM module to exit the process when
__wasi_proc_exitis called. Not only that, currently it also allows the WASI module to control the exit code.This seems like a capability that should be explicitly provided to the module rather than something that is on by default. It's unintuitive that a process running in a sandbox would have the ability to crash the entire app without the caller giving it explicit permission to do so.
This is particularly an issue when running 3rd party modules since you rarely have a complete idea on when and where the module might call
__wasi_proc_exitand with what exit codes.Returning instead of exitting when a sandboxed process finishes aligns better with the principles of least privilege and secure by default.
Context
#46254 (comment)
https://github.com/nodejs/node/blob/main/test/wasi/test-return-on-exit.js