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

Apply modulo 256 to BSD WEXITSTATUS #4213

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krobelus
Copy link

@krobelus krobelus commented Dec 19, 2024

wait(2p)1 says

WEXITSTATUS(status)
If WIFEXITED(status) is true, evaluates to the low-order 8 bits
of the argument passed to _exit(2) or exit(3) by the child.

meaning WEXITSTATUS(status) is an 8-bit value. We accidentally return
too many bits. For example WEXITSTATUS(-1) returns -1 instead of 255.

Fix it, matching the C library and our other WEXITSTATUS implementations.

Originally reported at fish-shell/fish-shell#10919

Footnotes

  1. https://manpage.me/index.cgi?apropos=0&q=wait&sektion=2&manpath=FreeBSD+12-CURRENT+and+Ports&arch=default&format=html

wait(2p)[^1] says

> WEXITSTATUS(status)
>         If WIFEXITED(status) is true, evaluates to the low-order 8 bits
>         of the argument passed to _exit(2) or exit(3) by the child.

meaning WEXITSTATUS(status) is an 8-bit value.  We accidentally return
too many bits. For example WEXITSTATUS(-1) returns -1 instead of 255.

Fix it, matching the C library and our other WEXITSTATUS implementations.

[^1] https://manpage.me/index.cgi?apropos=0&q=wait&sektion=2&manpath=FreeBSD+12-CURRENT+and+Ports&arch=default&format=html

Originally reported at fish-shell/fish-shell#10919
@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

krobelus added a commit to fish-shell/fish-shell that referenced this pull request Dec 23, 2024
The libc crate has a bug on BSD where WEXITSTATUS is not an 8-bit
value, causing assertion failures.

Any libc higher than our 0.2.155 would increase our MSRV, see libc
commit 5ddbdc29f (Bump MSRV to 1.71, 2024-01-07), so we want to
woraround this anyway.  It's probably not worth using a patched
version of libc since it's just one line.

While at it, tighten some types I guess.

Upstream fix: rust-lang/libc#4213

Closes #10919
krobelus added a commit to fish-shell/fish-shell that referenced this pull request Dec 23, 2024
The libc crate has a bug on BSD where WEXITSTATUS is not an 8-bit
value, causing assertion failures.

Any libc higher than our 0.2.155 would increase our MSRV, see libc
commit 5ddbdc29f (Bump MSRV to 1.71, 2024-01-07), so we want to
woraround this anyway.  It's probably not worth using a patched
version of libc since it's just one line.

While at it, tighten some types I guess.

Upstream fix: rust-lang/libc#4213

Closes #10919

Cherry-picked from c1b4605
faho pushed a commit to faho/fish-shell that referenced this pull request Dec 26, 2024
The libc crate has a bug on BSD where WEXITSTATUS is not an 8-bit
value, causing assertion failures.

Any libc higher than our 0.2.155 would increase our MSRV, see libc
commit 5ddbdc29f (Bump MSRV to 1.71, 2024-01-07), so we want to
woraround this anyway.  It's probably not worth using a patched
version of libc since it's just one line.

While at it, tighten some types I guess.

Upstream fix: rust-lang/libc#4213

Closes fish-shell#10919
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants