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

[Ecordonnier]

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

[github-actions]

GNU testsuite comparison:

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

[cakebaker]

Thanks!

[collinfunk]

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]

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]

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]

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]

@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,

[Ecordonnier]

Follow-up PR #9618

[ChrisDryden]

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]

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

[ChrisDryden]

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]

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.

[Ecordonnier]

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().

Btw, I found one trick to make it work with Command::exec(): it is not ideal, but you can restore the correct SIGPIPE signal handler by using Command::pre_exec() which runs after the call to signal(SIGPIPE, SIG_DFL).