From dc9d91b2813a11815cc1cddf2bb3f9aeee2fe8e1 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Wed, 30 Sep 2026 10:52:10 +0200 Subject: [PATCH 1/3] fix: guard attributionPC against null pc (UBSan) A null walking pc can reach attributionPC with pc_is_return_address=true when an optimistic unwind reads a zeroed return-address slot. Pointer arithmetic on nullptr is UB and UBSan (asan nightly config) aborts the test JVM on it. A null pc has no code to attribute either way, so pass it through unchanged. --- ddprof-lib/src/main/cpp/stackWalker.inline.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/ddprof-lib/src/main/cpp/stackWalker.inline.h b/ddprof-lib/src/main/cpp/stackWalker.inline.h index 965f554cfc..13b67434ab 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.inline.h +++ b/ddprof-lib/src/main/cpp/stackWalker.inline.h @@ -61,7 +61,15 @@ inline void fillFrame(ASGCT_CallFrame& frame, FrameTypeId type, int bci, jmethod // rescues only the padding-gap and zero-size-symbol cases, not zero-gap // adjacency (see BinarySearchPicksNextSymbolAtZeroGapBoundary). inline const void* attributionPC(const void* pc, bool pc_is_return_address) { - return pc_is_return_address ? (const void*)((const char*)pc - 1) : pc; + // A null walking pc (a zeroed return-address slot read during an optimistic + // unwind) has no code to attribute: findLibraryByAddress(nullptr) fails and + // no FDE matches, adjusted or not. Pointer arithmetic on nullptr is UB + // (UBSan: "applying non-zero offset to null pointer"), so pass it through + // unchanged -- the raw null is exactly what the pre-adjustment walkers used + // for lookup anyway. + return pc != nullptr && pc_is_return_address + ? (const void*)((const char*)pc - 1) + : pc; } // The walking pc and "was it loaded from a return-address slot?" are one From d6a04bf902bfc36daf42c9f14e30719dc3e7e2ef Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Wed, 30 Sep 2026 10:52:10 +0200 Subject: [PATCH 2/3] fix: use CallTraceSet in fuzz_callTraceStorage processTraces lambda 9010c4c3a switched CallTraceSet to CountingAllocator; the harness lambda still declared std::unordered_set with the default allocator, which is not convertible to std::function --- ddprof-lib/src/test/fuzz/fuzz_callTraceStorage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddprof-lib/src/test/fuzz/fuzz_callTraceStorage.cpp b/ddprof-lib/src/test/fuzz/fuzz_callTraceStorage.cpp index 091780f6d9..592f7d2ae3 100644 --- a/ddprof-lib/src/test/fuzz/fuzz_callTraceStorage.cpp +++ b/ddprof-lib/src/test/fuzz/fuzz_callTraceStorage.cpp @@ -77,7 +77,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { } else if (op < 0xC0) { // processTraces() — verify I1 and I2 std::unordered_set seen; - g_storage->processTraces([&](const std::unordered_set& traces) { + g_storage->processTraces([&](const CallTraceSet& traces) { for (CallTrace* t : traces) { if (t) seen.insert(t->trace_id); } From fbfa8795699909d6abf653d2e5cc72ee903d4467 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Wed, 30 Sep 2026 10:52:10 +0200 Subject: [PATCH 3/3] test: assert label on the dump that fired jmethodid_skipped_count The counter delta spans the whole churn window (dumps and background JFR flushes) while the label assertion read only the last dump file; a stale trace can be evicted from the call-trace storage before the final dump, making the test flaky across JDKs/platforms. Snapshot the dump whose window observed the counter crossing and assert on it; if the counter only fired between dumps, take one more dump before stop. --- .../JMethodIDInvalidationStressTest.java | 47 ++++++++++++++++--- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java index 443fbd635b..64ff214cdf 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java @@ -30,6 +30,7 @@ import java.lang.reflect.Method; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -110,6 +111,7 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti Path baseFile = tempFile("jmethodid-churn-base"); Path dumpFile = tempFile("jmethodid-churn-dump"); + Path firedDumpFile = null; AtomicBoolean running = new AtomicBoolean(true); List churnThreads = new ArrayList<>(); @@ -148,9 +150,23 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti // with class unloading, not just after it. long deadline = System.currentTimeMillis() + DURATION_MILLIS; int dumps = 0; + long prevSkipped = before.getOrDefault("jmethodid_skipped_count", 0L); while (System.currentTimeMillis() < deadline) { profiler.dump(dumpFile); dumps++; + // The stale-jmethodID branch (JMETHODID_SKIPPED) and the '' label it + // serializes are produced in the same fillJavaMethodInfo call, so a dump whose + // window observed the counter crossing is guaranteed to carry the branch's + // output. Snapshot it: the assertion cannot use just the last dump file, since + // the counter also fires during background JFR buffer flushes between dumps, + // and a stale trace still present at an early dump can be evicted from the + // call-trace storage by later churn before the final dump is written. + long skipped = profiler.getDebugCounters().getOrDefault("jmethodid_skipped_count", 0L); + if (firedDumpFile == null && skipped > prevSkipped) { + firedDumpFile = tempFile("jmethodid-churn-fired"); + Files.copy(dumpFile, firedDumpFile, StandardCopyOption.REPLACE_EXISTING); + } + prevSkipped = skipped; Thread.sleep(50); } @@ -162,6 +178,21 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti // Reaching this line means the profiler survived the whole churn window. Map after = profiler.getDebugCounters(); + long skippedDelta = after.getOrDefault("jmethodid_skipped_count", 0L) + - before.getOrDefault("jmethodid_skipped_count", 0L); + long unreadableLineTableDelta = after.getOrDefault("line_number_table_unreadable", 0L) + - before.getOrDefault("line_number_table_unreadable", 0L); + + // If the counter only fired during background JFR buffer flushes between dumps, + // no dump window observed it and no snapshot was taken. One more dump + // re-serializes any trace still carrying that stale jmethodID -- its + // '' MethodInfo is cached from the first resolution -- so the label + // assertion below stays checkable. + if (skippedDelta > 0 && firedDumpFile == null) { + firedDumpFile = tempFile("jmethodid-churn-fired"); + profiler.dump(firedDumpFile); + } + profiler.stop(); assertTrue(Files.size(dumpFile) > 0, @@ -174,11 +205,6 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti + "can't tell 'no stale jmethodID was hit' apart from 'churn never ran at all' " + "(e.g. a regression in generateChurnClassBytecode or IsolatedClassLoader)."); - long skippedDelta = after.getOrDefault("jmethodid_skipped_count", 0L) - - before.getOrDefault("jmethodid_skipped_count", 0L); - long unreadableLineTableDelta = after.getOrDefault("line_number_table_unreadable", 0L) - - before.getOrDefault("line_number_table_unreadable", 0L); - // Whether the stale-jmethodID race actually gets hit within the churn window is // JVM/host-discretionary (see class javadoc); treat "never observed" as an aborted // run rather than a failure so a healthy host that just didn't race tightly enough @@ -206,7 +232,10 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti + " JVMTI-resolution-failure branch ran; skipping to avoid a spurious failure."); // Assert that the recording produced by that branch uses the '' label and // never the legacy 'jvmtiError' one -- this fails if the label is reverted to 'jvmtiError'. - assertUnloadedFrameLabel(dumpFile); + // With skippedDelta > 0, firedDumpFile is always set: either the dump whose window + // observed the counter crossing (label emitted in that same fillJavaMethodInfo call) + // or the extra post-churn dump taken above. + assertUnloadedFrameLabel(firedDumpFile); } finally { running.set(false); for (Thread t : churnThreads) { @@ -233,6 +262,12 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti Files.deleteIfExists(dumpFile); } catch (IOException ignored) { } + if (firedDumpFile != null) { + try { + Files.deleteIfExists(firedDumpFile); + } catch (IOException ignored) { + } + } } }