From 17db8d91291060f927f5174e72bdda4c10cc7c84 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Fri, 2 Oct 2026 19:30:08 +0200 Subject: [PATCH] Compute the attribution pc with integer arithmetic attributionPC() subtracted 1 from a char pointer. During an optimistic unwind the pc can be garbage read from a return-address slot; for 0 or 0x1 that offsets a pointer to or from null, which is undefined behavior and is reported by UBSan's pointer-overflow check from walkVM's native frame resolution. Do the subtraction on uintptr_t instead, where wrap-around is defined; the resulting address still matches no library and no FDE, so attribution is unchanged. Add a gtest that feeds 0 and 0x1 through attributionPC(), which aborts under the asan config's -fsanitize=pointer-overflow before this change. Co-Authored-By: Claude Opus 5.5 --- ddprof-lib/src/main/cpp/stackWalker.inline.h | 8 +++++++- .../test/cpp/returnAddressAttribution_ut.cpp | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/ddprof-lib/src/main/cpp/stackWalker.inline.h b/ddprof-lib/src/main/cpp/stackWalker.inline.h index e10708fab0..e0553416e0 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.inline.h +++ b/ddprof-lib/src/main/cpp/stackWalker.inline.h @@ -60,8 +60,14 @@ inline void fillFrame(ASGCT_CallFrame& frame, FrameTypeId type, int bci, jmethod // zero-size-symbol cases, not zero-gap adjacency (see // BinarySearchPicksNextSymbolAtZeroGapBoundary), which is why the adjustment // is needed. +// +// The adjustment is done on uintptr_t rather than on a char pointer: pc can be +// garbage read from a return-address slot during an optimistic unwind (UBSan +// has caught both nullptr and 0x1 here), and offsetting a pointer to or from +// null is undefined behavior, while unsigned integer wrap-around is not. Such +// an address still matches no library and no FDE either way. inline const void* attributionPC(const void* pc, bool pc_is_return_address) { - return pc_is_return_address ? (const void*)((const char*)pc - 1) : pc; + return pc_is_return_address ? (const void*)((uintptr_t)pc - 1) : pc; } // The walking pc and "was it loaded from a return-address slot?" are one diff --git a/ddprof-lib/src/test/cpp/returnAddressAttribution_ut.cpp b/ddprof-lib/src/test/cpp/returnAddressAttribution_ut.cpp index 564b81faac..72c7dd1a3c 100644 --- a/ddprof-lib/src/test/cpp/returnAddressAttribution_ut.cpp +++ b/ddprof-lib/src/test/cpp/returnAddressAttribution_ut.cpp @@ -90,6 +90,24 @@ TEST(ReturnAddressAttributionCharacterizationTest, AttributionPcArithmetic) { EXPECT_EQ((const void*)((const char*)p - 1), attributionPC(p, true)); } +// An optimistic unwind can read garbage such as 0 or 1 from a return-address +// slot and hand it to attributionPC(). Offsetting a pointer to or from null is +// undefined behavior, which the asan config's -fsanitize=pointer-overflow with +// -fno-sanitize-recover=all turns into an abort; outside sanitizer builds this +// pins the wrapped values. The inputs go through volatile so the arithmetic +// runs at run time where UBSan instruments it. +TEST(ReturnAddressAttributionCharacterizationTest, AttributionPcOfGarbageNearNullIsDefined) { + volatile uintptr_t zero = 0; + volatile uintptr_t one = 1; + const void* null_pc = (const void*)zero; + const void* one_pc = (const void*)one; + + EXPECT_EQ(null_pc, attributionPC(null_pc, false)); + EXPECT_EQ(one_pc, attributionPC(one_pc, false)); + EXPECT_EQ((const void*)UINTPTR_MAX, attributionPC(null_pc, true)); + EXPECT_EQ(nullptr, attributionPC(one_pc, true)); +} + // Supporting characterization (not gating): pins findFrameDesc's and // binarySearch's row-selection semantics directly, independent of the live // walker. These do NOT change with the fix -- they document why Test 3's