utils: use || instead of | in waitpid_ignore_stopped - #2192
Merged
Conversation
WIFSTOPPED() and WIFCONTINUED() are boolean predicates, but POSIX only specifies that they evaluate to a non-zero value for a matching status, not that they evaluate to exactly 1. Combining them with the bitwise OR operator therefore relies on a property the standard does not guarantee. glibc and musl both happen to expand these macros to a comparison, so the current code works there, but the logical OR operator expresses the intent directly and does not depend on the values being normalized. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Erik Sjölund <erik.sjolund@gmail.com>
|
Ephemeral COPR build failed. @containers/packit-build please check. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Claude Opus 5 helped finding the bug and writing this PR.
utils: use
||instead of|inwaitpid_ignore_stoppedWhat
One-line change in
src/libcrun/utils.h:Why
WIFSTOPPED()andWIFCONTINUED()are boolean predicates, but POSIX only requiresthem to evaluate to a non-zero value for a matching status — not to exactly
1.Combining them with
|relies on a normalization the standard does not guarantee.This is in the same spirit as 910eb16 ("utils: normalize
S_ISDIR()result to 0 or 1").Is this a bug fix?
No — and I would rather say so plainly than oversell it. Both glibc and musl expand
these macros to a comparison, so they already yield
0or1and the loop behavescorrectly on every platform crun supports. Both operands are also side-effect-free,
so nothing changes from restoring short-circuit evaluation. Treat this as a
readability and robustness cleanup.
Testing
on glibc and musl.
-Wall -Wextra.waitpid_ignore_stopped()has 12 call sites across
container.c,linux.c,seccomp.c,net_device.cand
utils.c, covering container teardown,exec, and the seccomp notify helper.