Skip to content

env: use Command::exec() instead of libc::execvp() #9614

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.

Sign up for GitHub

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

Jump to bottom
Merged
cakebaker merged 1 commit into from
Dec 9, 2025
Merged

env: use Command::exec() instead of libc::execvp() #9614

cakebaker merged 1 commit into from
Dec 9, 2025
+15 −38

Conversation

@Ecordonnier
Copy link
Contributor

@Ecordonnier Ecordonnier commented Dec 9, 2025

No need to use the unsafe libc::execvp(), the standard rust library provides the functionality via the safe function Command::exec().

@oech3

This comment was marked as resolved.

Sign in to view
@cakebaker cakebaker changed the title nice: use Command::exec() instead of libc::execvp() env: use Command::exec() instead of libc::execvp() Dec 9, 2025
@Ecordonnier
env: use Command::exec() instead of libc::execvp()
e27da7e 
No need to use the unsafe `libc::execvp()`, the standard rust library provides
the functionality via the safe function `Command::exec()`.

Signed-off-by: Etienne Cordonnier <[email protected]>
@Ecordonnier Ecordonnier mentioned this pull request Dec 9, 2025
Merged
@github-actions
Copy link

github-actions bot commented Dec 9, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@cakebaker cakebaker merged commit 2feb6b9 into uutils:main Dec 9, 2025
127 checks passed
@cakebaker
Copy link
Contributor

cakebaker commented Dec 9, 2025

Thanks!

@Ecordonnier Ecordonnier deleted the eco/env-command-exec branch December 9, 2025 16:43
@collinfunk
Copy link

collinfunk commented Dec 9, 2025

Does Command::exec() do signal (SIGPIPE, SIGPIPE) like the rest of the standard library before executing a command? If so, it is probably better to use libc::execvp().

@collinfunk
Copy link

collinfunk commented Dec 9, 2025

It does:

$ strace -e rt_sigaction,execve ./target/debug/env true
execve("./target/debug/env", ["./target/debug/env", "true"], 0x7ffd9354e808 /* 85 vars */) = 0
rt_sigaction(SIGPIPE, {sa_handler=SIG_IGN, sa_mask=[PIPE], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f73276bc2c0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
rt_sigaction(SIGSEGV, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
rt_sigaction(SIGSEGV, {sa_handler=0x558ebd8bc5d0, sa_mask=[], sa_flags=SA_RESTORER|SA_ONSTACK|SA_SIGINFO, sa_restorer=0x7f73276bc2c0}, NULL, 8) = 0
rt_sigaction(SIGBUS, NULL, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0
rt_sigaction(SIGBUS, {sa_handler=0x558ebd8bc5d0, sa_mask=[], sa_flags=SA_RESTORER|SA_ONSTACK|SA_SIGINFO, sa_restorer=0x7f73276bc2c0}, NULL, 8) = 0
rt_sigaction(SIGSEGV, {sa_handler=SIG_DFL, sa_mask=~[RTMIN RT_1], sa_flags=SA_RESTORER, sa_restorer=0x7f73276bc2c0}, {sa_handler=0x558ebd8bc5d0, sa_mask=[], sa_flags=SA_RESTORER|SA_ONSTACK|SA_SIGINFO, sa_restorer=0x7f73276bc2c0}, 8) = 0
rt_sigaction(SIGBUS, {sa_handler=SIG_DFL, sa_mask=~[RTMIN RT_1], sa_flags=SA_RESTORER, sa_restorer=0x7f73276bc2c0}, {sa_handler=0x558ebd8bc5d0, sa_mask=[], sa_flags=SA_RESTORER|SA_ONSTACK|SA_SIGINFO, sa_restorer=0x7f73276bc2c0}, 8) = 0
rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[PIPE], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f73276bc2c0}, {sa_handler=SIG_IGN, sa_mask=[PIPE], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f73276bc2c0}, 8) = 0
rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[PIPE], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f73276bc2c0}, {sa_handler=SIG_DFL, sa_mask=[PIPE], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f73276bc2c0}, 8) = 0
execve("/home/collin/.local/bin/true", ["true"], 0x7fffa0931f20 /* 85 vars */) = 0

This causes env --ignore-signal=PIPE to be ineffective. The yes command shouldn't receive a SIGPIPE:

$ ./target/debug/env --ignore-signal=PIPE yes | head -n 1
y
$ echo ${PIPESTATUS[0]} ${PIPESTATUS[1]}
141 0

Here is the correct behavior:

$ env --ignore-signal=PIPE yes | head -n 1
y
yes: standard output: Broken pipe
$ echo ${PIPESTATUS[0]} ${PIPESTATUS[1]}
1 0

@Ecordonnier
Copy link
Contributor Author

Ecordonnier commented Dec 9, 2025 •
edited
Loading

OK, thanks for the feedback that Command::exec() calls signal (SIGPIPE, SIG_DFL). I'll check if there is a solution with Command::exec(). Even if we revert this PR, I'll add a test for this corner-case to make it clear why libc::execvp is used.

@collinfunk
Copy link

collinfunk commented Dec 9, 2025

No problem. Rust reseting inherited signal handlers annoys me. I hope this project can give Rust some motivation to change some of that behavior. :)

@Ecordonnier
Copy link
Contributor Author

Ecordonnier commented Dec 9, 2025 •
edited
Loading

@collinfunk rust is already in the process of fixing it, but it's not yet stabilized: rust-lang/rust#97889 (comment) / rust-lang/rust#62569

I'll revert this PR and add a test. I don't think there is a solution preserving ignore_signal using Command::exec() with a stable rust compiler version,

This was referenced Dec 9, 2025
Open
Open
@Ecordonnier
Copy link
Contributor Author

Ecordonnier commented Dec 9, 2025

Follow-up PR #9618

@ChrisDryden
Copy link
Contributor

ChrisDryden commented Dec 9, 2025

We should make some integration tests for this behavior so that it gets locked in. I'll try and implement it tonight. Any chance you have some other examples on how this command stuff could manifest issues?

@Ecordonnier
Copy link
Contributor Author

Ecordonnier commented Dec 9, 2025 •
edited
Loading

I've already added a test in my PR. what do you want to add? The tracking issue is #8919

@ChrisDryden
Copy link
Contributor

ChrisDryden commented Dec 9, 2025

I was seeing there were a few PR in different utilities related to this, was thinking one that tests the behavior for all of the commands that expect this behavior

@collinfunk
Copy link

collinfunk commented Dec 9, 2025

I'm planning to add something to the GNU test suite, but I don't expect a new release soon. So it won't be helpful immediately.

@moonfruit moonfruit mentioned this pull request Dec 15, 2025
Closed
@oech3 oech3 mentioned this pull request Dec 16, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Ecordonnier @oech3 @cakebaker @collinfunk @ChrisDryden