Repository: getsentry/symbolic
Pull request #897 (2025-02-10 17:50:28)

fix(symcache): Explicitly map "holes" between functions
Currently, if we look up an address that falls "between" two functions/symbols, we will always resolve it to the first of the two functions/symbols because each range implicitly extends to the start of the next one. This can lead to bad user experience when we map (invalid) addresses to entirely nonsensical functions.

Therefore, we now explicitly insert a mapping at the end of the function that maps to "nowhere" to denote the function's end.

github-actions[bot]: 
<!--
  1 failure:  Please consider a...
  0 warning: 
  
  1 markdown notices
  DangerID: danger-id-Danger;
-->

<table>
  <thead>
    <tr>
      <th width="50"></th>
      <th width="100%" data-danger-table="true">Fails</th>
    </tr>
  </thead>
  <tbody><tr>
      <td>:no_entry_sign:</td>
      <td>Please consider adding a changelog entry for the next release.</td>
    </tr>
  </tbody>
</table>



### Instructions and example for changelog

Please add an entry to `CHANGELOG.md` to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

```markdown
## Unreleased

- Explicitly map "holes" between functions ([#897](https://github.com/getsentry/symbolic/pull/897))
```

If none of the above apply, you can opt out of this check by adding `#skip-changelog` to the PR description.
<p align="right">
  Generated by :no_entry_sign: <a href="https://danger.systems/js">dangerJS</a> against 22979db6a3453db16e5e96b66db330cbb7aecd78
</p>

loewenheim: > the code and logic looks reasonable. you folks mentioned a slack conversation? can you distill that down to an explanation of what exactly is happening here?

