Validate signal origin in CPU and WallClock handlers (PROF-14324) (#494)

Author: jbachorik

ddprof-lib/src/main/cpp/counters.h

@@ -98,7 +98,11 @@
   X(WALKVM_CONT_ENTRY_NULL,     "walkvm_cont_entry_null")                      \
   X(NATIVE_LIBS_DROPPED, "native_libs_dropped")                                \
   X(SIGACTION_PATCHED_LIBS, "sigaction_patched_libs")                          \
-  X(SIGACTION_INTERCEPTED, "sigaction_intercepted")
+  X(SIGACTION_INTERCEPTED, "sigaction_intercepted")                             \
+  X(CTIMER_SIGNAL_OWN, "ctimer_signal_own")                                     \
+  X(CTIMER_SIGNAL_FOREIGN, "ctimer_signal_foreign")                             \
+  X(WALLCLOCK_SIGNAL_OWN, "wallclock_signal_own")                               \
+  X(WALLCLOCK_SIGNAL_FOREIGN, "wallclock_signal_foreign")
 #define X_ENUM(a, b) a,
 typedef enum CounterId : int {
   DD_COUNTER_TABLE(X_ENUM) DD_NUM_COUNTERS

ddprof-lib/src/main/cpp/ctimer_linux.cpp

@@ -17,15 +17,21 @@
 
 #ifdef __linux__
 
+#include "counters.h"
 #include "guards.h"
 #include "ctimer.h"
 #include "debugSupport.h"
 #include "jvmThread.h"
 #include "libraries.h"
+#include "log.h"
 #include "profiler.h"
+#include "signalCookie.h"
 #include "threadState.inline.h"
 #include <assert.h>
+#include <errno.h>
+#include <stddef.h>
 #include <stdlib.h>
+#include <string.h>
 #include <sys/syscall.h>
 #include <time.h>
 #include <unistd.h>
@@ -53,9 +59,25 @@ int CTimer::registerThread(int tid) {
   }
 
   struct sigevent sev;
-  sev.sigev_value.sival_ptr = NULL;
+  // Zero the whole struct first so any padding / future fields the kernel
+  // inspects (sigev_notify_function, sigev_notify_attributes on glibc) are
+  // not populated from stack garbage.
+  memset(&sev, 0, sizeof(sev));
+  // Cookie identifying this timer as ddprof-owned. When the signal is delivered
+  // the handler checks siginfo->si_value.sival_ptr against SignalCookie::cpu()
+  // and drops/forwards any SIGPROF that does not carry it (e.g. from a Go
+  // runtime's setitimer(ITIMER_PROF) or a foreign library's raise()).
+  sev.sigev_value.sival_ptr = SignalCookie::cpu();
   sev.sigev_signo = _signal;
   sev.sigev_notify = SIGEV_THREAD_ID;
+  // glibc/musl layout convention: sigev_notify_thread_id sits immediately
+  // after sigev_notify inside the union — the tid is written as the *second*
+  // int starting at &sev.sigev_notify, so bytes [sizeof(int), 2*sizeof(int))
+  // of that int-pointer must be in-bounds of struct sigevent. Guard against
+  // a future libc change by statically asserting that both ints fit.
+  static_assert(offsetof(struct sigevent, sigev_notify) + 2 * sizeof(int)
+                    <= sizeof(struct sigevent),
+                "sigevent layout assumption broken: tid write would overflow");
   ((int *)&sev.sigev_notify)[1] = tid;
 
   // Use raw syscalls, since libc wrapper allows only predefined clocks
@@ -76,7 +98,21 @@ int CTimer::registerThread(int tid) {
   ts.it_interval.tv_sec = (time_t)(_interval / 1000000000);
   ts.it_interval.tv_nsec = _interval % 1000000000;
   ts.it_value = ts.it_interval;
-  syscall(__NR_timer_settime, timer, 0, &ts, NULL);
+  if (syscall(__NR_timer_settime, timer, 0, &ts, NULL) < 0) {
+    // Arming failed after publishing the timer in _timers[tid]. Reclaim the
+    // slot only if it still contains this timer; otherwise a concurrent
+    // unregisterThread(tid) has already claimed responsibility for cleanup
+    // (avoids a double timer_delete).
+    int settime_errno = errno;
+    char errbuf[64];
+    strerror_r(settime_errno, errbuf, sizeof(errbuf));
+    Log::warn("timer_settime failed for tid=%d: %s", tid, errbuf);
+    errno = settime_errno;
+    if (__sync_bool_compare_and_swap(&_timers[tid], timer + 1, 0)) {
+      syscall(__NR_timer_delete, timer);
+    }
+    return -1;
+  }
   return 0;
 }
 
@@ -119,6 +155,12 @@ Error CTimer::start(Arguments &args) {
     _max_timers = max_timers;
   }
 
+  // Prime the origin-check cache from this non-signal context before any
+  // SIGPROF can fire — reading the env var lazily from the handler itself
+  // would go through a C++ function-local-static guard, which is not
+  // async-signal-safe.
+  OS::primeSignalOriginCheck();
+
   OS::installSignalHandler(_signal, signalHandler);
 
   // Register all existing threads
@@ -143,6 +185,17 @@ void CTimer::stop() {
 }
 
 void CTimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) {
+  // Reject signals that did not originate from our timer_create timers.
+  // This guards against Go's process-wide setitimer(ITIMER_PROF) and other
+  // foreign SIGPROF sources that would otherwise drive our handler onto
+  // threads we never registered — see doc/plans/SignalOriginValidation.md.
+  if (!OS::shouldProcessSignal(siginfo, SI_TIMER, SignalCookie::cpu())) {
+    Counters::increment(CTIMER_SIGNAL_FOREIGN);
+    OS::forwardForeignSignal(signo, siginfo, ucontext);
+    return;
+  }
+  Counters::increment(CTIMER_SIGNAL_OWN);
+
   // Atomically try to enter critical section - prevents all reentrancy races
   CriticalSection cs;
   if (!cs.entered()) {

ddprof-lib/src/main/cpp/itimer.cpp

@@ -30,6 +30,12 @@ long ITimer::_interval;
 CStack ITimer::_cstack;
 
 void ITimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) {
+  // NOTE: ITimer uses setitimer(ITIMER_PROF) which delivers signals with
+  // si_code==SI_KERNEL — no sival payload is available. The signal-origin
+  // check implemented in CTimer/WallClock cannot be applied here. ITimer
+  // is therefore vulnerable to the foreign-SIGPROF deadlock scenario this
+  // feature addresses. Use CTimer (the default) when signal-origin
+  // validation is required.
   if (!_enabled)
     return;
 

ddprof-lib/src/main/cpp/os.h

@@ -122,11 +122,73 @@ class OS {
     static bool isLinux();
     static bool isMusl();
 
+    // Returns nullptr on sigaction() failure; returns the previous sa_sigaction otherwise.
     static SigAction installSignalHandler(int signo, SigAction action, SigHandler handler = NULL);
     static SigAction replaceCrashHandler(SigAction action);
     static int getProfilingSignal(int mode);
     static bool sendSignalToThread(int thread_id, int signo);
 
+    // Send a signal to a specific thread with a cookie payload (synchronous —
+    // the kernel queues it and returns). Uses rt_tgsigqueueinfo(2) on Linux;
+    // receiver sees si_code == SI_QUEUE with the cookie in si_value.sival_ptr.
+    // Engines use this when they need to discriminate their own signals from
+    // foreign ones in the handler.
+    static bool sendSignalWithCookie(int thread_id, int signo, void* cookie);
+
+    // Accept-policy for a signal in a handler context. Async-signal-safe.
+    //
+    // Returns true iff the signal SHOULD be processed by the caller's engine:
+    //   - origin check disabled (accept-all fallback, DDPROF_SIGNAL_ORIGIN_CHECK=0)
+    //     → true
+    //   - origin check enabled AND siginfo matches both expected_si_code and
+    //     expected_cookie → true
+    //   - otherwise → false
+    //
+    // Naming note: this is a policy predicate, NOT a pure "did this signal
+    // come from us?" identification. A `false` answer means "forward this to
+    // the chained handler"; a `true` answer means "process normally".
+    // `expected_si_code` is SI_TIMER (timer_create) or SI_QUEUE
+    // (sendSignalWithCookie).
+    static bool shouldProcessSignal(siginfo_t* siginfo, int expected_si_code, void* expected_cookie);
+
+    // Forwards a signal we decided not to process to the previously-installed
+    // handler (as captured by installSignalHandler). When
+    // DDPROF_FORWARD_APPLY_SIGMASK=1 is set, also reproduces the previous
+    // handler's sa_mask so the chained handler sees the same signal-blocking
+    // environment it would under normal kernel delivery.
+    // Async-signal-safe on the fast path (no sa_mask applied). When
+    // DDPROF_FORWARD_APPLY_SIGMASK=1 is set, the slow path uses raw
+    // rt_sigprocmask syscalls which are async-signal-safe. The forwarded
+    // handler's safety is the caller's concern.
+    //
+    // Drop semantics (Linux): signals whose previous disposition was SIG_DFL or
+    // SIG_IGN are silently dropped — kernel-default termination is NOT reproduced
+    // (terminating the process on every forwarded foreign signal would be worse
+    // than ignoring it). Callers must not rely on default-action reproduction.
+    //
+    // macOS: always a no-op (rt_tgsigqueueinfo is unavailable so no cookie
+    // discrimination is possible; all signals are accepted by the engine handler
+    // and never forwarded). signalOriginCheckEnabled() returns false on macOS.
+    static void forwardForeignSignal(int signo, siginfo_t* siginfo, void* ucontext);
+
+    // Runtime feature flag: is the signal origin check active? Reads
+    // DDPROF_SIGNAL_ORIGIN_CHECK env var and caches the result. Default on;
+    // set to "false"/"0"/"off"/"no" to disable (regression tests only).
+    // Callers running from signal-handler context must ensure
+    // primeSignalOriginCheck() was called earlier from non-signal context
+    // so the env-var getenv() cost is not paid during signal delivery.
+    static bool signalOriginCheckEnabled();
+
+    // Prime the signalOriginCheckEnabled() cache from non-signal context.
+    // Called by engine start() paths before installing handlers so later
+    // env-var overrides are picked up. Safe to call multiple times; no-op
+    // after the first call unless forceReload=true (used only by unit tests).
+    //
+    // Without priming, the cache holds its safe defaults (origin check
+    // enabled, sigmask chain disabled) — signals still classify correctly
+    // even if priming never runs.
+    static void primeSignalOriginCheck(bool forceReload = false);
+
     static void* safeAlloc(size_t size);
     static void safeFree(void* addr, size_t size);