Repository: luser/rust-minidump
Pull request #475 (2022-02-28 12:50:04)

Don't return nil DebugIds for Pdb70

Gankra: Is this part of #474, or a different problem? (Any chance for a test?)
Gankra: >  I should really remember to do this before pushing.

It's ok, as long as you don't mind me reminding you 😸 

(I am often strategically lazy to get over the mental barrier of some difficult task, relying on my reviewers to point out when I've gone too far so that I have external "motivation" to do the Right Thing... brains are weird.)

Patch:
diff --git a/breakpad-symbols/src/lib.rs b/breakpad-symbols/src/lib.rs
index a0b93ec..90c2bdc 100644
--- a/breakpad-symbols/src/lib.rs
+++ b/breakpad-symbols/src/lib.rs
@@ -121,11 +121,8 @@ impl Module for SimpleModule {
             .as_ref()
             .map_or(Cow::from(""), |s| Cow::Borrowed(&s[..]))
     }
-    fn code_identifier(&self) -> CodeId {
-        self.code_identifier
-            .as_ref()
-            .cloned()
-            .unwrap_or_else(CodeId::nil)
+    fn code_identifier(&self) -> Option<CodeId> {
+        self.code_identifier.as_ref().cloned()
     }
     fn debug_file(&self) -> Option<Cow<str>> {
         self.debug_file.as_ref().map(|s| Cow::Borrowed(&s[..]))
@@ -601,13 +598,13 @@ impl FrameSymbolizer for SimpleFrame {
 
 // Can't make Module derive Hash, since then it can't be used as a trait
 // object (because the hash method is generic), so this is a hacky workaround.
-type ModuleKey = (String, String, Option<String>, Option<String>);
+type ModuleKey = (String, Option<String>, Option<String>, Option<String>);
 
 /// Helper for deriving a hash key from a `Module` for `Symbolizer`.
 fn key(module: &(dyn Module + Sync)) -> ModuleKey {
     (
         module.code_file().to_string(),
-        module.code_identifier().to_string(),
+        module.code_identifier().map(|s| s.to_string()),
         module.debug_file().map(|s| s.to_string()),
         module.debug_identifier().map(|s| s.to_string()),
     )
diff --git a/minidump-common/src/traits.rs b/minidump-common/src/traits.rs
index 3e5c25e..541e37d 100644
--- a/minidump-common/src/traits.rs
+++ b/minidump-common/src/traits.rs
@@ -19,7 +19,7 @@ pub trait Module {
     /// builds of the same code module.  This may contain a uuid, timestamp,
     /// version number, or any combination of this or other information, in an
     /// implementation-defined format.
-    fn code_identifier(&self) -> CodeId;
+    fn code_identifier(&self) -> Option<CodeId>;
     /// The filename containing debugging information associated with the code
     /// module.  If debugging information is stored in a file separate from the
     /// code module itself (as is the case when .pdb or .dSYM files are used),
@@ -49,8 +49,8 @@ impl<'a> Module for (&'a str, DebugId) {
     fn code_file(&self) -> Cow<str> {
         Cow::Borrowed("")
     }
-    fn code_identifier(&self) -> CodeId {
-        CodeId::nil()
+    fn code_identifier(&self) -> Option<CodeId> {
+        None
     }
     fn debug_file(&self) -> Option<Cow<str>> {
         let &(file, _id) = self;
diff --git a/minidump-processor/src/process_state.rs b/minidump-processor/src/process_state.rs
index 1bac5b7..cd513f2 100644
--- a/minidump-processor/src/process_state.rs
+++ b/minidump-processor/src/process_state.rs
@@ -810,7 +810,7 @@ Unknown streams encountered:
                     "debug_id": module.debug_identifier().unwrap_or_default().breakpad().to_string(),
                     "end_addr": json_hex(module.raw.base_of_image + module.raw.size_of_image as u64),
                     "filename": &name,
-                    "code_id": module.code_identifier().as_str(),
+                    "code_id": module.code_identifier().unwrap_or_default().as_str(),
                     "version": module.version(),
                     // [UNSTABLE:evil_json]
                     "cert_subject": self.cert_info.get(name),
@@ -872,7 +872,7 @@ Unknown streams encountered:
 
             "unloaded_modules": self.unloaded_modules.iter().map(|module| json!({
                 "base_addr": json_hex(module.raw.base_of_image),
-                "code_id": module.code_identifier().as_str(),
+                "code_id": module.code_identifier().unwrap_or_default().as_str(),
                 "end_addr": json_hex(module.raw.base_of_image + module.raw.size_of_image as u64),
                 "filename": module.name,
                 "cert_subject": self.cert_info.get(&module.name),
diff --git a/minidump/src/minidump.rs b/minidump/src/minidump.rs
index 3ba88f6..6685ce5 100644
--- a/minidump/src/minidump.rs
+++ b/minidump/src/minidump.rs
@@ -810,7 +810,7 @@ impl MinidumpModule {
             self.raw.misc_record.data_size,
             self.raw.misc_record.rva,
             self.code_file(),
-            self.code_identifier(),
+            self.code_identifier().unwrap_or_default(),
         )?;
         // Print CodeView data.
         match self.codeview_info {
@@ -925,28 +925,46 @@ impl Module for MinidumpModule {
         Cow::Borrowed(&self.name)
     }
 
-    fn code_identifier(&self) -> CodeId {
+    fn code_identifier(&self) -> Option<CodeId> {
         match self.codeview_info {
-            Some(CodeView::Elf(ref raw)) => CodeId::from_binary(&raw.build_id),
-            Some(CodeView::Pdb70(ref raw)) if raw.age == 0 => {
-                // This is a macOS minidump. It looks like the age is typically
-                // >0 for Windows minidumps, and Breakpad sets it to 0 on macOS.
-                // We're relying on this observation in order to distinguish
-                // between these two platforms since macOS uses PDB70 instead of
-                // its own dedicated format.
-                // See the following linked comment in a Windows PDB writer:
-                // https://github.com/microsoft/microsoft-pdb/blob/082c5290e5aff028ae84e43affa8be717aa7af73/PDB/dbi/pdb.cpp#L602-L606
-                // TODO: Figure out a way to get access to `MinidumpSystemInfo`
-                // here instead of relying on `age`.
-                CodeId::new(format!("{:#}", raw.signature))
+            Some(CodeView::Pdb70(ref raw)) => {
+                if raw.age == 0 {
+                    // This is a macOS minidump. It looks like the age is
+                    // typically >0 for Windows minidumps, and Breakpad sets it
+                    // to 0 on macOS. We're relying on this observation in order
+                    // to distinguish between these two platforms since macOS
+                    // uses PDB70 instead of its own dedicated format.
+                    // See the following linked comment in a Windows PDB writer:
+                    // https://github.com/microsoft/microsoft-pdb/blob/082c5290e5aff028ae84e43affa8be717aa7af73/PDB/dbi/pdb.cpp#L602-L606
+                    // TODO: Figure out a way to get access to
+                    // `MinidumpSystemInfo` here instead of relying on `age`.
+                    Some(CodeId::new(format!("{:#}", raw.signature)))
+                } else {
+                    Some(CodeId::new(format!(
+                        "{0:08X}{1:x}",
+                        self.raw.time_date_stamp, self.raw.size_of_image
+                    )))
+                }
             }
-            _ => {
-                // TODO: Breakpad stubs this out on non-Windows.
-                CodeId::new(format!(
-                    "{0:08X}{1:x}",
-                    self.raw.time_date_stamp, self.raw.size_of_image
-                ))
+            Some(CodeView::Pdb20(_)) => Some(CodeId::new(format!(
+                "{0:08X}{1:x}",
+                self.raw.time_date_stamp, self.raw.size_of_image
+            ))),
+            Some(CodeView::Elf(ref raw)) => {
+                // Return None instead of sentinel CodeIds for empty
+                // `build_id`s. Non-executable mapped files like fonts or .jar
+                // files will usually fall under this case.
+                if raw.build_id.iter().all(|byte| *byte == 0) {
+                    None
+                } else {
+                    Some(CodeId::from_binary(&raw.build_id))
+                }
             }
+            // Occasionally things will make it into the module stream that
+            // shouldn't be there, and so no meaningful CodeId can be found from
+            // those. One of those things are SysV shared memory segments which
+            // have no CodeView record.
+            _ => None,
         }
     }
     fn debug_file(&self) -> Option<Cow<'_, str>> {
@@ -972,7 +990,7 @@ impl Module for MinidumpModule {
                     &raw.signature.data4,
                 )
                 .ok()?;
-                Some(DebugId::from_parts(uuid, raw.age))
+                (!uuid.is_nil()).then(|| DebugId::from_parts(uuid, raw.age))
             }
             Some(CodeView::Pdb20(ref raw)) => Some(DebugId::from_pdb20(raw.signature, raw.age)),
             Some(CodeView::Elf(ref raw)) => {
@@ -1072,7 +1090,7 @@ impl MinidumpUnloadedModule {
             format_time_t(self.raw.time_date_stamp),
             self.raw.module_name_rva,
             self.code_file(),
-            self.code_identifier(),
+            self.code_identifier().unwrap_or_default(),
         )?;
 
         Ok(())
@@ -1099,11 +1117,14 @@ impl Module for MinidumpUnloadedModule {
     fn code_file(&self) -> Cow<'_, str> {
         Cow::Borrowed(&self.name)
     }
-    fn code_identifier(&self) -> CodeId {
-        CodeId::new(format!(
+    fn code_identifier(&self) -> Option<CodeId> {
+        // TODO: This should be returning None if the unloaded module is coming
+        // from a non-Windows minidump. We'll need info about the operating
+        // system, ideally sourced from the SystemInfo to be able to do this.
+        Some(CodeId::new(format!(
             "{0:08X}{1:x}",
             self.raw.time_date_stamp, self.raw.size_of_image
-        ))
+        )))
     }
     fn debug_file(&self) -> Option<Cow<'_, str>> {
         None
@@ -5043,7 +5064,7 @@ mod test {
         assert_eq!(modules[0].code_file(), "single module");
         // time_date_stamp and size_of_image concatenated
         assert_eq!(
-            modules[0].code_identifier(),
+            modules[0].code_identifier().unwrap(),
             CodeId::new("B1054D2Aada542bd".to_string())
         );
         assert_eq!(modules[0].debug_file().unwrap(), "c:\\foo\\file.pdb");
@@ -5085,7 +5106,7 @@ mod test {
         assert_eq!(modules[0].code_file(), "single module");
         // time_date_stamp and size_of_image concatenated
         assert_eq!(
-            modules[0].code_identifier(),
+            modules[0].code_identifier().unwrap(),
             CodeId::new("B1054D2Aada542bd".to_string())
         );
         assert_eq!(modules[0].debug_file().unwrap(), "c:\\foo\\file.pdb");
@@ -5118,7 +5139,7 @@ mod test {
         assert_eq!(modules[0].code_file(), "single module");
         // time_date_stamp and size_of_image concatenated
         assert_eq!(
-            modules[0].code_identifier(),
+            modules[0].code_identifier().unwrap(),
             CodeId::new("B1054D2Aada542bd".to_string())
         );
     }
@@ -6102,7 +6123,7 @@ c70206ca83eb2852-de0206ca83eb2852  -w-s  10bac9000 fd:05 1196511 /usr/lib64/libt
         assert_eq!(modules[0].code_file(), "module 1");
         // The full build ID.
         assert_eq!(
-            modules[0].code_identifier(),
+            modules[0].code_identifier().unwrap(),
             CodeId::new("000102030405060708090a0b0c0d0e0f1011121314151617".to_string())
         );
         assert_eq!(modules[0].debug_file().unwrap(), "module 1");
@@ -6116,7 +6137,7 @@ c70206ca83eb2852-de0206ca83eb2852  -w-s  10bac9000 fd:05 1196511 /usr/lib64/libt
         assert_eq!(modules[1].code_file(), "module 2");
         // The full build ID.
         assert_eq!(
-            modules[1].code_identifier(),
+            modules[1].code_identifier().unwrap(),
             CodeId::new("0001020304050607".to_string())
         );
         assert_eq!(modules[1].debug_file().unwrap(), "module 2");
@@ -6163,7 +6184,7 @@ c70206ca83eb2852-de0206ca83eb2852  -w-s  10bac9000 fd:05 1196511 /usr/lib64/libt
         assert_eq!(modules.len(), 1);
         // should be the uuid stored in cv record
         assert_eq!(
-            modules[0].code_identifier(),
+            modules[0].code_identifier().unwrap(),
             CodeId::new("AABBCCDDEEFF00112233445566778899".to_owned())
         );
         // should match code identifier, but with the age appended to it
@@ -6193,15 +6214,45 @@ c70206ca83eb2852-de0206ca83eb2852  -w-s  10bac9000 fd:05 1196511 /usr/lib64/libt
             Some(&STOCK_VERSION_INFO),
         )
         .cv_record(&cv_record1);
+
+        // Add a module with a PDB70 build id of nothing but zeros
+        let name2 = DumpString::new("module 2", Endian::Little);
+        let cv_record2 = Section::with_endian(Endian::Little)
+            // signature
+            .D32(md::CvSignature::Pdb70 as u32)
+            // signature, a GUID
+            .D32(0x0)
+            .D16(0x0)
+            .D16(0x0)
+            .append_bytes(b"\0\0\0\0\0\0\0\0")
+            // age, breakpad writes 0
+            .D32(0)
+            // pdb_file_name
+            .append_bytes(b"\0");
+        let module2 = SynthModule::new(
+            Endian::Little,
+            0x100000000,
+            0x4000,
+            &name2,
+            0xb1054d2a,
+            0x34571371,
+            Some(&STOCK_VERSION_INFO),
+        )
+        .cv_record(&cv_record2);
         let dump = SynthMinidump::with_endian(Endian::Little)
             .add_module(module1)
+            .add_module(module2)
             .add(name1)
-            .add(cv_record1);
+            .add(cv_record1)
+            .add(name2)
+            .add(cv_record2);
+
         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());
+        assert!(modules[1].debug_identifier().is_none());
     }
 
     #[test]
@@ -6413,4 +6464,46 @@ c70206ca83eb2852-de0206ca83eb2852  -w-s  10bac9000 fd:05 1196511 /usr/lib64/libt
         let data = b"MDMP\x93\xa7\x00\x00\r\x00\x00\x00 \xff\xff\xff\xff\xff\xff\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00";
         assert!(Minidump::read(data.as_ref()).is_err());
     }
+
+    #[test]
+    fn test_empty_module() {
+        let name = DumpString::new("/SYSV00000000 (deleted)", Endian::Little);
+        let module = SynthModule::new(
+            Endian::Little,
+            0x7f602915e000,
+            0x26000,
+            &name,
+            0x0,
+            0x0,
+            // All of these are completely zeroed out in the wild.
+            Some(&md::VS_FIXEDFILEINFO {
+                signature: 0,
+                struct_version: 0,
+                file_version_hi: 0,
+                file_version_lo: 0,
+                product_version_hi: 0,
+                product_version_lo: 0,
+                file_flags_mask: 0,
+                file_flags: 0,
+                file_os: 0,
+                file_type: 0,
+                file_subtype: 0,
+                file_date_hi: 0,
+                file_date_lo: 0,
+            }),
+        );
+        let dump = SynthMinidump::with_endian(Endian::Little)
+            .add_module(module)
+            .add(name);
+        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].code_identifier(), None);
+        assert_eq!(modules[0].debug_identifier(), None);
+        assert_eq!(modules[0].code_file(), "/SYSV00000000 (deleted)");
+        assert_eq!(modules[0].debug_file(), None);
+        assert_eq!(modules[0].raw.base_of_image, 0x7f602915e000);
+        assert_eq!(modules[0].raw.size_of_image, 0x26000);
+    }
 }


Test patch:
diff --git a/minidump/tests/test_minidump.rs b/minidump/tests/test_minidump.rs
index 0ccbbea..4e742c7 100644
--- a/minidump/tests/test_minidump.rs
+++ b/minidump/tests/test_minidump.rs
@@ -64,7 +64,7 @@ fn test_module_list() {
     assert_eq!(modules[0].size(), 0x2d000);
     assert_eq!(modules[0].code_file(), "c:\\test_app.exe");
     assert_eq!(
-        modules[0].code_identifier(),
+        modules[0].code_identifier().unwrap(),
         CodeId::new("45D35F6C2d000".to_string())
     );
     assert_eq!(modules[0].debug_file().unwrap(), "c:\\test_app.pdb");
@@ -78,7 +78,7 @@ fn test_module_list() {
     assert_eq!(modules[12].size(), 0xb000);
     assert_eq!(modules[12].code_file(), "C:\\WINDOWS\\system32\\psapi.dll");
     assert_eq!(
-        modules[12].code_identifier(),
+        modules[12].code_identifier().unwrap(),
         CodeId::new("411096CAb000".to_string())
     );
     assert_eq!(modules[12].debug_file().unwrap(), "psapi.pdb");