feat: please turn the WASI returnOnExit option to default true

[HarikrishnanBalagopal]

Feature Request

Overview

node/doc/api/wasi.md

Lines 139 to 142 in c4103c1

	* `returnOnExit` {boolean} By default, WASI applications terminate the Node.js
	process via the `__wasi_proc_exit()` function. Setting this option to `true`
	causes `wasi.start()` to return the exit code rather than terminate the
	process. **Default:** `false`.

node/lib/wasi.js

Lines 112 to 116 in c4103c1

	if (options.returnOnExit !== undefined) {
	validateBoolean(options.returnOnExit, 'options.returnOnExit');
	if (options.returnOnExit)
	wrap.proc_exit = FunctionPrototypeBind(wasiReturnOnProcExit, this);
	}

The current behaviour is for the WASI/WASM module to exit the process when __wasi_proc_exit is 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_exit and 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

[bnoordhuis]

@nodejs/wasi

[mhdawson]

@cjihrig what's your take on this? A default of true makes some sense to me but I'm sure you have more context.

[cjihrig]

I don't have strong feelings either way.

My only concern about making the default true is that the implementation of returnOnExit is currently pretty hacky. It monkey patches the WASI imports to throw a JavaScript Symbol that WASM cannot catch. If we were to make the default true, we should plumb the returnOnExit option to the native layer (ideally to here by making it an option on uvwasi_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.

[cjihrig]

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 returnOnExit to true.

[mhdawson]

@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

[tniessen]

How about making it semver-minor and not backporting to 19.x and below?

[cjihrig]

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.

[mhdawson]

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.

[mhdawson]

I'll plan to submit a PR Friday or more likely Monday.

[mhdawson]

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.