Skip to content

Fix forced signal delivery - #1131

Merged
Sangho Lee (sangho2) merged 2 commits into
mainfrom
sanghle/fix_signal
Aug 17, 2026
Merged

Fix forced signal delivery#1131
Sangho Lee (sangho2) merged 2 commits into
mainfrom
sanghle/fix_signal

Conversation

@sangho2

@sangho2 Sangho Lee (sangho2) commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

This PR fixes forced delivery of a signal. Two issues fixed:

  1. handle_exception_request covers SIGFPE, SIGTRAP, and SIGILL, but force_signal_with_info asserts the signal is SIGKILL or SIGSEGV.
  2. siginfo_exception is used to return SigInfo with a fixed code (SI_KERNEL), ignoring other codes like FPE_INTDIV and ILL_ILLOPN.

@CvvT Weiteng Chen (CvvT) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks!

@wdcui Weidong Cui (wdcui) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't find any issue, but the agent reported the following issue:

This PR makes SIGFPE/SIGILL delivery reachable, but  siginfo_exception()  still emits  SI_KERNEL  with address  0 . Linux handlers expect  FPE_INTDIV  or  ILL_ILLOPN  and the faulting RIP. The fix must also bypass  RLIMIT_SIGPENDING  explicitly for forced signals; simply changing  si_code  would allow these signals to be dropped. SIGTRAP can retain  SI_KERNEL .

@sangho2 Sangho Lee (sangho2) added the must-not-merge:undergoing-restructuring Known deeper set of changes are happening on this PR before it is mergeable again label Aug 12, 2026
@sangho2

Copy link
Copy Markdown
Contributor Author

I didn't find any issue, but the agent reported the following issue:

This PR makes SIGFPE/SIGILL delivery reachable, but  siginfo_exception()  still emits  SI_KERNEL  with address  0 . Linux handlers expect  FPE_INTDIV  or  ILL_ILLOPN  and the faulting RIP. The fix must also bypass  RLIMIT_SIGPENDING  explicitly for forced signals; simply changing  si_code  would allow these signals to be dropped. SIGTRAP can retain  SI_KERNEL .

The review makes sense, which would bump up this one-line change though. Let me figure out minimal change.

@sangho2 Sangho Lee (sangho2) removed the must-not-merge:undergoing-restructuring Known deeper set of changes are happening on this PR before it is mergeable again label Aug 17, 2026
@sangho2
Sangho Lee (sangho2) force-pushed the sanghle/fix_signal branch 2 times, most recently from 607ab1b to bdfef4c Compare August 17, 2026 17:15
@sangho2

Sangho Lee (sangho2) commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

I didn't find any issue, but the agent reported the following issue:
This PR makes SIGFPE/SIGILL delivery reachable, but  siginfo_exception()  still emits  SI_KERNEL  with address  0 . Linux handlers expect  FPE_INTDIV  or  ILL_ILLOPN  and the faulting RIP. The fix must also bypass  RLIMIT_SIGPENDING  explicitly for forced signals; simply changing  si_code  would allow these signals to be dropped. SIGTRAP can retain  SI_KERNEL .

The review makes sense, which would bump up this one-line change though. Let me figure out minimal change.

Added a minimal fix to return FPE_INTDIV and ILL_ILLOPN based on Signal. Left TODO because this is not fine-grained (additional machinery is needed).

@github-actions

Copy link
Copy Markdown

🤖 SemverChecks 🤖 No breaking API changes detected

Note: this does not mean API is unchanged, or even that there are no breaking changes; simply, none of the detections triggered.

@sangho2
Sangho Lee (sangho2) added this pull request to the merge queue Aug 17, 2026
Merged via the queue into main with commit a5e9ae2 Aug 17, 2026
14 checks passed
@sangho2
Sangho Lee (sangho2) deleted the sanghle/fix_signal branch August 17, 2026 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants