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 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); } 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) { + } + } } }