diff --git a/ddprof-lib/src/main/cpp/wallClock.cpp b/ddprof-lib/src/main/cpp/wallClock.cpp index ce655f5b1..75a47cbe2 100644 --- a/ddprof-lib/src/main/cpp/wallClock.cpp +++ b/ddprof-lib/src/main/cpp/wallClock.cpp @@ -60,6 +60,7 @@ void WallClockASGCT::sharedSignalHandler(int signo, siginfo_t *siginfo, // would otherwise drive our wallclock sampling path. if (!OS::shouldProcessSignal(siginfo, SI_QUEUE, SignalCookie::wallclock())) { Counters::increment(WALLCLOCK_SIGNAL_FOREIGN); + SIGNAL_HANDLER_GUARD_RELEASE(); OS::forwardForeignSignal(signo, siginfo, ucontext); return; } @@ -234,6 +235,17 @@ void WallClockASGCT::timerLoop() { void WallClockJvmti::sharedSignalHandler(int signo, siginfo_t *siginfo, void *ucontext) { SIGNAL_HANDLER_GUARD(); + // Reject any SIGVTALRM that did not originate from our rt_tgsigqueueinfo + // send (mirrors WallClockASGCT). Defends against stray in-process tgkill or + // external sigqueue driving the JVMTI RequestStackTrace path. + if (!OS::shouldProcessSignal(siginfo, SI_QUEUE, SignalCookie::wallclock())) { + Counters::increment(WALLCLOCK_SIGNAL_FOREIGN); + SIGNAL_HANDLER_GUARD_RELEASE(); + OS::forwardForeignSignal(signo, siginfo, ucontext); + return; + } + Counters::increment(WALLCLOCK_SIGNAL_OWN); + WallClockJvmti *engine = reinterpret_cast(Profiler::instance()->wallEngine()); if (signo == SIGVTALRM) { @@ -284,6 +296,7 @@ void WallClockJvmti::signalHandler(int signo, siginfo_t *siginfo, void WallClockJvmti::initialize(Arguments &args) { // Caller must have verified VM::canRequestStackTrace() before selecting // this engine; see Profiler::selectWallEngine(). + OS::primeSignalOriginCheck(); OS::installSignalHandler(SIGVTALRM, sharedSignalHandler); } @@ -308,7 +321,7 @@ void WallClockJvmti::timerLoop() { auto sampleThreads = [&](int tid, int &num_failures, int &threads_already_exited, int &permission_denied) { - if (!OS::sendSignalToThread(tid, SIGVTALRM)) { + if (!OS::sendSignalWithCookie(tid, SIGVTALRM, SignalCookie::wallclock())) { num_failures++; if (errno != 0) { if (errno == ESRCH) { diff --git a/ddprof-lib/src/test/cpp/signalOrigin_ut.cpp b/ddprof-lib/src/test/cpp/signalOrigin_ut.cpp index a6a1a7e2f..68a55245c 100644 --- a/ddprof-lib/src/test/cpp/signalOrigin_ut.cpp +++ b/ddprof-lib/src/test/cpp/signalOrigin_ut.cpp @@ -12,8 +12,10 @@ #include #include +#include "guards.h" #include "os.h" #include "signalCookie.h" +#include "thread.h" #ifdef __linux__ @@ -333,6 +335,54 @@ TEST_F(SignalOriginTest, ResetForTestingClearsOldactionCache) { sigaction(SIGUSR1, &saved, nullptr); } +// -------- WallclockGuardContract: predicate contract for WallClockJvmti guard -------- +// +// WallClockJvmti::sharedSignalHandler gates on +// OS::shouldProcessSignal(siginfo, SI_QUEUE, SignalCookie::wallclock()) +// These cases cover the three branches the guard must handle: own send, +// bare tgkill/kill (no cookie), and a queued signal with a foreign cookie. + +TEST_F(SignalOriginTest, WallclockGuardContract_OwnSignalAccepted) { + siginfo_t si = makeSiginfo(SI_QUEUE, SignalCookie::wallclock()); + EXPECT_TRUE(OS::shouldProcessSignal(&si, SI_QUEUE, SignalCookie::wallclock())); +} + +TEST_F(SignalOriginTest, WallclockGuardContract_BareKillRejected) { + // tgkill/pthread_kill delivers SI_TKILL; kill/raise delivers SI_USER. + for (int code : {SI_TKILL, SI_USER}) { + siginfo_t si = makeSiginfo(code, nullptr); + EXPECT_FALSE(OS::shouldProcessSignal(&si, SI_QUEUE, SignalCookie::wallclock())) + << "bare signal with si_code " << code << " must be rejected"; + } +} + +TEST_F(SignalOriginTest, WallclockGuardContract_ForeignCookieRejected) { + siginfo_t si = makeSiginfo(SI_QUEUE, (void*)0xF00D); + EXPECT_FALSE(OS::shouldProcessSignal(&si, SI_QUEUE, SignalCookie::wallclock())); +} + +// Regression test for the fix: when a foreign signal is handled, +// SIGNAL_HANDLER_GUARD_RELEASE() must be called before forwardForeignSignal so +// that a chained handler escaping via siglongjmp cannot leave _signal_depth +// permanently incremented. Verifies the SIGNAL_HANDLER_GUARD / release +// contract directly: depth is 0 after an early release, and the destructor is +// a no-op. +TEST_F(SignalOriginTest, WallclockGuardContract_ForeignSignalReleasesGuard) { + ProfiledThread::initCurrentThread(); + EXPECT_EQ(0, getInSignalDepth()); + { + SIGNAL_HANDLER_GUARD(); + EXPECT_EQ(1, getInSignalDepth()); + // Mirrors the fix: release before forwarding so a non-returning + // chained handler cannot leave depth > 0. + SIGNAL_HANDLER_GUARD_RELEASE(); + EXPECT_EQ(0, getInSignalDepth()); + // Destructor runs here; must not double-decrement. + } + EXPECT_EQ(0, getInSignalDepth()); + ProfiledThread::release(); +} + TEST_F(SignalOriginTest, ForwardAppliesSigmaskWhenEnabled) { // Verify that the slow path (DDPROF_FORWARD_APPLY_SIGMASK=1) still // chains to the previous SA_SIGINFO handler correctly.