The essence is that we have a user case where we are getting a frame with an address that falls "between" functions. Under the current logic, we map it to the last function before it and that creates an entirely nonsensical frame (the function we're finding has nothing to do with the error that happened). The user thinks that this is caused by stack corruption.
Swatinem: Well yes, so much is obvious. I was rather wondering what legitimate reason there might be that a return addr on the stack is pointing past the end of the function?
I mentioned my hunch that the `call` instruction never assumes to be returned to. This should be verifyable by looking at a disassembler to see that the last instruction of the function is the `call`, which implicitly pushes the addr *after* that instruction on the stack.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e433b4f4..58d338f4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+**Fixes**
+
+- symcache: Explicitly map "holes" between functions ([#897](https://github.com/getsentry/symbolic/pull/897))
+
 ## 12.13.4
 
 **Fixes**
diff --git a/symbolic-symcache/src/writer.rs b/symbolic-symcache/src/writer.rs
index e6c95c4c..170c6472 100644
--- a/symbolic-symcache/src/writer.rs
+++ b/symbolic-symcache/src/writer.rs
@@ -10,6 +10,7 @@ use symbolic_debuginfo::{DebugSession, FileFormat, Function, ObjectLike, Symbol}
 use watto::{Pod, StringTable, Writer};
 
 use super::{raw, transform};
+use crate::raw::NO_SOURCE_LOCATION;
 use crate::{Error, ErrorKind};
 
 /// The SymCache Converter.
@@ -373,6 +374,19 @@ impl<'a> SymCacheConverter<'a> {
         if function_end > *last_addr {
             *last_addr = function_end;
         }
+
+        // Insert an explicit "empty" mapping for the end of the function.
+        // This is to ensure that addresses that fall "between" functions don't get
+        // erroneously mapped to the previous function.
+        //
+        // We only do this if there is no previous mapping for the end address—we don't
+        // want to overwrite valid mappings.
+        //
+        // If the next function starts right at this function's end, that's no trouble,
+        // it will just overwrite this mapping with one of its ranges.
+        if let btree_map::Entry::Vacant(vacant_entry) = self.ranges.entry(function_end) {
+            vacant_entry.insert(NO_SOURCE_LOCATION);
+        }
     }
 
     /// Processes an individual [`Symbol`].
@@ -429,6 +443,22 @@ impl<'a> SymCacheConverter<'a> {
         if symbol.address as u32 >= *last_addr {
             self.last_addr = None;
         }
+
+        // Insert an explicit "empty" mapping for the end of the symbol.
+        // This is to ensure that addresses that fall "between" symbols don't get
+        // erroneously mapped to the previous symbol.
+        //
+        // We only do this if there is no previous mapping for the end address—we don't
+        // want to overwrite valid mappings.
+        //
+        // If the next symbol starts right at this symbols's end, that's no trouble,
+        // it will just overwrite this mapping.
+        if symbol.size > 0 {
+            let end_address = (symbol.address + symbol.size) as u32;
+            if let btree_map::Entry::Vacant(vacant_entry) = self.ranges.entry(end_address) {
+                vacant_entry.insert(NO_SOURCE_LOCATION);
+            }
+        }
     }
 
     // Methods for serializing to a [`Write`] below:


Test patch:
diff --git a/py/tests/test_crashprobe.py b/py/tests/test_crashprobe.py
index 16a3a43d..99c67aa8 100644
--- a/py/tests/test_crashprobe.py
+++ b/py/tests/test_crashprobe.py
@@ -364,7 +364,7 @@ def test_jump_into_an_nx_page(res_path, make_report_sym, version, build, arch):
 
     # So let's assert for the second best
     else:
-        assert basename(bt[0]["full_path"]) is None
+        assert "full_path" not in bt[0]
         assert bt[0]["line"] in (None, 0)
 
     _test_doCrash_call(bt)
diff --git a/symbolic-symcache/tests/test_writer.rs b/symbolic-symcache/tests/test_writer.rs
index 95c08bc0..a95196a6 100644
--- a/symbolic-symcache/tests/test_writer.rs
+++ b/symbolic-symcache/tests/test_writer.rs
@@ -23,7 +23,7 @@ fn test_write_header_linux() -> Result<(), Error> {
     }
 
     let symcache = SymCache::parse(&buffer)?;
-    insta::assert_debug_snapshot!(symcache, @r#"
+    insta::assert_debug_snapshot!(symcache, @r###"
     SymCache {
         version: 8,
         debug_id: DebugId {
@@ -33,11 +33,11 @@ fn test_write_header_linux() -> Result<(), Error> {
         arch: Amd64,
         files: 55,
         functions: 697,
-        source_locations: 8284,
-        ranges: 6828,
+        source_locations: 8431,
+        ranges: 6975,
         string_bytes: 49877,
     }
-    "#);
+    "###);
 
     Ok(())
 }
@@ -67,7 +67,7 @@ fn test_write_header_macos() -> Result<(), Error> {
     converter.process_object(&object)?;
     converter.serialize(&mut Cursor::new(&mut buffer))?;
     let symcache = SymCache::parse(&buffer)?;
-    insta::assert_debug_snapshot!(symcache, @r#"
+    insta::assert_debug_snapshot!(symcache, @r###"
     SymCache {
         version: 8,
         debug_id: DebugId {
@@ -77,11 +77,11 @@ fn test_write_header_macos() -> Result<(), Error> {
         arch: Amd64,
         files: 36,
         functions: 639,
-        source_locations: 7199,
-        ranges: 5782,
+        source_locations: 7382,
+        ranges: 5965,
         string_bytes: 40958,
     }
-    "#);
+    "###);
 
     Ok(())
 }
@@ -351,3 +351,26 @@ fn test_trailing_marker() -> Result<(), Error> {
 
     Ok(())
 }
+
+/// Tests that addresses between functions are unmapped. See
+/// <https://github.com/getsentry/symbolic/pull/897>.
+#[test]
+fn test_lookup_between_functions() {
+    // This file is a Python fixture
+    let buffer = ByteView::open(
+        "../py/tests/res/ext/1.4.1/release/dSYMs/3CD3E3CC-281E-3AF3-84EB-CDE6D56F0559.dSYM/Contents/Resources/DWARF/CrashLibiOS",
+    )
+    .unwrap();
+    let object = Object::parse(&buffer).unwrap();
+
+    let mut buffer = Vec::new();
+    let mut converter = SymCacheConverter::new();
+    converter.process_object(&object).unwrap();
+    converter.serialize(&mut Cursor::new(&mut buffer)).unwrap();
+    let symcache = SymCache::parse(&buffer).unwrap();
+    // This address is exactly at the end of the function "-[CRLCrashNXPage desc]",
+    // which starts at 0x8b0c and has size 0x2c. The next function,
+    // "-[CRLCrashStackGuard category]", starts at 0x8b3c.
+    let symbols = symcache.lookup(0x8b38).collect::<Vec<_>>();
+    assert!(symbols.is_empty());
+}