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

DRAFT: Use a noop SIGPIPE handler instead of SIG_IGN #121578

Closed
wants to merge 2 commits into from

Conversation

Enselic
Copy link
Member

@Enselic Enselic commented Feb 25, 2024

Here is an implementation of the proposal to use a noop handler instead of SIG_IGN.

Note that I have based this PR on #121573 which is not merged yet.

I have two questions:

  1. How can code (which might be written in C and called from Rust) and our tests know if SIGPIPE is ignored?
    The oldact return value of sigaction() will now point to a hidden function that will be tricky to interpret by others instead of a well-known handler constant.
assertion `left == right` failed: FIXME: How do we know if SIGPIPE is ignored here?
  left: 139622762777888
 right: 1
  1. Are we sure we want child processes to get SIG_DFL if the Rust runtime has set it to SIG_IGN?
    Personally I currently think the answer is "yes", but in the current implementation, the answer is explicitly and by design "no". I have added this as an open unresolved question for #[unix_sigpipe = "sig_ign"] to the tracking issue.

@rustbot
Copy link
Collaborator

rustbot commented Feb 25, 2024

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 25, 2024
@Enselic Enselic changed the title DRAFT: Use a noop SIGPIPE handler instead of SIG_IGN DRAFT: Use a noop SIGPIPE handler instead of SIG_IGN Feb 25, 2024
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-16 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=Enselic
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_bcfd2505-6ef1-4d94-9883-ae208918d31e
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=sigpipe-noop
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_bcfd2505-6ef1-4d94-9883-ae208918d31e
GITHUB_REF=refs/pull/121578/merge
GITHUB_REF_NAME=121578/merge
GITHUB_REF_PROTECTED=false
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=552a371d093de7a40eaf82295d20e4339f292bfd
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_bcfd2505-6ef1-4d94-9883-ae208918d31e
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_bcfd2505-6ef1-4d94-9883-ae208918d31e
GITHUB_TRIGGERING_ACTOR=Enselic
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/121578/merge
GITHUB_WORKFLOW_SHA=552a371d093de7a40eaf82295d20e4339f292bfd
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_19_X64=/opt/hostedtoolcache/go/1.19.13/x64
---
#12 writing image sha256:a1b7042c748c5fa0b5e16f74d3b30dee7435f1c63245612efcebce3a82065561 done
#12 naming to docker.io/library/rust-ci done
#12 DONE 10.0s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-16]
##[group]Clock drift check
  local time: Sun Feb 25 07:00:56 UTC 2024
  network time: Sun, 25 Feb 2024 07:00:56 GMT
  network time: Sun, 25 Feb 2024 07:00:56 GMT
##[endgroup]
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-16', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'build.optimized-compiler-builtins', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-16/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
........................................................................................   792/16211
........................................................................................   880/16211
........................................................................................   968/16211
...................................................................................i....  1056/16211
.....................................................................F........F...F.....  1144/16211
........................................................................................  1320/16211
........................................................................................  1408/16211
........................................................................................  1496/16211
........................................................................................  1584/16211
---
status: exit status: 101
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/attributes/unix_sigpipe/unix_sigpipe-error" && RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/attributes/unix_sigpipe/unix_sigpipe-error/a"
stdout: none
--- stderr -------------------------------
thread 'main' panicked at /checkout/tests/ui/attributes/unix_sigpipe/auxiliary/sigpipe-utils.rs:26:9:
assertion `left == right` failed: FIXME: How do we know if SIGPIPE is ignored here?
 right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
------------------------------------------

---
status: exit status: 101
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/attributes/unix_sigpipe/unix_sigpipe-only-feature" && RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/attributes/unix_sigpipe/unix_sigpipe-only-feature/a"
stdout: none
--- stderr -------------------------------
thread 'main' panicked at /checkout/tests/ui/attributes/unix_sigpipe/auxiliary/sigpipe-utils.rs:26:9:
assertion `left == right` failed: FIXME: How do we know if SIGPIPE is ignored here?
 right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
------------------------------------------

---
status: exit status: 101
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/attributes/unix_sigpipe/unix_sigpipe-not-used" && RUST_TEST_THREADS="16" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/attributes/unix_sigpipe/unix_sigpipe-not-used/a"
stdout: none
--- stderr -------------------------------
thread 'main' panicked at /checkout/tests/ui/attributes/unix_sigpipe/auxiliary/sigpipe-utils.rs:26:9:
assertion `left == right` failed: FIXME: How do we know if SIGPIPE is ignored here?
 right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
------------------------------------------

target_os = "fuchsia",
target_os = "horizon",
)))]
extern "C" fn _rustc_sigaction_noop(_: libc::c_int) {}
Copy link
Member

@bjorn3 bjorn3 Feb 26, 2024

Choose a reason for hiding this comment

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

Maybe put this next to the sa_sigaction assignment?

@bors
Copy link
Contributor

bors commented Feb 26, 2024

☔ The latest upstream changes (presumably #121627) made this pull request unmergeable. Please resolve the merge conflicts.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 10, 2024
…ark-Simulacrum

unix_sigpipe: Add test for SIGPIPE disposition in child processes

To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578).

Part of rust-lang#97889
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 11, 2024
…ark-Simulacrum

unix_sigpipe: Add test for SIGPIPE disposition in child processes

To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578).

Part of rust-lang#97889
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 30, 2024
…ark-Simulacrum

unix_sigpipe: Add test for SIGPIPE disposition in child processes

To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578).

Part of rust-lang#97889
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 30, 2024
…ark-Simulacrum

unix_sigpipe: Add test for SIGPIPE disposition in child processes

To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578).

Part of rust-lang#97889
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 30, 2024
Rollup merge of rust-lang#121573 - Enselic:sigpipe-child-process, r=Mark-Simulacrum

unix_sigpipe: Add test for SIGPIPE disposition in child processes

To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578).

Part of rust-lang#97889
@Enselic
Copy link
Member Author

Enselic commented Apr 1, 2024

I will close this for now as this is not something we will merge in the near future. I have an updated local branch that I probably will resume work on at a later point.

@Enselic Enselic closed this Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants