Skip to content

feat: please turn the WASI returnOnExit option to default true #46923

Closed
Closed
feat: please turn the WASI returnOnExit option to default true#46923
Labels
feature requestIssues requesting new Node.js features.wasiIssues and PRs related to the WebAssembly System Interface.

Description

@HarikrishnanBalagopal
HarikrishnanBalagopal
opened on Mar 3, 2023

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

Activity

  1. VoltrexKeyva
    added
    wasiIssues and PRs related to the WebAssembly System Interface.
    on Mar 4, 2023

  2. bnoordhuis commented on Mar 5, 2023

    @bnoordhuis
    bnoordhuis
    on Mar 5, 2023
    Member

    @nodejs/wasi

  3. Trott
    added
    feature requestIssues requesting new Node.js features.
    on Mar 5, 2023
  4. targos
    added this to Node.js feature requestson Mar 10, 2023
  5. github-project-automation
    moved this to Pending Triage in Node.js feature requestson Mar 10, 2023

  6. mhdawson commented on Mar 28, 2023

    @mhdawson
    mhdawson
    on Mar 28, 2023
    Member

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

  7. cjihrig commented on Mar 28, 2023

    @cjihrig
    cjihrig
    on Mar 28, 2023
    Contributor

    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.

  8. cjihrig commented on Mar 29, 2023

    @cjihrig
    cjihrig
    on Mar 29, 2023
    Contributor

    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.

  9. mhdawson commented on Mar 30, 2023

    @mhdawson
    mhdawson
    on Mar 30, 2023
    Member

    @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

  10. tniessen commented on Mar 30, 2023

    @tniessen
    tniessen
    on Mar 30, 2023
    Member

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

  11. cjihrig commented on Mar 30, 2023

    @cjihrig
    cjihrig
    on Mar 30, 2023
    Contributor

    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.

  12. mhdawson commented on Mar 30, 2023

    @mhdawson
    mhdawson
    on Mar 30, 2023
    Member

    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.

  13. mhdawson commented on Mar 30, 2023

    @mhdawson
    mhdawson
    on Mar 30, 2023
    Member

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

  14. targos
    added a commit that references this issue on May 2, 2023

    wasi: make returnOnExit true by default

    75c1d1b

  15. mhdawson commented on May 15, 2023

    @mhdawson
    mhdawson
    on May 15, 2023
    Member

    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.

  16. mhdawson
    closed this as completedon May 15, 2023
  17. avivkeller
    removed this from Node.js feature requestson Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    feature requestIssues requesting new Node.js features.wasiIssues and PRs related to the WebAssembly System Interface.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions