Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow non-portable and signal values as exit statuses. #235

Merged
merged 3 commits into from
Mar 9, 2021

Conversation

sunfishcode
Copy link
Member

Exit codes of at least 256 aren't portable to POSIX exit, so
programs expecting to return full 32-bit Windows System Error Codes
aren't practically portable.

And on POSIX, error codes of at least 128 are reserved for reporting
program exits via signals, and 127 and 126 are reserved for POSIX-style
shells. While it's theoretically possible for POSIX applications to
return these explicitly, this is very rare, not often useful,
particularly in programs intended to be portable, and could potentially
be confusing to users.

If a need arrises for programs to return values in [126,256), or to
provide other kinds of information upon program exit, we can look at
relaxing these restrictions or adding new APIs to WASI for program
termination, but for now it makes sense to start with something simple.

With that, this PR proposes:

  • The WASI exit function takes a u8, but if the value is at
    least 126, it traps. Otherwise it is provided to the environment.
  • WASI libc's exit will map from int to u8 by applying the mask
    as specified in POSIX exit.

No other WASI syscalls trap right now, but exit has no other way to
indicate errors.

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good idea. This kind of restriction could always be loosened down the line if change out mind.

;;; reports successful completion of the program. An exit code of
;;; `$exitcode::failure` or any other value less than 126 reports a
;;; failure, and the value is provided to the environment. If a value
;;; of at least 126 is given, this function behaves as if it were
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the phrases "If a value of at least 126 is given" I little hard to parse. Wouldn't "If a value of 126 or above..", or "If a value greater than or equal to 126.." be more a commonly used phasing.

I noticed this phrases on the PR description and was momentarily confused there too. Is this choice of phrase deliberate, or following some precedent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, I'm reading a book right now that uses "at least" and "at most" instead of "greater than or equal" and "less than or equal". It makes sense in that context, but this PR isn't in that context ;-). I'll change it.

sunfishcode added a commit to sunfishcode/wasmtime that referenced this pull request May 1, 2020
sunfishcode added a commit to sunfishcode/wasmtime that referenced this pull request May 1, 2020
sunfishcode added a commit to sunfishcode/wasmtime that referenced this pull request May 1, 2020
sunfishcode added a commit to sunfishcode/wasmtime that referenced this pull request May 1, 2020
sunfishcode added a commit to sunfishcode/wasmtime that referenced this pull request May 12, 2020
sunfishcode added a commit to sunfishcode/wasmtime that referenced this pull request May 13, 2020
sunfishcode added a commit to bytecodealliance/wasmtime that referenced this pull request May 13, 2020
…rocess. (#1646)

* Remove Cranelift's OutOfBounds trap, which is no longer used.

* Change proc_exit to unwind instead of exit the host process.

This implements the semantics in WebAssembly/WASI#235.

Fixes #783.
Fixes #993.

* Fix exit-status tests on Windows.

* Revert the wiggle changes and re-introduce the wasi-common implementations.

* Move `wasi_proc_exit` into the wasmtime-wasi crate.

* Revert the spec_testsuite change.

* Remove the old proc_exit implementations.

* Make `TrapReason` an implementation detail.

* Allow exit status 2 on Windows too.

* Fix a documentation link.

* Really fix a documentation link.
@peterhuene
Copy link
Contributor

@sunfishcode as Wasmtime is enforcing this restriction for proc_exit, should this get merged into the next WASI snapshot?

Exit codes of at least 256 aren't portable to [POSIX exit], so
programs expecting to return full 32-bit [Windows System Error Codes]
aren't practically portable.

And on POSIX, error codes of at least 128 are reserved for reporting
program exits via signals, and 127 and 126 are reserved for POSIX-style
shells. While it's theoretically possible for POSIX applications to
return these explicitly, this is very rare, not often useful,
particularly in programs intended to be portable, and could potentially
be confusing to users.

If a need arrises for programs to return values in [126,256), or to
provide other kinds of information upon program exit, we can look at
relaxing these restrictions or adding new APIs to WASI for program
termination, but for now it makes sense to start with something simple.

With that, this PR proposes:
 - The WASI `exit` function takes a `u8`, but if the value is at
   least 126, it traps. Otherwise it is provided to the environment.
 - WASI libc's `exit` will map from `int` to `u8` by applying the mask
   as specified in [POSIX exit].

No other WASI syscalls trap right now, but `exit` has no other way to
indicate errors.

[POSIX exit]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/_Exit.html
[Windows System Error Codes]: https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes?redirectedfrom=MSDN#system-error-codes
@sunfishcode
Copy link
Member Author

Yes, let's merge this and aim for the next snapshot.

@sunfishcode sunfishcode merged commit 5472428 into WebAssembly:main Mar 9, 2021
@sunfishcode sunfishcode deleted the exit-codes branch March 9, 2021 18:20
sunfishcode added a commit that referenced this pull request Mar 9, 2021
It appears I didn't fully regenerate the docs after merging #235. This
runs the tools/repo_docs.sh script to regenerate them.
@sunfishcode sunfishcode mentioned this pull request Mar 9, 2021
sunfishcode added a commit that referenced this pull request Mar 9, 2021
It appears I didn't fully regenerate the docs after merging #235. This
runs the tools/repo_docs.sh script to regenerate them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants