Stack sampling profiler: fix leaf function unwinding and remove blacklisting

The leaf function unwinding has a bug where the new instruction pointer
is set to the stack pointer rather than the value at the stack
pointer. This causes the module check on the next iteration to fail,
resulting in stacks truncated at depth 1. This change fixes that bug.

The change also removes the associated leaf function blacklisting
implementation. This implementation is no longer needed as the third
party code that precipitated it is now detected by the valid module
check.

BUG=555770

Review URL: https://codereview.chromium.org/1503273003

Cr-Commit-Position: refs/heads/master@{#363725}

base/profiler/win32_stack_frame_unwinder.cc

@@ -7,10 +7,6 @@
 #include <windows.h>
 #include <utility>
 
-#include "base/containers/hash_tables.h"
-#include "base/memory/singleton.h"
-#include "base/stl_util.h"
-
 namespace base {
 
 // Win32UnwindFunctions -------------------------------------------------------
@@ -104,58 +100,6 @@ ScopedModuleHandle Win32UnwindFunctions::GetModuleForProgramCounter(
   return ScopedModuleHandle(module_handle);
 }
 
-// LeafUnwindBlacklist --------------------------------------------------------
-
-// Records modules that are known to have functions that violate the Microsoft
-// x64 calling convention and would be dangerous to manually unwind if
-// encountered as the last frame on the call stack. Functions like these have
-// been observed in injected third party modules that either do not provide
-// function unwind information, or do not provide the required function prologue
-// and epilogue. The former case was observed in several AV products and the
-// latter in a WndProc function associated with Actual Window
-// Manager/aimemb64.dll. See https://crbug.com/476422.
-class LeafUnwindBlacklist {
- public:
-  static LeafUnwindBlacklist* GetInstance();
-
-  // Returns true if |module| has been blacklisted.
-  bool IsBlacklisted(const void* module) const;
-
-  // Records |module| for blacklisting.
-  void BlacklistModule(const void* module);
-
- private:
-  friend struct DefaultSingletonTraits<LeafUnwindBlacklist>;
-
-  LeafUnwindBlacklist();
-  ~LeafUnwindBlacklist();
-
-  // The set of modules known to have functions that violate the Microsoft x64
-  // calling convention.
-  base::hash_set<const void*> blacklisted_modules_;
-
-  DISALLOW_COPY_AND_ASSIGN(LeafUnwindBlacklist);
-};
-
-// static
-LeafUnwindBlacklist* LeafUnwindBlacklist::GetInstance() {
-  // Leaky for performance reasons.
-  return Singleton<LeafUnwindBlacklist,
-                   LeakySingletonTraits<LeafUnwindBlacklist>>::get();
-}
-
-bool LeafUnwindBlacklist::IsBlacklisted(const void* module) const {
-  return ContainsKey(blacklisted_modules_, module);
-}
-
-void LeafUnwindBlacklist::BlacklistModule(const void* module) {
-  CHECK(module);
-  blacklisted_modules_.insert(module);
-}
-
-LeafUnwindBlacklist::LeafUnwindBlacklist() {}
-LeafUnwindBlacklist::~LeafUnwindBlacklist() {}
-
 }  // namespace
 
 // Win32StackFrameUnwinder ----------------------------------------------------
@@ -172,8 +116,6 @@ Win32StackFrameUnwinder::~Win32StackFrameUnwinder() {}
 bool Win32StackFrameUnwinder::TryUnwind(CONTEXT* context,
                                         ScopedModuleHandle* module) {
 #ifdef _WIN64
-  CHECK(!at_top_frame_ || unwind_info_present_for_all_frames_);
-
   ScopedModuleHandle frame_module =
       unwind_functions_->GetModuleForProgramCounter(context->Rip);
   if (!frame_module.IsValid()) {
@@ -203,74 +145,25 @@ bool Win32StackFrameUnwinder::TryUnwind(CONTEXT* context,
                                      context);
     at_top_frame_ = false;
   } else {
-    // RtlLookupFunctionEntry didn't find unwind information. This could mean
-    // the code at the instruction pointer is in:
-    //
-    // 1. a true leaf function (i.e. a function that neither calls a function,
-    //    nor allocates any stack space itself) in which case the return
-    //    address is at RSP, or
-    //
-    // 2. a function that doesn't adhere to the Microsoft x64 calling
-    //    convention, either by not providing the required unwind information,
-    //    or by not having the prologue or epilogue required for unwinding;
-    //    this case has been observed in crash data in injected third party
-    //    DLLs.
-    //
-    // In valid code, case 1 can only occur (by definition) as the last frame
-    // on the stack. This happens in about 5% of observed stacks and can
-    // easily be unwound by popping RSP and using it as the next frame's
-    // instruction pointer.
-    //
-    // Case 2 can occur anywhere on the stack, and attempting to unwind the
-    // stack will result in treating whatever value happens to be on the stack
-    // at RSP as the next frame's instruction pointer. This is certainly wrong
-    // and very likely to lead to crashing by deferencing invalid pointers in
-    // the next RtlVirtualUnwind call.
-    //
-    // If we see case 2 at a location not the last frame, and all the previous
-    // frame had valid unwind information, then this is definitely bad code.
-    // We blacklist the module as untrustable for unwinding if we encounter a
-    // function in it that doesn't have unwind information.
-
     if (at_top_frame_) {
       at_top_frame_ = false;
 
-      // We are at the end of the stack. It's very likely that we're in case 1
+      // This is a leaf function (i.e. a function that neither calls a function,
-      // since the vast majority of code adheres to the Microsoft x64 calling
+      // nor allocates any stack space itself) so the return address is at RSP.
-      // convention. But there's a small chance we might be unlucky and be in
+      context->Rip = *reinterpret_cast<DWORD64*>(context->Rsp);
-      // case 2. If this module is known to have bad code according to the
-      // leaf unwind blacklist, stop here, otherwise manually unwind.
-      if (LeafUnwindBlacklist::GetInstance()->IsBlacklisted(
-              reinterpret_cast<const void*>(image_base))) {
-        return false;
-      }
-
-      context->Rip = context->Rsp;
       context->Rsp += 8;
-      unwind_info_present_for_all_frames_ = false;
     } else {
-      // We're not at the end of the stack. This frame is untrustworthy and we
+      // In theory we shouldn't get here, as it means we've encountered a
-      // can't safely unwind from here.
+      // function without unwind information below the top of the stack, which
-      if (!image_base) {
+      // is forbidden by the Microsoft x64 calling convention.
-        // A null image_base means that the the last unwind produced an invalid
+      //
-        // instruction pointer. This has been observed where unwind information
+      // The one known case in Chrome code that executes this path occurs
-        // was present for a function but was inconsistent with the actual
+      // because of BoringSSL unwind information inconsistent with the actual
-        // function code, in particular in BoringSSL. See
+      // function code. See https://crbug.com/542919.
-        // https://crbug.com/542919.
+      //
-      } else if (unwind_info_present_for_all_frames_) {
+      // Note that dodgy third-party generated code that otherwise would enter
-        // Unwind information was present for all previous frames, so we can
+      // this path should be caught by the module check above, since the code
-        // be confident this is case 2. Record the module to be blacklisted.
+      // typically is located outside of a module.
-        LeafUnwindBlacklist::GetInstance()->BlacklistModule(
-            reinterpret_cast<const void *>(image_base));
-      } else {
-        // We started off on a function without unwind information. It's very
-        // likely that all frames up to this point have been good, and this
-        // frame is case 2. But it's possible that the initial frame was case
-        // 2 but hadn't been blacklisted yet, and we've started to go off into
-        // the weeds. Since we can't be sure, just bail out without
-        // blacklisting the module; chances are we'll later encounter the same
-        // function on a stack with full unwind information.
-      }
       return false;
     }
   }
@@ -286,7 +179,6 @@ bool Win32StackFrameUnwinder::TryUnwind(CONTEXT* context,
 Win32StackFrameUnwinder::Win32StackFrameUnwinder(
     scoped_ptr<UnwindFunctions> unwind_functions)
     : at_top_frame_(true),
-      unwind_info_present_for_all_frames_(true),
       unwind_functions_(std::move(unwind_functions)) {}
 
 }  // namespace base

base/profiler/win32_stack_frame_unwinder_unittest.cc

@@ -31,33 +31,17 @@ class TestUnwindFunctions : public Win32StackFrameUnwinder::UnwindFunctions {
   // Instructs GetModuleForProgramCounter to return null on the next call.
   void SetUnloadedModule();
 
-  // These functions set whether the next frame will have a RUNTIME_FUNCTION,
+  // These functions set whether the next frame will have a RUNTIME_FUNCTION.
-  // and allow specification of a custom image_base.
   void SetHasRuntimeFunction(CONTEXT* context);
-  void SetHasRuntimeFunction(DWORD64 image_base, CONTEXT* context);
-  void SetHasRuntimeFunction(DWORD64 image_base,
-                             const RUNTIME_FUNCTION& runtime_function,
-                             DWORD program_counter_offset,
-                             CONTEXT* context);
   void SetNoRuntimeFunction(CONTEXT* context);
-  void SetNoRuntimeFunction(DWORD64 image_base, CONTEXT* context);
 
  private:
-  enum RuntimeFunctionState { NO_RUNTIME_FUNCTION, HAS_RUNTIME_FUNCTION };
-
   enum { kImageBaseIncrement = 1 << 20 };
 
-  // Sets whether the next frame should have a RUNTIME_FUNCTION, and allows
-  // specification of a custom image_base.
-  void SetNextFrameState(RuntimeFunctionState runtime_function_state,
-                         DWORD64 image_base,
-                         CONTEXT* context);
-
   static RUNTIME_FUNCTION* const kInvalidRuntimeFunction;
 
   bool module_is_loaded_;
   DWORD64 expected_program_counter_;
-  DWORD64 custom_image_base_;
   DWORD64 next_image_base_;
   DWORD64 expected_image_base_;
   RUNTIME_FUNCTION* next_runtime_function_;
@@ -72,7 +56,6 @@ RUNTIME_FUNCTION* const TestUnwindFunctions::kInvalidRuntimeFunction =
 TestUnwindFunctions::TestUnwindFunctions()
     : module_is_loaded_(true),
       expected_program_counter_(0),
-      custom_image_base_(0),
       next_image_base_(kImageBaseIncrement),
       expected_image_base_(0),
       next_runtime_function_(kInvalidRuntimeFunction) {
@@ -82,13 +65,8 @@ PRUNTIME_FUNCTION TestUnwindFunctions::LookupFunctionEntry(
     DWORD64 program_counter,
     PDWORD64 image_base) {
   EXPECT_EQ(expected_program_counter_, program_counter);
-  if (custom_image_base_) {
+  *image_base = expected_image_base_ = next_image_base_;
-    *image_base = expected_image_base_ = custom_image_base_;
+  next_image_base_ += kImageBaseIncrement;
-    custom_image_base_ = 0;
-  } else {
-    *image_base = expected_image_base_ = next_image_base_;
-    next_image_base_ += kImageBaseIncrement;
-  }
   RUNTIME_FUNCTION* return_value = next_runtime_function_;
   next_runtime_function_ = kInvalidRuntimeFunction;
   return return_value;
@@ -124,58 +102,19 @@ void TestUnwindFunctions::SetUnloadedModule() {
 }
 
 void TestUnwindFunctions::SetHasRuntimeFunction(CONTEXT* context) {
-  SetNextFrameState(HAS_RUNTIME_FUNCTION, 0, context);
+  RUNTIME_FUNCTION runtime_function = {};
-}
+  runtime_function.BeginAddress = 16;
-
+  runtime_function.EndAddress = runtime_function.BeginAddress + 256;
-void TestUnwindFunctions::SetHasRuntimeFunction(DWORD64 image_base,
-                                                CONTEXT* context) {
-  SetNextFrameState(HAS_RUNTIME_FUNCTION, image_base, context);
-}
-
-void TestUnwindFunctions::SetHasRuntimeFunction(
-    DWORD64 image_base,
-    const RUNTIME_FUNCTION& runtime_function,
-    DWORD program_counter_offset,
-    CONTEXT* context) {
-  custom_image_base_ = image_base;
   runtime_functions_.push_back(runtime_function);
   next_runtime_function_ = &runtime_functions_.back();
+
   expected_program_counter_ = context->Rip =
-      image_base + program_counter_offset;
+      next_image_base_ + runtime_function.BeginAddress + 8;
 }
 
 void TestUnwindFunctions::SetNoRuntimeFunction(CONTEXT* context) {
-  SetNextFrameState(NO_RUNTIME_FUNCTION, 0, context);
+  expected_program_counter_ = context->Rip = 100;
-}
+  next_runtime_function_ = nullptr;
-
-void TestUnwindFunctions::SetNoRuntimeFunction(DWORD64 image_base,
-                                               CONTEXT* context) {
-  SetNextFrameState(NO_RUNTIME_FUNCTION, image_base, context);
-}
-
-
-void TestUnwindFunctions::SetNextFrameState(
-    RuntimeFunctionState runtime_function_state,
-    DWORD64 image_base,
-    CONTEXT* context) {
-  if (image_base)
-    custom_image_base_ = image_base;
-
-  if (runtime_function_state == HAS_RUNTIME_FUNCTION) {
-    RUNTIME_FUNCTION runtime_function = {};
-    runtime_function.BeginAddress = 16;
-    runtime_function.EndAddress = runtime_function.BeginAddress + 256;
-    runtime_functions_.push_back(runtime_function);
-    next_runtime_function_ = &runtime_functions_.back();
-
-    DWORD64 image_base = custom_image_base_ ? custom_image_base_ :
-        next_image_base_;
-    expected_program_counter_ = context->Rip =
-        image_base + runtime_function.BeginAddress + 8;
-  } else {
-    expected_program_counter_ = context->Rip = 100;
-    next_runtime_function_ = nullptr;
-  }
 }
 
 }  // namespace
@@ -240,12 +179,13 @@ TEST_F(Win32StackFrameUnwinderTest, FrameAtTopWithoutUnwindInfo) {
   scoped_ptr<Win32StackFrameUnwinder> unwinder = CreateUnwinder();
   CONTEXT context = {0};
   ScopedModuleHandle module;
-  const DWORD64 original_rsp = 128;
+  DWORD64 next_ip = 0x0123456789abcdef;
+  DWORD64 original_rsp = reinterpret_cast<DWORD64>(&next_ip);
   context.Rsp = original_rsp;
 
   unwind_functions_->SetNoRuntimeFunction(&context);
   EXPECT_TRUE(unwinder->TryUnwind(&context, &module));
-  EXPECT_EQ(original_rsp, context.Rip);
+  EXPECT_EQ(next_ip, context.Rip);
   EXPECT_EQ(original_rsp + 8, context.Rsp);
   EXPECT_TRUE(module.IsValid());
 
@@ -261,9 +201,8 @@ TEST_F(Win32StackFrameUnwinderTest, FrameAtTopWithoutUnwindInfo) {
 }
 
 // Checks that a frame below the top of the stack with missing unwind info
-// results in blacklisting the module.
+// terminates the unwinding.
-TEST_F(Win32StackFrameUnwinderTest, BlacklistedModule) {
+TEST_F(Win32StackFrameUnwinderTest, FrameBelowTopWithoutUnwindInfo) {
-  const DWORD64 image_base_for_module_with_bad_function = 1024;
   {
     // First stack, with a bad function below the top of the stack.
     scoped_ptr<Win32StackFrameUnwinder> unwinder = CreateUnwinder();
@@ -273,109 +212,9 @@ TEST_F(Win32StackFrameUnwinderTest, BlacklistedModule) {
     EXPECT_TRUE(unwinder->TryUnwind(&context, &module));
     EXPECT_TRUE(module.IsValid());
 
-    unwind_functions_->SetNoRuntimeFunction(
-        image_base_for_module_with_bad_function,
-        &context);
-    EXPECT_FALSE(unwinder->TryUnwind(&context, &module));
-  }
-
-  {
-    // Second stack; check that a function at the top of the stack without
-    // unwind info from the previously-seen module is blacklisted.
-    scoped_ptr<Win32StackFrameUnwinder> unwinder = CreateUnwinder();
-    CONTEXT context = {0};
-    ScopedModuleHandle module;
-    unwind_functions_->SetNoRuntimeFunction(
-        image_base_for_module_with_bad_function,
-        &context);
-    EXPECT_FALSE(unwinder->TryUnwind(&context, &module));
-  }
-
-  {
-    // Third stack; check that a function at the top of the stack *with* unwind
-    // info from the previously-seen module is not blacklisted. Then check that
-    // functions below the top of the stack with unwind info are not
-    // blacklisted, regardless of whether they are in the previously-seen
-    // module.
-    scoped_ptr<Win32StackFrameUnwinder> unwinder = CreateUnwinder();
-    CONTEXT context = {0};
-    ScopedModuleHandle module;
-    unwind_functions_->SetHasRuntimeFunction(
-        image_base_for_module_with_bad_function,
-        &context);
-    EXPECT_TRUE(unwinder->TryUnwind(&context, &module));
-    EXPECT_TRUE(module.IsValid());
-
-    unwind_functions_->SetHasRuntimeFunction(&context);
-    module.Set(nullptr);
-    EXPECT_TRUE(unwinder->TryUnwind(&context, &module));
-    EXPECT_TRUE(module.IsValid());
-
-    unwind_functions_->SetHasRuntimeFunction(
-        image_base_for_module_with_bad_function,
-        &context);
-    module.Set(nullptr);
-    EXPECT_TRUE(unwinder->TryUnwind(&context, &module));
-    EXPECT_TRUE(module.IsValid());
-  }
-
-  {
-    // Fourth stack; check that a function at the top of the stack without
-    // unwind info and not from the previously-seen module is not
-    // blacklisted. Then check that functions below the top of the stack with
-    // unwind info are not blacklisted, regardless of whether they are in the
-    // previously-seen module.
-    scoped_ptr<Win32StackFrameUnwinder> unwinder = CreateUnwinder();
-    CONTEXT context = {0};
-    ScopedModuleHandle module;
     unwind_functions_->SetNoRuntimeFunction(&context);
-    EXPECT_TRUE(unwinder->TryUnwind(&context, &module));
-    EXPECT_TRUE(module.IsValid());
-
-    unwind_functions_->SetHasRuntimeFunction(&context);
-    module.Set(nullptr);
-    EXPECT_TRUE(unwinder->TryUnwind(&context, &module));
-    EXPECT_TRUE(module.IsValid());
-
-    unwind_functions_->SetHasRuntimeFunction(
-        image_base_for_module_with_bad_function,
-        &context);
-    module.Set(nullptr);
-    EXPECT_TRUE(unwinder->TryUnwind(&context, &module));
-    EXPECT_TRUE(module.IsValid());
-  }
-}
-
-// Checks that a frame below the top of the stack with missing unwind info does
-// not result in blacklisting the module if the first frame also was missing
-// unwind info. This ensures we don't blacklist an innocent module because the
-// first frame was bad but we didn't know it at the time.
-TEST_F(Win32StackFrameUnwinderTest, ModuleFromQuestionableFrameNotBlacklisted) {
-  const DWORD64 image_base_for_questionable_module = 2048;
-  {
-    // First stack, with both the first and second frames missing unwind info.
-    scoped_ptr<Win32StackFrameUnwinder> unwinder = CreateUnwinder();
-    CONTEXT context = {0};
-    ScopedModuleHandle module;
-    unwind_functions_->SetNoRuntimeFunction(&context);
-    EXPECT_TRUE(unwinder->TryUnwind(&context, &module));
-    EXPECT_TRUE(module.IsValid());
-
-    unwind_functions_->SetNoRuntimeFunction(image_base_for_questionable_module,
-                                            &context);
     EXPECT_FALSE(unwinder->TryUnwind(&context, &module));
   }
-
-  {
-    // Second stack; check that the questionable module was not blacklisted.
-    scoped_ptr<Win32StackFrameUnwinder> unwinder = CreateUnwinder();
-    CONTEXT context = {0};
-    ScopedModuleHandle module;
-    unwind_functions_->SetNoRuntimeFunction(image_base_for_questionable_module,
-                                            &context);
-    EXPECT_TRUE(unwinder->TryUnwind(&context, &module));
-    EXPECT_TRUE(module.IsValid());
-  }
 }
 
 }  // namespace base