Skip to content

Newtype SigalNum #8096

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

Merged
youknowone merged 13 commits into from
Jun 15, 2026
Merged

Newtype SigalNum #8096

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7ce5376
Newtype SigalNum
ShaharNaveh Jun 11, 2026
89a812c
Merge remote-tracking branch 'upstream/main' into signals-newtype
ShaharNaveh Jun 14, 2026
86383c6
Improved doc
ShaharNaveh Jun 14, 2026
19fbd50
impl Index<SigalNum> for SignalHandlersInner
ShaharNaveh Jun 14, 2026
d2a7331
clippy
ShaharNaveh Jun 14, 2026
7689418
Allow dead code
ShaharNaveh Jun 14, 2026
eef6f26
fix lint
ShaharNaveh Jun 14, 2026
b9ed125
fix typo
ShaharNaveh Jun 14, 2026
72efa5d
Merge remote-tracking branch 'upstream/main' into signals-newtype
ShaharNaveh Jun 14, 2026
52a1efd
More fixes
ShaharNaveh Jun 14, 2026
68f8a3a
fix
ShaharNaveh Jun 14, 2026
a7a355a
Fix for windows
ShaharNaveh Jun 14, 2026
bbf319b
coderabbit suggestions
ShaharNaveh Jun 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
impl Index<SigalNum> for SignalHandlersInner
  • Loading branch information
@ShaharNaveh
ShaharNaveh committed Jun 14, 2026
commit 19fbd50968da72643478495b2dd992525dbfde97
41 changes: 32 additions & 9 deletions crates/vm/src/signal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use core::{
cell::{Cell, RefCell},
fmt,
ops::{Deref, DerefMut, Range},
ops::{Deref, DerefMut, Index, IndexMut, Range},
sync::atomic::{AtomicBool, Ordering},
};
use std::sync::mpsc;
Expand Down Expand Up @@ -32,10 +32,6 @@ thread_local! {
static IN_SIGNAL_HANDLER: Cell<bool> = const { Cell::new(false) };
}

pub(crate) fn new_signal_handlers() -> Box<RefCell<[Option<PyObjectRef>; NSIG]>> {
Box::new(const { RefCell::new([const { None }; NSIG]) })
}

struct SignalHandlerGuard;

impl Drop for SignalHandlerGuard {
Expand Down Expand Up @@ -79,11 +75,16 @@ fn trigger_signals(vm: &VirtualMachine) -> PyResult<()> {
let signal_handlers = vm.signal_handlers.get().unwrap().borrow();
for (signum, trigger) in TRIGGERS.iter().enumerate().skip(1) {
let triggered = trigger.swap(false, Ordering::Relaxed);

let signum = (signum as i32)
.try_into()
.expect("TRIGGERS has the same length as the signal_handlers");

if triggered
&& let Some(handler) = &signal_handlers[signum]
&& let Some(callable) = handler.to_callable()
{
callable.invoke((signum, vm.ctx.none()), vm)?;
callable.invoke((signum.as_i32(), vm.ctx.none()), vm)?;
Comment on lines +74 to +90

@coderabbitai coderabbitai Bot Jun 14, 2026

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.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Drop the signal_handlers borrow before invoking Python.

signal_handlers.borrow() now lives across callable.invoke(...). If the Python signal handler calls back into _signal.signal() to replace a handler, crates/vm/src/stdlib/_signal.rs, Lines 222-255, will hit borrow_mut() on the same RefCell and panic on a valid delivery path. Copy out the PyObjectRef (or callable) inside a short borrow scope, then invoke it after the borrow is dropped.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/vm/src/signal.rs` around lines 74 - 90, The `signal_handlers` borrow
from `signal_handlers.borrow()` is held across the `callable.invoke()` call,
which causes a panic if the Python signal handler tries to call back into
`_signal.signal()` to modify signal handlers due to a conflict on the same
`RefCell`. Restructure the code to extract the handler and convert it to a
callable inside a short borrow scope, then explicitly drop the borrow before
invoking the callable. This can be achieved by moving the handler lookup, the
callable extraction, and the invoke call outside the scope of the borrow, or by
using a scoped block that ensures the borrow is released before the invoke call
happens.

}
}
if let Some(signal_rx) = &vm.signal_rx {
Expand Down Expand Up @@ -257,16 +258,38 @@ pub fn get_sigint_event() -> Option<isize> {
if handle == 0 { None } else { Some(handle) }
}

pub struct SignalHandlers(Box<RefCell<[Option<PyObjectRef>; NSIG]>>);
pub struct SignalHandlersInner([Option<PyObjectRef>; NSIG]);

impl Default for SignalHandlersInner {
fn default() -> Self {
Self([const { None }; NSIG])
}
}

impl Index<SignalNum> for SignalHandlersInner {
type Output = Option<PyObjectRef>;

fn index(&self, index: SignalNum) -> &Self::Output {
&self.0[index.as_usize()]
}
}

impl IndexMut<SignalNum> for SignalHandlersInner {
fn index_mut(&mut self, index: SignalNum) -> &mut Self::Output {
&mut self.0[index.as_usize()]
}
}

pub struct SignalHandlers(Box<RefCell<SignalHandlersInner>>);

impl Default for SignalHandlers {
fn default() -> Self {
Self(Box::new(const { RefCell::new([const { None }; NSIG]) }))
Self(Box::new(RefCell::new(SignalHandlersInner::default())))
}
}

impl Deref for SignalHandlers {
type class="x x-first x-last">[Option<PyObjectRef>; NSIG]>>;
type class="x x-first x-last">SignalHandlersInner>>;

fn deref(&self) -> &Self::Target {
&self.0
Expand Down
11 changes: 7 additions & 4 deletions crates/vm/src/stdlib/_signal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ pub(crate) mod _signal {

use crate::{
Py, PyObjectRef, PyResult, VirtualMachine,
signal::{self, SignalNum},
signal::{self, SignalHandlers, SignalNum},
};
use core::{
ops::Range,
Expand Down Expand Up @@ -198,9 +198,12 @@ pub(crate) mod _signal {
None
};

// SAFETY: Trust `SIGNUM_RANGE`
let signum = unsafe { SignalNum::new_unchecked(signum) };

vm.signal_handlers
.get_or_init(SignalHandlers::default)
.borrow_mut()[signum as usize] = py_handler;
.borrow_mut()[signum] = py_handler;
}

let int_handler = module
Expand Down Expand Up @@ -252,14 +255,14 @@ pub(crate) mod _signal {
.map_err(|_| vm.new_os_error("Failed to set signal"))?;

let signal_handlers = vm.signal_handlers.get_or_init(SignalHandlers::default);
let old_handler = signal_handlers.borrow_mut()[signalnum as usize].replace(handler);
let old_handler = signal_handlers.borrow_mut()[signalnum].replace(handler);
Ok(old_handler)
}

#[pyfunction]
fn getsignal(signalnum: SignalNum, vm: &VirtualMachine) -> PyResult {
let signal_handlers = vm.signal_handlers.get_or_init(SignalHandlers::default);
let handler = signal_handlers.borrow()[signalnum.as_usize()]
let handler = signal_handlers.borrow()[signalnum]
.clone()
.unwrap_or_else(|| vm.ctx.none());
Ok(handler)
Expand Down
Loading