From b458f36762b1511bda7822f302c0adb46588ceab Mon Sep 17 00:00:00 2001 From: Ivan Penkov Date: Thu, 15 Oct 2015 20:47:15 -0700 Subject: [PATCH] Issue in StackwalkerAMD64::GetCallerByFramePointerRecovery. There is an issue in StackwalkerAMD64::GetCallerByFramePointerRecovery. Occasionally it produces invalid frames (instruction pointer == 0) which prevents the AMD64 stack walker from proceeding to do stack scanning and instead leads to premature termination of the stack walking process. For more details: http://crbug/537444 BUG= R=mark@chromium.org Review URL: https://codereview.chromium.org/1408973002 . --- src/processor/stackwalker_amd64.cc | 34 +++-- src/processor/stackwalker_amd64.h | 8 ++ src/processor/stackwalker_amd64_unittest.cc | 132 ++++++++++++++++++++ 3 files changed, 165 insertions(+), 9 deletions(-) diff --git a/src/processor/stackwalker_amd64.cc b/src/processor/stackwalker_amd64.cc index f252a33b..8ccf73c4 100644 --- a/src/processor/stackwalker_amd64.cc +++ b/src/processor/stackwalker_amd64.cc @@ -147,6 +147,23 @@ StackFrameAMD64* StackwalkerAMD64::GetCallerByCFIFrameInfo( return frame.release(); } +bool StackwalkerAMD64::IsEndOfStack(uint64_t caller_rip, uint64_t caller_rsp, + uint64_t callee_rsp) { + // Treat an instruction address of 0 as end-of-stack. + if (caller_rip == 0) { + return true; + } + + // If the new stack pointer is at a lower address than the old, then + // that's clearly incorrect. Treat this as end-of-stack to enforce + // progress and avoid infinite loops. + if (caller_rsp < callee_rsp) { + return true; + } + + return false; +} + StackFrameAMD64* StackwalkerAMD64::GetCallerByFramePointerRecovery( const vector& frames) { StackFrameAMD64* last_frame = static_cast(frames.back()); @@ -175,8 +192,11 @@ StackFrameAMD64* StackwalkerAMD64::GetCallerByFramePointerRecovery( uint64_t caller_rsp = last_rbp + 16; // Simple sanity check that the stack is growing downwards as expected. - if (caller_rbp < last_rbp || caller_rsp < last_rsp) + if (IsEndOfStack(caller_rip, caller_rsp, last_rsp) || + caller_rbp < last_rbp) { + // Reached end-of-stack or stack is not growing downwards. return NULL; + } StackFrameAMD64* frame = new StackFrameAMD64(); frame->trust = StackFrame::FRAME_TRUST_FP; @@ -284,15 +304,11 @@ StackFrame* StackwalkerAMD64::GetCallerFrame(const CallStack* stack, new_frame->context.rbp = static_cast(new_frame->context.rbp); } - // Treat an instruction address of 0 as end-of-stack. - if (new_frame->context.rip == 0) - return NULL; - - // If the new stack pointer is at a lower address than the old, then - // that's clearly incorrect. Treat this as end-of-stack to enforce - // progress and avoid infinite loops. - if (new_frame->context.rsp <= last_frame->context.rsp) + if (IsEndOfStack(new_frame->context.rip, new_frame->context.rsp, + last_frame->context.rsp)) { + // Reached end-of-stack. return NULL; + } // new_frame->context.rip is the return address, which is the instruction // after the CALL that caused us to arrive at the callee. Set diff --git a/src/processor/stackwalker_amd64.h b/src/processor/stackwalker_amd64.h index 8f3dbd52..67c45510 100644 --- a/src/processor/stackwalker_amd64.h +++ b/src/processor/stackwalker_amd64.h @@ -78,6 +78,14 @@ class StackwalkerAMD64 : public Stackwalker { StackFrameAMD64* GetCallerByCFIFrameInfo(const vector &frames, CFIFrameInfo* cfi_frame_info); + // Checks whether end-of-stack is reached. An instruction address of 0 is an + // end-of-stack marker. If the stack pointer of the caller is at a lower + // address than the stack pointer of the callee, then that's clearly incorrect + // and it is treated as end-of-stack to enforce progress and avoid infinite + // loops. + bool IsEndOfStack(uint64_t caller_rip, uint64_t caller_rsp, + uint64_t callee_rsp); + // Assumes a traditional frame layout where the frame pointer has not been // omitted. The expectation is that caller's %rbp is pushed to the stack // after the return address of the callee, and that the callee's %rsp can diff --git a/src/processor/stackwalker_amd64_unittest.cc b/src/processor/stackwalker_amd64_unittest.cc index a54198bf..23f38f70 100644 --- a/src/processor/stackwalker_amd64_unittest.cc +++ b/src/processor/stackwalker_amd64_unittest.cc @@ -370,6 +370,138 @@ TEST_F(GetCallerFrame, ScanWithFunctionSymbols) { EXPECT_EQ(0x50000000b0000100ULL, frame1->function_base); } +// StackwalkerAMD64::GetCallerByFramePointerRecovery should never return an +// instruction pointer of 0 because IP of 0 is an end of stack marker and the +// stack walk may be terminated prematurely. Instead it should return NULL +// so that the stack walking code can proceed to stack scanning. +TEST_F(GetCallerFrame, GetCallerByFramePointerRecovery) { + MockCodeModule user32_dll(0x00007ff9cb8a0000ULL, 0x14E000, "user32.dll", + "version1"); + SetModuleSymbols(&user32_dll, // user32.dll + "PUBLIC fa60 0 DispatchMessageWorker\n" + "PUBLIC fee0 0 UserCallWinProcCheckWow\n" + "PUBLIC 1cdb0 0 _fnHkINLPMSG\n" + "STACK CFI INIT fa60 340 .cfa: $rsp .ra: .cfa 8 - ^\n" + "STACK CFI fa60 .cfa: $rsp 128 +\n" + "STACK CFI INIT fee0 49f .cfa: $rsp .ra: .cfa 8 - ^\n" + "STACK CFI fee0 .cfa: $rsp 240 +\n" + "STACK CFI INIT 1cdb0 9f .cfa: $rsp .ra: .cfa 8 - ^\n" + "STACK CFI 1cdb0 .cfa: $rsp 80 +\n"); + + // Create some modules with some stock debugging information. + MockCodeModules local_modules; + local_modules.Add(&user32_dll); + + Label frame0_rsp; + Label frame0_rbp; + Label frame1_rsp; + Label frame2_rsp; + + stack_section.start() = 0x00000099abf0f238ULL; + stack_section + .Mark(&frame0_rsp) + .D64(0x00007ff9cb8b00dcULL) + .Mark(&frame1_rsp) + .D64(0x0000000000000000ULL) + .D64(0x0000000000000001ULL) + .D64(0x00000099abf0f308ULL) + .D64(0x00007ff9cb8bce3aULL) // Stack residue from execution of + // user32!_fnHkINLPMSG+0x8a + .D64(0x000000000000c2e0ULL) + .D64(0x00000099abf0f328ULL) + .D64(0x0000000100000001ULL) + .D64(0x0000000000000000ULL) + .D64(0x0000000000000000ULL) + .D64(0x0000000000000000ULL) + .D64(0x0000000000000000ULL) + .D64(0x0000000000000000ULL) + .D64(0x0000000000000000ULL) + .D64(0x00007ff9ccad53e4ULL) + .D64(0x0000000000000048ULL) + .D64(0x0000000000000001ULL) + .D64(0x00000099abf0f5e0ULL) + .D64(0x00000099b61f7388ULL) + .D64(0x0000000000000030ULL) + .D64(0xffffff66540f0a1fULL) + .D64(0xffffff6649e08c77ULL) + .D64(0x00007ff9cb8affb4ULL) // Return address in + // user32!UserCallWinProcCheckWow+0xd4 + .D64(0x0000000000000000ULL) + .D64(0x00000099abf0f368ULL) + .D64(0x0000000000000000ULL) + .D64(0x0000000000000000ULL) + .D64(0x0000000000000000ULL) + .D64(0x00000099a8150fd8ULL) + .D64(0x00000099abf0f3e8ULL) + .D64(0x00007ff9cb8afc07ULL) // Return address in + // user32!DispatchMessageWorker+0x1a7 + .Mark(&frame2_rsp) + .Append(256, 0) + .Mark(&frame0_rbp) // The following are expected by + // GetCallerByFramePointerRecovery. + .D64(0xfffffffffffffffeULL) // %caller_rbp = *(%callee_rbp) + .D64(0x0000000000000000ULL) // %caller_rip = *(%callee_rbp + 8) + .D64(0x00000099a3e31040ULL) // %caller_rsp = *(%callee_rbp + 16) + .Append(256, 0); + + RegionFromSection(); + raw_context.rip = 0x00000099a8150fd8ULL; // IP in context frame is guarbage + raw_context.rsp = frame0_rsp.Value(); + raw_context.rbp = frame0_rbp.Value(); + + StackFrameSymbolizer frame_symbolizer(&supplier, &resolver); + StackwalkerAMD64 walker(&system_info, &raw_context, &stack_region, + &local_modules, &frame_symbolizer); + vector modules_without_symbols; + vector modules_with_corrupt_symbols; + ASSERT_TRUE(walker.Walk(&call_stack, &modules_without_symbols, + &modules_with_corrupt_symbols)); + ASSERT_EQ(0U, modules_without_symbols.size()); + ASSERT_EQ(0U, modules_with_corrupt_symbols.size()); + frames = call_stack.frames(); + + ASSERT_EQ(3U, frames->size()); + + { // To avoid reusing locals by mistake + StackFrameAMD64 *frame = static_cast(frames->at(0)); + EXPECT_EQ(StackFrame::FRAME_TRUST_CONTEXT, frame->trust); + ASSERT_EQ(StackFrameAMD64::CONTEXT_VALID_ALL, frame->context_validity); + EXPECT_EQ("", frame->function_name); + EXPECT_EQ(0x00000099a8150fd8ULL, frame->instruction); + EXPECT_EQ(0x00000099a8150fd8ULL, frame->context.rip); + EXPECT_EQ(frame0_rsp.Value(), frame->context.rsp); + EXPECT_EQ(frame0_rbp.Value(), frame->context.rbp); + } + + { // To avoid reusing locals by mistake + StackFrameAMD64 *frame = static_cast(frames->at(1)); + EXPECT_EQ(StackFrame::FRAME_TRUST_SCAN, frame->trust); + ASSERT_EQ((StackFrameAMD64::CONTEXT_VALID_RIP | + StackFrameAMD64::CONTEXT_VALID_RSP | + StackFrameAMD64::CONTEXT_VALID_RBP), + frame->context_validity); + EXPECT_EQ("UserCallWinProcCheckWow", frame->function_name); + EXPECT_EQ(140710838468828ULL, frame->instruction + 1); + EXPECT_EQ(140710838468828ULL, frame->context.rip); + EXPECT_EQ(frame1_rsp.Value(), frame->context.rsp); + EXPECT_EQ(&user32_dll, frame->module); + } + + { // To avoid reusing locals by mistake + StackFrameAMD64 *frame = static_cast(frames->at(2)); + EXPECT_EQ(StackFrame::FRAME_TRUST_CFI, frame->trust); + ASSERT_EQ((StackFrameAMD64::CONTEXT_VALID_RIP | + StackFrameAMD64::CONTEXT_VALID_RSP | + StackFrameAMD64::CONTEXT_VALID_RBP), + frame->context_validity); + EXPECT_EQ("DispatchMessageWorker", frame->function_name); + EXPECT_EQ(140710838467591ULL, frame->instruction + 1); + EXPECT_EQ(140710838467591ULL, frame->context.rip); + EXPECT_EQ(frame2_rsp.Value(), frame->context.rsp); + EXPECT_EQ(&user32_dll, frame->module); + } +} + // Test that set_max_frames_scanned prevents using stack scanning // to find caller frames. TEST_F(GetCallerFrame, ScanningNotAllowed) {