From 4dc2f2af9d845b9d56480899d22d30754fe7bc0a Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Wed, 5 Jun 2024 22:20:22 -0700 Subject: [PATCH] Add signal safety comments and changes --- analysis/runtime/src/handlers.rs | 5 ++++ analysis/runtime/src/runtime/backend.rs | 5 ++-- .../runtime/src/runtime/global_runtime.rs | 5 ++++ .../runtime/src/runtime/scoped_runtime.rs | 14 ++++++++++- analysis/runtime/src/runtime/skip.rs | 23 +++++++++++-------- 5 files changed, 40 insertions(+), 12 deletions(-) diff --git a/analysis/runtime/src/handlers.rs b/analysis/runtime/src/handlers.rs index bbd155a2a1..03b2075f66 100644 --- a/analysis/runtime/src/handlers.rs +++ b/analysis/runtime/src/handlers.rs @@ -2,6 +2,11 @@ use crate::events::{Event, EventKind}; use crate::mir_loc::MirLocId; use crate::runtime::global_runtime::RUNTIME; +// WARNING! Most handlers in this file may be called from a signal handler, +// so they and all their callees should be signal-safe. +// Signal handlers are generally not supposed to call memory allocation +// functions, so those do not need to be signal-safe. + /// A hook function (see [`HOOK_FUNCTIONS`]). /// /// Instruments 64-bit `c2rust transpile`d `malloc`, which is similar to `libc::malloc`. diff --git a/analysis/runtime/src/runtime/backend.rs b/analysis/runtime/src/runtime/backend.rs index 4ea989bee0..1093fae9c6 100644 --- a/analysis/runtime/src/runtime/backend.rs +++ b/analysis/runtime/src/runtime/backend.rs @@ -89,8 +89,9 @@ impl Backend { let event = match events.pop() { Some(event) => event, None => { - // We can't use anything with a futex here since - // the event sender might run inside a signal handler + // We can't block on a lock/semaphore here since + // it might not be safe for the event sender to wake + // us from inside a signal handler backoff.snooze(); continue; } diff --git a/analysis/runtime/src/runtime/global_runtime.rs b/analysis/runtime/src/runtime/global_runtime.rs index 0f4b798935..e85a4e15ec 100644 --- a/analysis/runtime/src/runtime/global_runtime.rs +++ b/analysis/runtime/src/runtime/global_runtime.rs @@ -40,10 +40,15 @@ impl GlobalRuntime { /// /// It also silently drops the [`Event`] if the [`ScopedRuntime`] /// has been [`ScopedRuntime::finalize`]d/[`GlobalRuntime::finalize`]d. + /// + /// May be called from a signal handler, so it needs to be async-signal-safe. pub fn send_event(&self, event: Event) { + // # Async-signal-safety: OnceCell::get() is just a dereference match self.runtime.get() { None => { // Silently drop the [`Event`] as the [`ScopedRuntime`] isn't ready/initialized yet. + // + // # Async-signal-safety: `skip_event(_, BeforeMain)` is async-signal-safe. skip_event(event, SkipReason::BeforeMain); } Some(runtime) => { diff --git a/analysis/runtime/src/runtime/scoped_runtime.rs b/analysis/runtime/src/runtime/scoped_runtime.rs index 98ad024345..f2210d8d42 100644 --- a/analysis/runtime/src/runtime/scoped_runtime.rs +++ b/analysis/runtime/src/runtime/scoped_runtime.rs @@ -103,6 +103,8 @@ impl ExistingRuntime for MainThreadRuntime { self.backend.lock().unwrap().flush(); } + // # Async-signal-safety: NOT SAFE!!! + // Do not use this with programs that install signal handlers. fn send_event(&self, event: Event) { self.backend.lock().unwrap().write(event); } @@ -122,6 +124,10 @@ pub struct BackgroundThreadRuntime { impl BackgroundThreadRuntime { fn push_event(&self, mut event: Event, can_sleep: bool) { + // # Async-signal-safety: This needs `can_sleep == false` if called from + // a signal handler; in that case, it spins instead of sleeping + // which should be safe. `ArrayQueue::push` is backed by a fixed-size + // array so it does not allocate. let backoff = Backoff::new(); while let Err(event_back) = self.tx.push(event) { if can_sleep { @@ -162,10 +168,16 @@ impl ExistingRuntime for BackgroundThreadRuntime { fn send_event(&self, event: Event) { match self.finalized.get() { None => { + // # Async-signal-safety: `push_event` is safe if `can_sleep == false` self.push_event(event, false); } Some(()) => { - // Silently drop the [`Event`] as the [`BackgroundThreadRuntime`] has already been [`BackgroundThreadRuntime::finalize`]d. + // Silently drop the [`Event`] as the [`BackgroundThreadRuntime`] + // has already been [`BackgroundThreadRuntime::finalize`]d. + // + // # Async-signal-safety: `skip_event(_, AfterMain)` is NOT SAFE; + // however, see the comment in `skip_event` for an explanation + // of why this will probably be okay in practice. skip_event(event, SkipReason::AfterMain); } } diff --git a/analysis/runtime/src/runtime/skip.rs b/analysis/runtime/src/runtime/skip.rs index 8fc05d9d37..be4b901b83 100644 --- a/analysis/runtime/src/runtime/skip.rs +++ b/analysis/runtime/src/runtime/skip.rs @@ -1,10 +1,8 @@ use std::{ fmt::{self, Display, Formatter}, - sync::atomic::{AtomicU64, Ordering}, + sync::atomic::{AtomicBool, AtomicU64, Ordering}, }; -use once_cell::sync::OnceCell; - use crate::events::Event; #[derive(Debug)] @@ -24,8 +22,7 @@ impl Display for SkipReason { } static EVENTS_SKIPPED_BEFORE_MAIN: AtomicU64 = AtomicU64::new(0); - -static WARNED_AFTER_MAIN: OnceCell<()> = OnceCell::new(); +static WARNED_AFTER_MAIN: AtomicBool = AtomicBool::new(false); /// Notify the user if any [`Event`]s were skipped before `main`. /// @@ -45,14 +42,22 @@ pub(super) fn skip_event(event: Event, reason: SkipReason) { use SkipReason::*; match reason { BeforeMain => { + // # Async-signal-safety: atomic increments are safe. EVENTS_SKIPPED_BEFORE_MAIN.fetch_add(1, Ordering::Relaxed); } AfterMain => { - // This is after `main`, so it's safe to use things like `eprintln!`, - // which uses `ReentrantMutex` internally, which may use `pthread` mutexes. - WARNED_AFTER_MAIN.get_or_init(|| { + // # Async-signal-safety: not really signal-safe, but if we + // get a signal after `main` ends, we're probably fine. + // The allocator should have enough free memory by now + // to not need to call `mmap`. + if !WARNED_AFTER_MAIN.swap(true, Ordering::Relaxed) { + // WARNED_AFTER_MAIN was previously `false` but we swapped it, + // which will happen exactly once per run so we can print now. eprintln!("skipping {reason}"); - }); + } + // TODO: It would be nice to get rid of the two `eprintln`s here + // so we can guarantee signal safety, but then we would get no + // debugging output. eprintln!("skipped event after `main`: {:?}", event.kind); } };