fix(stackwalking): Use debug info to inform scanning (#1913)

In an effort to reduce the number of "hallucinated" stack frames, #1651
introduced a sanity check for stack scanning into the minidump
processor: if we have CFI for a module and the stack scanner would
produce a frame that is not covered by that CFI, the frame is discarded.

However, this made the processor too strict—if the CFI for the module is
poor or incomplete, this may lead to us discarding valid frames. With
this PR, we now _additionally_ use debug information in the form of
symcaches to inform stack scanning. The new logic is: if we have both
CFI and debug info for a module, but neither covers the potential
frame's instruction address, then the frame is treated as invalid.

Obviously this involves pulling symcache computation into minidump
stackwalking, which previously didn't need to know about symcaches or
debug info at all. Performance-wise I believe this to be no
problem—symcache computation is cached and would have happened during
the symbolication step anyway, now it just happens earlier. But it does
leave us in a somewhat awkward spot where we compute debug info during
stackwalking but then don't actually fill it in. We may want to rethink
this in the future.

Fixes INGEST-838.

Author: loewenheim

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Bug Fixes
+
+- fix(stackwalking): Use debug info to inform scanning by @loewenheim in [#1905](https://github.com/getsentry/symbolicator/pull/1905)
+
 ## 26.3.1
 
 ### New Features ✨

crates/symbolicator-native/src/symbolication/process_minidump.rs

@@ -24,6 +24,8 @@ use symbolicator_sources::{ObjectId, ObjectType, SourceConfig};
 use tokio::sync::Notify;
 
 use crate::caches::cficaches::{CfiCacheActor, CfiModuleInfo, FetchCfiCache, FetchedCfiCache};
+use crate::caches::derived::DerivedCache;
+use crate::caches::symcaches::{FetchSymCache, OwnedSymCache, SymCacheActor};
 use crate::interface::{
     CompleteObjectInfo, CompletedSymbolicationResponse, ProcessMinidump, RawFrame, RawStacktrace,
     Registers, RewriteRules, SymbolicateStacktraces, SystemInfo,
@@ -156,6 +158,13 @@ struct SymbolicatorSymbolProvider {
     object_type: ObjectType,
     /// The actor used for fetching CFI.
     cficache_actor: CfiCacheActor,
+    /// The actor used for fetching symcaches.
+    ///
+    /// Note that debug information is currently _not_ used to symbolicate frames
+    /// during stackwalking (which `rust-minidump` supports in principle). Rather,
+    /// we use it as a sanity check for `rust-minidump`'s stackwalker. See the
+    /// implementation of `fill_symbol` for details.
+    symcache_actor: SymCacheActor,
     /// The debug ID of the first module in the minidump
     /// (by address).
     ///
@@ -177,6 +186,7 @@ impl SymbolicatorSymbolProvider {
         scope: Scope,
         sources: Arc<[SourceConfig]>,
         cficache_actor: CfiCacheActor,
+        symcache_actor: SymCacheActor,
         object_type: ObjectType,
         first_module_debug_id: Option<DebugId>,
         rewrite_first_module: RewriteRules,
@@ -185,13 +195,37 @@ impl SymbolicatorSymbolProvider {
             scope,
             sources,
             cficache_actor,
+            symcache_actor,
             object_type,
             first_module_debug_id,
             rewrite_first_module,
             cficaches: Default::default(),
         }
     }
 
+    fn object_id_from_module(&self, module: &dyn Module) -> ObjectId {
+        let code_file = non_empty_file_name(&module.code_file());
+        let mut debug_file = module.debug_file().as_deref().and_then(non_empty_file_name);
+
+        // Rewrite the first module's debug file according to the configured rewrite rules.
+        if module.debug_identifier() == self.first_module_debug_id
+            && let Some(new_debug_file) = debug_file
+                .as_ref()
+                .and_then(|debug_file| self.rewrite_first_module.rewrite(debug_file))
+        {
+            debug_file = Some(new_debug_file);
+        }
+
+        ObjectId {
+            code_id: module.code_identifier(),
+            code_file,
+            debug_id: module.debug_identifier(),
+            debug_file,
+            debug_checksum: None,
+            object_type: self.object_type,
+        }
+    }
+
     /// Fetches CFI for the given module, parses it into a `SymbolFile`, and stores it internally.
     async fn load_cfi_module(&self, module: &(dyn Module + Sync)) -> Arc<FetchedCfiCache> {
         let key = LookupKey::new(module);
@@ -220,27 +254,7 @@ impl SymbolicatorSymbolProvider {
 
         let sources = self.sources.clone();
         let scope = self.scope.clone();
-
-        let code_file = non_empty_file_name(&module.code_file());
-        let mut debug_file = module.debug_file().as_deref().and_then(non_empty_file_name);
-
-        // Rewrite the first module's debug file according to the configured rewrite rules.
-        if module.debug_identifier() == self.first_module_debug_id
-            && let Some(new_debug_file) = debug_file
-                .as_ref()
-                .and_then(|debug_file| self.rewrite_first_module.rewrite(debug_file))
-        {
-            debug_file = Some(new_debug_file);
-        }
-
-        let identifier = ObjectId {
-            code_id: module.code_identifier(),
-            code_file,
-            debug_id: module.debug_identifier(),
-            debug_file,
-            debug_checksum: None,
-            object_type: self.object_type,
-        };
+        let identifier = self.object_id_from_module(module);
 
         let cficache = self
             .cficache_actor
@@ -266,51 +280,121 @@ impl SymbolicatorSymbolProvider {
         }
         cficache
     }
-}
 
-#[async_trait]
-impl SymbolProvider for SymbolicatorSymbolProvider {
-    async fn fill_symbol(
+    /// Fetches debug info for the given module and parses it into a `SymCache`.
+    async fn load_symcache(&self, module: &(dyn Module + Sync)) -> DerivedCache<OwnedSymCache> {
+        let sources = self.sources.clone();
+        let scope = self.scope.clone();
+        let identifier = self.object_id_from_module(module);
+
+        self.symcache_actor
+            .fetch(FetchSymCache {
+                object_type: self.object_type,
+                identifier,
+                sources,
+                scope,
+            })
+            // NOTE: this `bind_hub` is important!
+            // `load_symcache` is being called concurrently from `rust-minidump` via
+            // `join_all`. We do need proper isolation of any async task that might
+            // manipulate any Sentry scope.
+            .bind_hub(Hub::new_from_top(Hub::current()))
+            .await
+    }
+
+    /// Try to validate the given frame's instruction address against the CFI for the module
+    /// (if we have it).
+    ///
+    /// Returns `false` if we have CFI for the module, but that CFI doesn't cover the frame's
+    /// instruction address. Returns `true` in any other case.
+    ///
+    /// This helps the `rust-minidump`'s stack-walker to make better decisions.
+    /// If the successfully loaded CFI unwind info can not find the potential instruction
+    /// the passed instruction is most likely not a valid instruction from this module.
+    /// But since the instruction maps into the range of this module, we know it cannot
+    /// be part of a different module either -> it's not a valid instruction.
+    ///
+    /// See: <https://github.com/rust-minidump/rust-minidump/blob/32e01a0e54d987025aa64486ebc4154f7f6b16d2/minidump-unwind/src/lib.rs#L865-L877>
+    async fn check_frame_by_cfi(
         &self,
         module: &(dyn Module + Sync),
         frame: &mut (dyn FrameSymbolizer + Send),
-    ) -> Result<(), FillSymbolError> {
-        // This function is being called for every context frame,
+    ) -> bool {
+        // This function is being called (through `fill_symbol`) for every context frame,
         // regardless if any stack walking happens.
         // In contrast, `walk_frame` below will be skipped in case the minidump
         // does not contain any actionable stack memory that would allow stack walking.
         // Loading this module here means we will be able to backfill a possibly missing
         // debug_id/file.
         let cfi_module = self.load_cfi_module(module).await;
 
-        // Try to validate the given instruction against the CFI module contents.
-        //
-        // This helps the `rust-minidump`'s stack-walker to make better decisions.
-        // Returning an error here will aggressively create frames.
-        // To indicate a that a potential instruction is not valid, we can return success
-        // here but also not fill in the symbol name.
-        //
-        // If the successfully loaded CFI unwind info can not find the potential instruction
-        // the passed instruction is most likely not a valid instruction from this module.
-        // But since the instruction maps into the range of this module, we know it cannot
-        // be part of a different module either -> it's not a valid instruction.
-        // Returning success in this case, means the frame will be skipped.
-        //
-        // See: https://github.com/rust-minidump/rust-minidump/blob/32e01a0e54d987025aa64486ebc4154f7f6b16d2/minidump-unwind/src/lib.rs#L865-L877
         if let Ok(Some(cached)) = &cfi_module.cache {
             let cfi = &cached.0.cfi_stack_info;
             let instruction = frame.get_instruction() - module.base_address();
 
             if !cfi.is_empty() && cfi.get(instruction).is_none() {
                 // We definitely do have CFI information loaded, but the given instruction does not
                 // map to any stack frame info.
-                return Ok(());
+                return false;
             }
         }
 
-        // In any other case, always return an error here to signal that we have no useful symbol information to
-        // contribute. Doing nothing would stop the stack scanning prematurely.
-        Err(FillSymbolError {})
+        true
+    }
+
+    /// Try to validate the given frame's instruction address against the debug information
+    /// for the module (if we have it).
+    ///
+    /// Returns `false` if we have debug info for the module, but that debug info doesn't cover the frame's
+    /// instruction address. Returns `true` in any other case.
+    ///
+    /// This helps the `rust-minidump`'s stack-walker to make better decisions.
+    /// If the successfully loaded symcache can not find the potential instruction
+    /// the passed instruction is most likely not a valid instruction from this module.
+    /// But since the instruction maps into the range of this module, we know it cannot
+    /// be part of a different module either -> it's not a valid instruction.
+    ///
+    /// See: <https://github.com/rust-minidump/rust-minidump/blob/32e01a0e54d987025aa64486ebc4154f7f6b16d2/minidump-unwind/src/lib.rs#L865-L877>
+    async fn check_frame_by_symbol_info(
+        &self,
+        module: &(dyn Module + Sync),
+        frame: &mut (dyn FrameSymbolizer + Send),
+    ) -> bool {
+        let symcache = self.load_symcache(module).await;
+
+        if let Ok(cached) = &symcache.cache {
+            let instruction = frame.get_instruction() - module.base_address();
+
+            if cached.get().lookup(instruction).next().is_none() {
+                // We definitely do have symbol information loaded, but the given instruction does not
+                // map to any symbol info.
+                return false;
+            }
+        }
+
+        true
+    }
+}
+
+#[async_trait]
+impl SymbolProvider for SymbolicatorSymbolProvider {
+    async fn fill_symbol(
+        &self,
+        module: &(dyn Module + Sync),
+        frame: &mut (dyn FrameSymbolizer + Send),
+    ) -> Result<(), FillSymbolError> {
+        if self.check_frame_by_cfi(module, frame).await
+            || self.check_frame_by_symbol_info(module, frame).await
+        {
+            // If either CFI or symbol info does not reject the frame, return an error here to signal that
+            // we have no useful symbol information to contribute. Doing nothing would stop the stack scanning prematurely.
+            Err(FillSymbolError {})
+        } else {
+            // To indicate a that a potential instruction is not valid, we can return success
+            // here but also not fill in the symbol name. This will cause `rust-minidump`'s stack
+            // scanner to disregard the frame.
+            Ok(())
+        }
     }
 
     async fn walk_frame(
@@ -359,6 +443,7 @@ fn object_info_from_minidump_module(ty: ObjectType, module: &MinidumpModule) ->
 
 async fn stackwalk(
     cficaches: CfiCacheActor,
+    symcaches: SymCacheActor,
     minidump: &Minidump,
     scope: Scope,
     sources: Arc<[SourceConfig]>,
@@ -387,6 +472,7 @@ async fn stackwalk(
         scope,
         sources,
         cficaches,
+        symcaches,
         ty,
         first_module_debug_id,
         rewrite_first_module,
@@ -540,6 +626,7 @@ impl SymbolicationActor {
             duration,
         } = stackwalk(
             self.cficaches.clone(),
+            self.symcaches.clone(),
             &minidump,
             scope.clone(),
             sources.clone(),

crates/symbolicator-native/src/symbolication/symbolicate.rs

@@ -32,7 +32,7 @@ use super::native::{get_relative_caller_addr, symbolicate_native_frame};
 pub struct SymbolicationActor {
     demangle_cache: DemangleCache,
     pub(crate) objects: ObjectsActor,
-    symcaches: SymCacheActor,
+    pub(crate) symcaches: SymCacheActor,
     pub(crate) cficaches: CfiCacheActor,
     ppdb_caches: PortablePdbCacheActor,
     pub(crate) sourcefiles_cache: Arc<SourceFilesCache>,

crates/symbolicator-native/tests/integration/process_minidump.rs

@@ -48,6 +48,39 @@ async fn test_minidump_macos() {
     assert_snapshot!(res);
 }
 
+/// Verifies that debug information is used to guide stack scanning.
+///
+/// The minidump being tested has frame pointers disabled, so in the absence of (usable) CFI,
+/// there is no choice but to scan. Left to its own devices, the stack scanner can produce a lot
+/// of spurious frames, so we want to use both CFI and debug info that we have available to guide
+/// it. The heuristic is as follows: if stack scanning would produce an instruction address
+/// * falling into some module,
+/// * for which we have CFI or debug info,
+/// * which doesn't cover that instruction address,
+/// then that address is probably bogus and we discard the frame.
+///
+/// The CFI half of this heuristic was implemented in
+/// https://github.com/getsentry/symbolicator/pull/1651.
+/// https://github.com/getsentry/symbolicator/pull/1913 added the debug info part.
+///
+/// This test simulates an actual customer situation: we have both debug info and CFI
+/// for the minidump, but the CFI is truncated/poor. If we only used CFI to constrain the
+/// stack scanner, all frames in `sentry_example` would be rejected. However, since we also use
+/// the (good) debug info to check the frames, they are retained.
+///
+/// The `dyld` frames in the stacktrace are found by scanning, and since we have no CFI or
+/// debug info at all for that module, they're all accepted.
+#[tokio::test]
+async fn test_minidump_macos_broken_cfi() {
+    let res = stackwalk_minidump("macos-no-fp.dmp").await;
+    let crashed_thread = res
+        .stacktraces
+        .iter()
+        .find(|st| st.is_requesting.unwrap_or_default())
+        .unwrap();
+    assert_snapshot!(crashed_thread);
+}
+
 #[tokio::test]
 async fn test_minidump_linux() {
     let res = stackwalk_minidump("linux.dmp").await;