diff --git a/ddprof-lib/src/main/cpp/codeCache.cpp b/ddprof-lib/src/main/cpp/codeCache.cpp index 61c76a3e..6e195737 100644 --- a/ddprof-lib/src/main/cpp/codeCache.cpp +++ b/ddprof-lib/src/main/cpp/codeCache.cpp @@ -1,5 +1,6 @@ /* * Copyright The async-profiler authors + * Copyright 2026, Datadog, Inc * SPDX-License-Identifier: Apache-2.0 */ @@ -423,3 +424,6 @@ void CodeCache::setBuildId(const char* build_id, size_t build_id_len) { } } +// NativeCodeBounds static member initialization +const void* NativeCodeBounds::_min = (const void*)UINTPTR_MAX; +const void* NativeCodeBounds::_max = (const void*)0; diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index d0cd0309..cc8c0e04 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -1,5 +1,6 @@ /* * Copyright The async-profiler authors + * Copyright 2026, Datadog, Inc * SPDX-License-Identifier: Apache-2.0 */ @@ -298,4 +299,43 @@ class CodeCacheArray { } }; +/** + * Global bounds tracking for all native code regions. + * Used by stack walker to quickly validate PC values point to actual code. + * + * Thread-safety: updateBounds() uses atomic CAS for concurrent updates. + * Signal-safety: contains() only reads two pointers (no locks). + */ +class NativeCodeBounds { + private: + static const void* _min; + static const void* _max; + + public: + /** + * Update bounds when a new library is loaded. + * Uses atomic CAS loop to handle concurrent updates safely. + */ + static void updateBounds(const void* start, const void* end) { + for (const void* m = _min; start < m && + !__sync_bool_compare_and_swap(&_min, m, start); m = _min); + for (const void* m = _max; end > m && + !__sync_bool_compare_and_swap(&_max, m, end); m = _max); + } + + /** + * Check if address falls within any known native code region. + * Signal-safe: only two pointer comparisons, no locks. + * Returns true if bounds haven't been initialized yet (fail-open). + */ + static bool contains(const void* pc) { + // If bounds haven't been initialized yet (_max <= _min), return true + // to avoid rejecting valid PCs before libraries are parsed. + if (_max <= _min) { + return true; + } + return pc >= _min && pc < _max; + } +}; + #endif // _CODECACHE_H diff --git a/ddprof-lib/src/main/cpp/stackWalker.cpp b/ddprof-lib/src/main/cpp/stackWalker.cpp index a077a95d..f0ecc425 100644 --- a/ddprof-lib/src/main/cpp/stackWalker.cpp +++ b/ddprof-lib/src/main/cpp/stackWalker.cpp @@ -6,6 +6,7 @@ #include #include "stackWalker.h" +#include "codeCache.h" #include "dwarf.h" #include "profiler.h" #include "safeAccess.h" @@ -111,8 +112,32 @@ int StackWalker::walkFP(void* ucontext, const void** callchain, int max_depth, S break; } + // Verify PC points to actual code, not data. + // When FP wanders into locals, the value at FRAME_PC_SLOT is a local variable + // (object pointer, integer, etc.) that rarely falls within code address ranges. + if (!CodeHeap::contains(pc) && !NativeCodeBounds::contains(pc)) { + break; + } + sp = fp + (FRAME_PC_SLOT + 1) * sizeof(void*); - fp = *(uintptr_t*)fp; + uintptr_t prev_fp = fp; + fp = (uintptr_t)SafeAccess::load((void**)fp); + + // Early detection: validate immediately instead of waiting for next iteration. + // When FP wanders into local variables (due to -fomit-frame-pointer or + // -momit-leaf-frame-pointer), the value read as "next FP" is typically a local + // variable that won't form a valid upward chain toward older stack frames. + if (fp != 0) { + // FP chain must progress toward higher addresses (older frames on stack). + // Stack grows downward, so caller's FP must be > current FP. + if (fp <= prev_fp) { + break; + } + // Check alignment and upper bound immediately rather than next iteration + if (!aligned(fp) || fp >= bottom) { + break; + } + } } if (truncated && depth > max_depth) { diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 8b6a7cf2..e3654f01 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -1,5 +1,6 @@ /* * Copyright The async-profiler authors + * Copyright 2026, Datadog, Inc * SPDX-License-Identifier: Apache-2.0 */ @@ -958,6 +959,8 @@ void Symbols::parseLibraries(CodeCacheArray* array, bool kernel_symbols) { if (haveKernelSymbols()) { cc->sort(); array->add(cc); + // Update global native code bounds for PC validation in stack walker + NativeCodeBounds::updateBounds(cc->minAddress(), cc->maxAddress()); } else { delete cc; } @@ -996,6 +999,9 @@ void Symbols::parseLibraries(CodeCacheArray* array, bool kernel_symbols) { cc->sort(); applyPatch(cc); array->add(cc); + + // Update global native code bounds for PC validation in stack walker + NativeCodeBounds::updateBounds(cc->minAddress(), cc->maxAddress()); } if (array->count() >= MAX_NATIVE_LIBS && !_libs_limit_reported) { diff --git a/ddprof-lib/src/main/cpp/symbols_macos.cpp b/ddprof-lib/src/main/cpp/symbols_macos.cpp index 268d0cad..289d6011 100644 --- a/ddprof-lib/src/main/cpp/symbols_macos.cpp +++ b/ddprof-lib/src/main/cpp/symbols_macos.cpp @@ -1,5 +1,6 @@ /* * Copyright The async-profiler authors + * Copyright 2026, Datadog, Inc * SPDX-License-Identifier: Apache-2.0 */ @@ -214,6 +215,8 @@ void Symbols::parseLibraries(CodeCacheArray* array, bool kernel_symbols) { } cc->sort(); array->add(cc); + // Update global native code bounds for PC validation in stack walker + NativeCodeBounds::updateBounds(cc->minAddress(), cc->maxAddress()); } else { delete cc; } diff --git a/ddprof-lib/src/test/cpp/stackWalkValidation_ut.cpp b/ddprof-lib/src/test/cpp/stackWalkValidation_ut.cpp new file mode 100644 index 00000000..3531dba6 --- /dev/null +++ b/ddprof-lib/src/test/cpp/stackWalkValidation_ut.cpp @@ -0,0 +1,186 @@ +/* + * Copyright 2026, Datadog, Inc + * SPDX-License-Identifier: Apache-2.0 + */ + +#include +#include + +#include "stackWalker.h" + +using namespace StackWalkValidation; + +class StackWalkValidationTest : public ::testing::Test { +protected: + // Simulated stack frame structure for testing FP chain validation logic + struct MockFrame { + uintptr_t saved_fp; // Saved frame pointer (points to caller's frame) + void* return_addr; // Return address + }; +}; + +// Test inDeadZone helper +TEST_F(StackWalkValidationTest, inDeadZone_NullPointer) { + EXPECT_TRUE(inDeadZone(nullptr)); +} + +TEST_F(StackWalkValidationTest, inDeadZone_LowAddress) { + // Addresses below DEAD_ZONE (0x1000) should be in dead zone + EXPECT_TRUE(inDeadZone((void*)0x100)); + EXPECT_TRUE(inDeadZone((void*)0xFFF)); +} + +TEST_F(StackWalkValidationTest, inDeadZone_ValidAddress) { + // Addresses above DEAD_ZONE should not be in dead zone + EXPECT_FALSE(inDeadZone((void*)0x1000)); + EXPECT_FALSE(inDeadZone((void*)0x10000)); +} + +TEST_F(StackWalkValidationTest, inDeadZone_HighAddress) { + // Very high addresses (near -DEAD_ZONE) should be in dead zone + EXPECT_TRUE(inDeadZone((void*)(~(uintptr_t)0))); + EXPECT_TRUE(inDeadZone((void*)(~(uintptr_t)0 - 0x100))); +} + +// Test aligned helper +TEST_F(StackWalkValidationTest, aligned_WordAligned) { + // Word-aligned addresses (8-byte on 64-bit) + EXPECT_TRUE(aligned(0x1000)); + EXPECT_TRUE(aligned(0x1008)); + EXPECT_TRUE(aligned(0x7fff00000000)); +} + +TEST_F(StackWalkValidationTest, aligned_NotAligned) { + // Non-word-aligned addresses + EXPECT_FALSE(aligned(0x1001)); + EXPECT_FALSE(aligned(0x1002)); + EXPECT_FALSE(aligned(0x1004)); // 4-byte aligned but not 8-byte on 64-bit + EXPECT_FALSE(aligned(0x1007)); +} + +// Test sameStack helper +TEST_F(StackWalkValidationTest, sameStack_SameLocation) { + int x; + EXPECT_TRUE(sameStack(&x, &x)); +} + +TEST_F(StackWalkValidationTest, sameStack_NearbyLocations) { + char buffer[4096]; + EXPECT_TRUE(sameStack(&buffer[4095], &buffer[0])); +} + +TEST_F(StackWalkValidationTest, sameStack_FarApart) { + // Addresses more than SAME_STACK_DISTANCE apart + void* hi = (void*)0x7fff00010000; + void* lo = (void*)0x7fff00000000; + EXPECT_FALSE(sameStack(hi, lo)); +} + +/** + * Test the FP chain progression validation logic. + * + * In a valid FP chain: + * - Stack grows downward (toward lower addresses) + * - Saved FP points to caller's frame (higher address) + * - Therefore: next_fp > current_fp + * + * When FP wanders into local variables: + * - The value read as "saved FP" is actually a local variable + * - Locals are typically at lower or similar addresses + * - This violates the fp > prev_fp invariant + */ +TEST_F(StackWalkValidationTest, FPChainProgression_ValidChain) { + // Allocate frames in a buffer with controlled layout + // Frame layout must simulate: frame1 (low addr) -> frame2 -> frame3 (high addr) + // where each frame's saved_fp points to the next higher frame + alignas(sizeof(uintptr_t)) char buffer[3 * sizeof(MockFrame)]; + + MockFrame* frame1 = (MockFrame*)(buffer); + MockFrame* frame2 = (MockFrame*)(buffer + sizeof(MockFrame)); + MockFrame* frame3 = (MockFrame*)(buffer + 2 * sizeof(MockFrame)); + + // Verify ordering: frame1 < frame2 < frame3 + ASSERT_LT((uintptr_t)frame1, (uintptr_t)frame2); + ASSERT_LT((uintptr_t)frame2, (uintptr_t)frame3); + + // Setup chain: frame1 -> frame2 -> frame3 -> null + frame3->saved_fp = 0; // Oldest frame (null FP = end) + frame3->return_addr = nullptr; + + frame2->saved_fp = (uintptr_t)frame3; + frame2->return_addr = (void*)0x400000; + + frame1->saved_fp = (uintptr_t)frame2; + frame1->return_addr = (void*)0x400100; + + uintptr_t fp = (uintptr_t)frame1; + + // Walk the chain and verify progression + uintptr_t prev_fp = fp; + fp = *(uintptr_t*)fp; // Read saved FP from frame1 + + // Valid: next FP (frame2) should be greater than current (frame1) + EXPECT_GT(fp, prev_fp); + EXPECT_TRUE(aligned(fp)); + + prev_fp = fp; + fp = *(uintptr_t*)fp; // Read saved FP from frame2 + + // Valid: next FP (frame3) should be greater than current (frame2) + EXPECT_GT(fp, prev_fp); + + // Final frame has null FP (chain terminator) + prev_fp = fp; + fp = *(uintptr_t*)fp; // Read saved FP from frame3 + EXPECT_EQ(fp, 0u); +} + +TEST_F(StackWalkValidationTest, FPChainProgression_BackwardsChain) { + // Simulate an invalid FP chain where FP goes backward + // This happens when FP points to a local variable + MockFrame frame1; + uintptr_t fake_local = 0x12345678; // Simulated local variable value + + // Point FP to a location that contains a "backwards" pointer + frame1.saved_fp = (uintptr_t)&fake_local - 0x1000; // Lower address + + uintptr_t fp = (uintptr_t)&frame1; + uintptr_t prev_fp = fp; + fp = *(uintptr_t*)fp; + + // Invalid: next FP is less than current (backwards) + // This is the condition our validation catches + EXPECT_LE(fp, prev_fp); +} + +TEST_F(StackWalkValidationTest, FPChainProgression_SameAddress) { + // FP pointing to itself (no progress) is invalid + MockFrame frame1; + frame1.saved_fp = (uintptr_t)&frame1; // Points to itself + + uintptr_t fp = (uintptr_t)&frame1; + uintptr_t prev_fp = fp; + fp = *(uintptr_t*)fp; + + // Invalid: FP equals prev_fp (no progress) + EXPECT_EQ(fp, prev_fp); +} + +/** + * Test that validates the relationship between alignment and FP validity. + * Pointer-sized local variables are word-aligned by the compiler, so alignment + * alone cannot distinguish valid FPs from locals. The progression check + * (fp > prev_fp) is the key discriminator. + */ +TEST_F(StackWalkValidationTest, PointerSizedLocalsAreAligned) { + // Demonstrate that pointer-sized local variables are word-aligned. + // This is why alignment alone isn't sufficient to validate FP chains. + // Note: smaller types like int may only be 4-byte aligned on 64-bit systems. + uintptr_t local_uintptr; + void* local_ptr; + size_t local_size; + + EXPECT_TRUE(aligned((uintptr_t)&local_uintptr)); + EXPECT_TRUE(aligned((uintptr_t)&local_ptr)); + EXPECT_TRUE(aligned((uintptr_t)&local_size)); +}