Repository: luser/rust-minidump
Pull request #461 (2022-02-18 14:18:07)

MinidumpModule: Don't return DebugId for invalid build_id
The minidump processor currently returns modules with debug ids of `Some(00000000-0000-0000-0000-000000000000)`. This fixes that case by checking that the `build_id` is not just a list of zeros.


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 1c088a3..7cf1a4c 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -201,9 +201,9 @@ dependencies = [
 
 [[package]]
 name = "debugid"
-version = "0.7.2"
+version = "0.7.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f91cf5a8c2f2097e2a32627123508635d47ce10563d999ec1a95addf08b502ba"
+checksum = "d6ee87af31d84ef885378aebca32be3d682b0e0dc119d5b4860a2c5bb5046730"
 dependencies = [
  "uuid",
 ]
diff --git a/breakpad-symbols/Cargo.toml b/breakpad-symbols/Cargo.toml
index 44e3d0e..80846df 100644
--- a/breakpad-symbols/Cargo.toml
+++ b/breakpad-symbols/Cargo.toml
@@ -16,7 +16,7 @@ travis-ci = { repository = "luser/rust-minidump" }
 [dependencies]
 async-trait = "0.1.51"
 circular = "0.3.0"
-debugid = "0.7"
+debugid = "0.7.3"
 log = "0.4.1"
 minidump-common = { version = "0.9.6", path = "../minidump-common" }
 nom = "~1.2.2"
diff --git a/minidump-common/Cargo.toml b/minidump-common/Cargo.toml
index abaf745..0d76ea1 100644
--- a/minidump-common/Cargo.toml
+++ b/minidump-common/Cargo.toml
@@ -15,7 +15,7 @@ travis-ci = { repository = "luser/rust-minidump" }
 [dependencies]
 arbitrary = { version = "1", optional = true, features = ["derive"] }
 bitflags = "1.3.2"
-debugid = "0.7"
+debugid = "0.7.3"
 enum-primitive-derive = "0.2.2"
 log = "0.4.1"
 num-traits = "0.2"
diff --git a/minidump-processor/Cargo.toml b/minidump-processor/Cargo.toml
index d3a9c61..adda3d2 100644
--- a/minidump-processor/Cargo.toml
+++ b/minidump-processor/Cargo.toml
@@ -23,7 +23,7 @@ symbolic-syms = []
 async-trait = "0.1.51"
 breakpad-symbols = { version = "0.9.6", path = "../breakpad-symbols", optional = true }
 clap = "2.34"
-debugid = "0.7"
+debugid = "0.7.3"
 log = "0.4"
 memmap2 = "0.5.3"
 minidump = { version = "0.9.6", path = "../minidump" }
diff --git a/minidump-processor/fuzz/Cargo.lock b/minidump-processor/fuzz/Cargo.lock
index e5b8ac6..f800432 100644
--- a/minidump-processor/fuzz/Cargo.lock
+++ b/minidump-processor/fuzz/Cargo.lock
@@ -85,6 +85,7 @@ version = "0.9.6"
 dependencies = [
  "async-trait",
  "circular",
+ "debugid",
  "log",
  "minidump-common",
  "nom",
@@ -170,6 +171,15 @@ dependencies = [
  "cfg-if",
 ]
 
+[[package]]
+name = "debugid"
+version = "0.7.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d6ee87af31d84ef885378aebca32be3d682b0e0dc119d5b4860a2c5bb5046730"
+dependencies = [
+ "uuid",
+]
+
 [[package]]
 name = "derive_arbitrary"
 version = "1.0.2"
@@ -564,9 +574,9 @@ checksum = "308cc39be01b73d0d18f82a0e7b2a3df85245f84af96fdddc5d202d27e47b86a"
 
 [[package]]
 name = "memmap2"
-version = "0.5.2"
+version = "0.5.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "fe3179b85e1fd8b14447cbebadb75e45a1002f541b925f0bfec366d56a81c56d"
+checksum = "057a3db23999c867821a7a59feb06a578fcb03685e983dff90daf9e7d24ac08f"
 dependencies = [
  "libc",
 ]
@@ -582,6 +592,7 @@ name = "minidump"
 version = "0.9.6"
 dependencies = [
  "arbitrary",
+ "debugid",
  "encoding",
  "log",
  "memmap2",
@@ -591,6 +602,7 @@ dependencies = [
  "scroll",
  "thiserror",
  "time",
+ "uuid",
 ]
 
 [[package]]
@@ -599,6 +611,7 @@ version = "0.9.6"
 dependencies = [
  "arbitrary",
  "bitflags",
+ "debugid",
  "enum-primitive-derive",
  "log",
  "num-traits 0.2.14",
@@ -614,6 +627,7 @@ dependencies = [
  "async-trait",
  "breakpad-symbols",
  "clap",
+ "debugid",
  "log",
  "memmap2",
  "minidump",
@@ -1207,6 +1221,12 @@ dependencies = [
  "percent-encoding",
 ]
 
+[[package]]
+name = "uuid"
+version = "0.8.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "bc5cf98d8186244414c848017f0e2676b3fcb46807f6668a97dfe67359a3c4b7"
+
 [[package]]
 name = "vcpkg"
 version = "0.2.15"
diff --git a/minidump-processor/src/stackwalker/amd64.rs b/minidump-processor/src/stackwalker/amd64.rs
index fc9eb66..65eb8b0 100644
--- a/minidump-processor/src/stackwalker/amd64.rs
+++ b/minidump-processor/src/stackwalker/amd64.rs
@@ -281,24 +281,31 @@ where
             // It's unclear why, perhaps the test is buggy, but for now we
             // preserve that behaviour.
             if let Some(last_bp) = last_bp {
-                let address_of_bp = address_of_ip - POINTER_WIDTH;
-                // Can assume this resolves because we already walked over it when
-                // checking address_of_ip values.
-                let bp = stack_memory.get_memory_at_address(address_of_bp as u64)?;
-                if last_bp == address_of_bp
-                    && bp > address_of_ip
-                    && bp - address_of_bp <= MAX_REASONABLE_GAP_BETWEEN_FRAMES
-                {
-                    // Final sanity check that resulting bp is still inside stack memory.
-                    if stack_memory
-                        .get_memory_at_address::<Pointer>(bp as u64)
-                        .is_some()
+                // If we're on the first iteration of the scan, there can't possibly be a
+                // frame pointer, because the entire stack frame is taken up by the return
+                // pointer. And if we're not on the first iteration, then the last iteration
+                // already loaded the location we expect the frame pointer to be in, so we can
+                // unconditionally load it here.
+                if i > 0 {
+                    let address_of_bp = address_of_ip - POINTER_WIDTH;
+                    // Can assume this resolves because we already walked over it when
+                    // checking address_of_ip values.
+                    let bp = stack_memory.get_memory_at_address(address_of_bp as u64)?;
+                    if last_bp == address_of_bp
+                        && bp > address_of_ip
+                        && bp - address_of_bp <= MAX_REASONABLE_GAP_BETWEEN_FRAMES
                     {
-                        caller_bp = Some(bp);
+                        // Final sanity check that resulting bp is still inside stack memory.
+                        if stack_memory
+                            .get_memory_at_address::<Pointer>(bp as u64)
+                            .is_some()
+                        {
+                            caller_bp = Some(bp);
+                        }
+                    } else if last_bp >= caller_sp {
+                        // Don't sanity check that the address is inside the stack? Hmm.
+                        caller_bp = Some(last_bp);
                     }
-                } else if last_bp >= caller_sp {
-                    // Don't sanity check that the address is inside the stack? Hmm.
-                    caller_bp = Some(last_bp);
                 }
             }
 
diff --git a/minidump-processor/src/stackwalker/x86.rs b/minidump-processor/src/stackwalker/x86.rs
index ebecb4e..a88df08 100644
--- a/minidump-processor/src/stackwalker/x86.rs
+++ b/minidump-processor/src/stackwalker/x86.rs
@@ -268,24 +268,31 @@ where
             // smaller than 128 KB.
             const MAX_REASONABLE_GAP_BETWEEN_FRAMES: Pointer = 128 * 1024;
 
-            let address_of_bp = address_of_ip - POINTER_WIDTH;
-            let bp = stack_memory.get_memory_at_address(address_of_bp as u64)?;
-            if bp > address_of_ip && bp - address_of_bp <= MAX_REASONABLE_GAP_BETWEEN_FRAMES {
-                // Sanity check that resulting bp is still inside stack memory.
-                if stack_memory
-                    .get_memory_at_address::<Pointer>(bp as u64)
-                    .is_some()
-                {
-                    caller_bp = Some(bp);
-                }
-            } else if let Some(last_bp) = last_bp {
-                if last_bp >= caller_sp {
+            // If we're on the first iteration of the scan, there can't possibly be a frame pointer,
+            // because the entire stack frame is taken up by the return pointer. And if we're
+            // not on the first iteration, then the last iteration already loaded the location
+            // we expect the frame pointer to be in, so we can unconditionally load it here.
+            if i > 0 {
+                let address_of_bp = address_of_ip - POINTER_WIDTH;
+                let bp = stack_memory.get_memory_at_address(address_of_bp as u64)?;
+
+                if bp > address_of_ip && bp - address_of_bp <= MAX_REASONABLE_GAP_BETWEEN_FRAMES {
                     // Sanity check that resulting bp is still inside stack memory.
                     if stack_memory
-                        .get_memory_at_address::<Pointer>(last_bp as u64)
+                        .get_memory_at_address::<Pointer>(bp as u64)
                         .is_some()
                     {
-                        caller_bp = Some(last_bp);
+                        caller_bp = Some(bp);
+                    }
+                } else if let Some(last_bp) = last_bp {
+                    if last_bp >= caller_sp {
+                        // Sanity check that resulting bp is still inside stack memory.
+                        if stack_memory
+                            .get_memory_at_address::<Pointer>(last_bp as u64)
+                            .is_some()
+                        {
+                            caller_bp = Some(last_bp);
+                        }
                     }
                 }
             }
diff --git a/minidump/Cargo.toml b/minidump/Cargo.toml
index f8e2ca0..f93f6ff 100644
--- a/minidump/Cargo.toml
+++ b/minidump/Cargo.toml
@@ -13,7 +13,7 @@ edition = "2018"
 
 [dependencies]
 arbitrary = { version = "1", optional = true, features = ["derive"] }
-debugid = "0.7"
+debugid = "0.7.3"
 encoding = "0.2"
 log = "0.4.1"
 memmap2 = "0.5.3"
diff --git a/minidump/src/minidump.rs b/minidump/src/minidump.rs
index 1105704..8898470 100644
--- a/minidump/src/minidump.rs
+++ b/minidump/src/minidump.rs
@@ -959,11 +959,14 @@ impl Module for MinidumpModule {
                 .ok()?;
                 Some(DebugId::from_parts(uuid, raw.age))
             }
-            Some(CodeView::Pdb20(ref raw)) => {
-                let uuid = Uuid::from_fields(raw.signature, 0, 0, &[0, 0, 0, 0]).ok()?;
-                Some(DebugId::from_parts(uuid, raw.age))
-            }
+            Some(CodeView::Pdb20(ref raw)) => Some(DebugId::from_pdb20(raw.signature, raw.age)),
             Some(CodeView::Elf(ref raw)) => {
+                // For empty or trivial `build_id`s, we don't want to return a `DebugId`.
+                // This can happen for mapped files that aren't executable, like fonts or .jar files.
+                if raw.build_id.iter().all(|byte| *byte == 0) {
+                    return None;
+                }
+
                 // For backwards-compat (Linux minidumps have historically
                 // been written using PDB70 CodeView info), treat build_id
                 // as if the first 16 bytes were a GUID.
@@ -4869,6 +4872,48 @@ mod test {
         );
     }
 
+    #[test]
+    fn test_module_list_pdb20() {
+        let name = DumpString::new("single module", Endian::Little);
+        let cv_record = Section::with_endian(Endian::Little)
+            .D32(md::CvSignature::Pdb20 as u32) // cv_signature
+            .D32(0x0) // cv_offset
+            .D32(0xabcd1234) // signature
+            .D32(1) // age
+            .append_bytes(b"c:\\foo\\file.pdb\0"); // pdb_file_name
+        let module = SynthModule::new(
+            Endian::Little,
+            0xa90206ca83eb2852,
+            0xada542bd,
+            &name,
+            0xb1054d2a,
+            0x34571371,
+            Some(&STOCK_VERSION_INFO),
+        )
+        .cv_record(&cv_record);
+        let dump = SynthMinidump::with_endian(Endian::Little)
+            .add_module(module)
+            .add(name)
+            .add(cv_record);
+        let dump = read_synth_dump(dump).unwrap();
+        let module_list = dump.get_stream::<MinidumpModuleList>().unwrap();
+        let modules = module_list.iter().collect::<Vec<_>>();
+        assert_eq!(modules.len(), 1);
+        assert_eq!(modules[0].base_address(), 0xa90206ca83eb2852);
+        assert_eq!(modules[0].size(), 0xada542bd);
+        assert_eq!(modules[0].code_file(), "single module");
+        // time_date_stamp and size_of_image concatenated
+        assert_eq!(
+            modules[0].code_identifier(),
+            CodeId::new("B1054D2Aada542bd".to_string())
+        );
+        assert_eq!(modules[0].debug_file().unwrap(), "c:\\foo\\file.pdb");
+        assert_eq!(
+            modules[0].debug_identifier().unwrap(),
+            DebugId::from_pdb20(0xabcd1234, 1)
+        );
+    }
+
     #[test]
     fn test_unloaded_module_list() {
         let name = DumpString::new("single module", Endian::Little);
@@ -5921,6 +5966,35 @@ c70206ca83eb2852-de0206ca83eb2852  -w-s  10bac9000 fd:05 1196511 /usr/lib64/libt
         assert_eq!(modules[0].debug_file().unwrap(), "helpivecrashed.dylib");
     }
 
+    #[test]
+    fn test_null_id() {
+        // Add a module with an ELF build id of nothing but zeros
+        let name1 = DumpString::new("module 1", Endian::Little);
+        const MODULE1_BUILD_ID: &[u8] = &[0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00];
+        let cv_record1 = Section::with_endian(Endian::Little)
+            .D32(md::CvSignature::Elf as u32) // signature
+            .append_bytes(MODULE1_BUILD_ID);
+        let module1 = SynthModule::new(
+            Endian::Little,
+            0x100000000,
+            0x4000,
+            &name1,
+            0xb1054d2a,
+            0x34571371,
+            Some(&STOCK_VERSION_INFO),
+        )
+        .cv_record(&cv_record1);
+        let dump = SynthMinidump::with_endian(Endian::Little)
+            .add_module(module1)
+            .add(name1)
+            .add(cv_record1);
+        let dump = read_synth_dump(dump).unwrap();
+        let module_list = dump.get_stream::<MinidumpModuleList>().unwrap();
+        let modules = module_list.iter().collect::<Vec<_>>();
+
+        assert!(modules[0].debug_identifier().is_none());
+    }
+
     #[test]
     fn test_thread_list_x86() {
         let context = synth_minidump::x86_context(Endian::Little, 0xabcd1234, 0x1010);


Test patch:
diff --git a/minidump-processor/src/stackwalker/amd64_unittest.rs b/minidump-processor/src/stackwalker/amd64_unittest.rs
index 6964eb9..f1791f6 100644
--- a/minidump-processor/src/stackwalker/amd64_unittest.rs
+++ b/minidump-processor/src/stackwalker/amd64_unittest.rs
@@ -663,3 +663,65 @@ async fn test_frame_pointer_barely_no_overflow() {
         }
     }
 }
+
+#[tokio::test]
+async fn test_scan_walk_overflow() {
+    // There's a possible overflow when address_of_ip starts out at 0.
+    //
+    // To avoid this, we only try to recover rbp when we're scanning at least
+    // 1 pointer width away from the start of the stack.
+    let mut f = TestFixture::new();
+    let mut stack = Section::new();
+    let stack_start = 0;
+    stack.start().set_const(stack_start);
+
+    let return_address1 = 0x00007500b0000100_u64;
+
+    let frame1_sp = Label::new();
+    let frame1_rbp = Label::new();
+
+    stack = stack
+        // frame 0
+        .D64(return_address1) // actual return address
+        // frame 1
+        .mark(&frame1_sp)
+        .append_repeated(0, 16) // space
+        .D64(0x00007400b0000000) // more junk
+        .D64(0x00007500d0000000)
+        .mark(&frame1_rbp);
+
+    f.raw.rip = 0x00007400c0000200;
+    f.raw.rbp = frame1_rbp.value().unwrap();
+    f.raw.rsp = stack.start().value().unwrap();
+
+    let s = f.walk_stack(stack).await;
+    assert_eq!(s.frames.len(), 2);
+
+    {
+        // To avoid reusing locals by mistake
+        let f0 = &s.frames[0];
+        assert_eq!(f0.trust, FrameTrust::Context);
+        assert_eq!(f0.context.valid, MinidumpContextValidity::All);
+    }
+
+    {
+        // To avoid reusing locals by mistake
+        let f1 = &s.frames[1];
+        assert_eq!(f1.trust, FrameTrust::Scan);
+        if let MinidumpContextValidity::Some(ref which) = f1.context.valid {
+            assert!(which.contains("rip"));
+            assert!(which.contains("rsp"));
+        } else {
+            unreachable!();
+        }
+
+        if let MinidumpRawContext::Amd64(ctx) = &f1.context.raw {
+            assert_eq!(ctx.rip, return_address1);
+            assert_eq!(ctx.rsp, frame1_sp.value().unwrap());
+            // We were unable to recover rbp, so it defaulted to 0.
+            assert_eq!(ctx.rbp, 0);
+        } else {
+            unreachable!();
+        }
+    }
+}