8000 Disregard si_addr for fatal signals sent from other processes by peterzhu2118 · Pull Request #10201 · ruby/ruby · GitHub
[go: up one dir, main page]
Skip to content

Disregard si_addr for fatal signals sent from other processes #10201

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
merged 1 commit into from
Mar 12, 2024
Merged

Disregard si_addr for fatal signals sent from other processes #10201

merged 1 commit into from
Mar 12, 2024

Conversation

peterzhu2118
Copy link
Member

Previously, when another process sends a fatal signals such as SIGBUS to Ruby, we could mis-interpret it as a stack overflow Ruby itself generated. When the si_pid field is set on the signal, we shouldn't check the si_addr field to check for stack overflow.

Signals sent with kill(2) and sigqueue(3) fill in si_pid and si_uid.

nobu
nobu reviewed Mar 8, 2024
View reviewed changes
signal.c Outdated
@@ -872,9 +872,9 @@ check_stack_overflow(int sig, const void *addr)
# else
# define FAULT_ADDRESS info->si_addr
# ifdef USE_UCONTEXT_REG
# define CHECK_STACK_OVERFLOW() check_stack_overflow(sig, (uintptr_t)FAULT_ADDRESS, ctx)
# define CHECK_STACK_OVERFLOW() if (!info->si_pid) check_stack_overflow(sig, (uintptr_t)FAULT_ADDRESS, ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Could you keep it an expression?

Suggested change
# define CHECK_STACK_OVERFLOW() if (!info->si_pid) check_stack_overflow(sig, (uintptr_t)FAULT_ADDRESS, ctx)
# define CHECK_STACK_OVERFLOW() (info->si_pid ? (void)0 : check_stack_overflow(sig, (uintptr_t)FAULT_ADDRESS, ctx))

@peterzhu2118 @XrXr
Disregard si_addr for fatal signals sent from other processes
10f9d7e 
Previously, when another process sends a fatal signals such as SIGBUS
to Ruby, we could mis-interpret it as a stack overflow Ruby itself
generated. When the si_pid field is set on the signal, we shouldn't
check the si_addr field to check for stack overflow.

> Signals sent with kill(2) and sigqueue(3) fill in si_pid and si_uid.

Co-authored-by: Alan Wu <alanwu@ruby-lang.org>
@launchable-app Launchable app
Copy link
launchable-app bot commented Mar 11, 2024 •
edited
Loading

Launchable Report

❌ Test session #2707587 failed os:ubuntu-22.04 test_opts:cppflags:-DVM_CHECK_MODE test_task:check details on CI
🔔 no issues ✖️1 test failed ✔️25634 tests passed

Passed test sessions

✅ Test session #2707576 passed os:macos-arm-oss test_opts: test_task:check details on CI
✅ Test session #2707578 passed os:macos-arm-oss test_opts:--repeat-count:2 test_task:test-all details on CI
✅ Test session #2707584 passed os:ubuntu-22.04 test_opts:--disable-yjit test_task:check details on CI
✅ Test session #2707585 passed os:ubuntu-22.04 test_opts:--enable-shared--enable-load-relative test_task:check details on CI
✅ Test session #2707586 passed os:ubuntu-20.04 test_opts:cppflags:-DVM_CHECK_MODE test_task:check details on CI
✅ Test session #2707591 passed os:macos-13 test_opts: test_task:check details on CI
✅ Test session #2707597 passed os:ubuntu-22.04 test_opts: test_task:check details on CI
✅ Test session #2707609 passed os:macos-12 test_opts: test_task:check details on CI

Build: refs_pull_10201_merge_10f9d7e686324388bf3fdbc25b0f10ce8355f281

@peterzhu2118 peterzhu2118 merged commit 1e7ee87 into ruby:master Mar 12, 2024
@peterzhu2118 peterzhu2118 deleted the sent-sigbus branch March 12, 2024 13:43
@nobu
Copy link
Member
nobu commented Apr 3, 2024

I guess you are using macOS or Linux because you mentioned SIGBUS.

However, on Linux, siginfo_t uses a union and the field corresponding to si_pid does not belong to the _sigfault field for SIGSEGV and SIGBUS.
It actually overlaps the si_addr field, which is usually non-zero on stack overflow.

In short, the stack overflow check does not work on Linux since this PR.

nobu added a commit to nobu/ruby that referenced this pull request Apr 3, 2024
@nobu
Check if a fatal signal is from kernel
f928e60 
On Linux, `siginfo_t` uses a union for each `si_code`, and the field
corresponding to `si_pid` does not belong to the `_sigfault` field for
SIGSEGV.  It actually overlaps the `si_addr` field, which is usually
non-zero on stack overflow.

ruby#10201 (comment)
@nobu nobu mentioned this pull request Apr 3, 2024
Merged
@XrXr
Copy link
Member
XrXr commented Apr 3, 2024 •
edited
Loading

Ah, thank you. This was for Linux (we send SIGBUS with kill sometimes) but we didn't read the man page carefully enough 🤦🏼

   si_signo, si_errno and si_code are defined for all signals.
   (si_errno is generally unused on Linux.)  The rest of the struct
   may be a union, so that one should read only the fields that are
   meaningful for the given signal:
   
   ...
   Signals sent with kill(2) and sigqueue(3) fill in si_pid and si_uid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nobu nobu nobu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@peterzhu2118 @nobu @XrXr
0