rust-minidump · msirringhaus · Mar 16, 2022 · Mar 14, 2022
@@ -8,16 +8,16 @@ license = "MIT"
 [dependencies]
 byteorder = "1.3.2"
 cfg-if = "1.0"
-memoffset = "0.5.1"
+memoffset = "0.6"
 minidump-common = "0.10"
 scroll = "0.11"
 tempfile = "3.1.0"
 thiserror = "1.0.21"
 
 [target.'cfg(unix)'.dependencies]
 libc = "0.2.74"
-goblin = "0.1.2"
-memmap2 = "0.2.2"
+goblin = "0.5"
+memmap2 = "0.5"
 
 [target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies]
 nix = "0.23"

@@ -1,6 +1,6 @@
 //! Minidump defines register structures which are different from the raw
 //! structures which we get from the kernel. These are platform specific
-//! functions to juggle the ucontext_t and user structures into minidump format.
+//! functions to juggle the `ucontext_t` and user structures into minidump format.
 
 #![allow(non_camel_case_types)]
 

@@ -124,7 +124,7 @@ pub fn write_dso_debug_stream(
 
     // Assume the program base is at the beginning of the same page as the PHDR
     let mut base = phdr & !0xfff;
-    let mut dyn_addr = 0 as ElfAddr;
+    let mut dyn_addr = 0;
     // Search for the program PT_DYNAMIC segment
     for ph in program_headers {
         // Adjust base address with the virtual address of the PT_LOAD segment
@@ -217,7 +217,7 @@ pub fn write_dso_debug_stream(
     }
 
     let mut linkmap_rva = u32::MAX;
-    if dso_vec.len() > 0 {
+    if !dso_vec.is_empty() {
         // If we have at least one DSO, create an array of MDRawLinkMap
         // entries in the minidump file.
         let mut linkmap = MemoryArrayWriter::<MDRawLinkMap>::alloc_array(buffer, dso_vec.len())?;

@@ -58,9 +58,13 @@ pub fn write_cpu_information(sys_info: &mut MDRawSystemInfo) -> Result<()> {
             continue;
         }
 
-        let split: Vec<_> = line.split(':').map(|x| x.trim()).collect();
-        let field = split[0];
-        let value = split.get(1); // Option, might be missing
+        let mut liter = line.split(':').map(|x| x.trim());
+        let field = liter.next().unwrap(); // guaranteed to have at least one item
+        let value = if let Some(val) = liter.next() {
+            val
+        } else {
+            continue;
+        };
 
         let mut is_first_entry = true;
         for mut entry in cpu_info_table.iter_mut() {
@@ -70,21 +74,17 @@ pub fn write_cpu_information(sys_info: &mut MDRawSystemInfo) -> Result<()> {
             }
             is_first_entry = false;
             if field == entry.info_name {
-                if let Some(val) = value {
-                    if let Ok(v) = val.parse() {
-                        entry.value = v;
-                        entry.found = true;
-                    } else {
-                        continue;
-                    }
+                if let Ok(v) = value.parse() {
+                    entry.value = v;
+                    entry.found = true;
                 } else {
                     continue;
                 }
             }
 
             // special case for vendor_id
-            if field == vendor_id_name && value.is_some() && !value.unwrap().is_empty() {
-                vendor_id = value.unwrap().to_string();
+            if field == vendor_id_name && !value.is_empty() {
+                vendor_id = value.to_owned();
             }
         }
     }

@@ -182,7 +182,7 @@ impl MappingInfo {
     }
 
     pub fn get_mmap(name: &Option<String>, offset: usize) -> Result<Mmap> {
-        if !MappingInfo::is_mapped_file_safe_to_open(&name) {
+        if !MappingInfo::is_mapped_file_safe_to_open(name) {
             return Err(MapsReaderError::NotSafeToOpenMapping(
                 name.clone().unwrap_or_default(),
             ));
@@ -298,23 +298,21 @@ impl MappingInfo {
 
     pub fn get_mapping_effective_name_and_path(&self) -> Result<(String, String)> {
         let mut file_path = self.name.clone().unwrap_or_default();
-        let file_name;
 
         // Tools such as minidump_stackwalk use the name of the module to look up
         // symbols produced by dump_syms. dump_syms will prefer to use a module's
         // DT_SONAME as the module name, if one exists, and will fall back to the
         // filesystem name of the module.
 
         // Just use the filesystem name if no SONAME is present.
-        let file_name = match self.elf_file_so_name() {
-            Ok(name) => name,
-            Err(_) => {
-                //   file_path := /path/to/libname.so
-                //   file_name := libname.so
-                let split: Vec<_> = file_path.rsplitn(2, '/').collect();
-                file_name = split.first().unwrap().to_string();
-                return Ok((file_path, file_name));
-            }
+        let file_name = if let Ok(name) = self.elf_file_so_name() {
+            name
+        } else {
+            //   file_path := /path/to/libname.so
+            //   file_name := libname.so
+            // SAFETY: The unwrap is safe as rsplit always returns at least one item
+            let file_name = file_path.rsplit('/').next().unwrap().to_owned();
+            return Ok((file_path, file_name));
         };
 
         if self.executable && self.offset != 0 {
@@ -421,10 +419,11 @@ mod tests {
         let (lines, linux_gate_loc) = get_lines_and_loc();
         // Only /usr/bin/cat and [heap]
         for line in lines {
-            match MappingInfo::parse_from_line(&line, linux_gate_loc, mappings.last_mut()) {
-                Ok(MappingInfoParsingResult::Success(map)) => mappings.push(map),
-                Ok(MappingInfoParsingResult::SkipLine) => continue,
-                Err(_) => assert!(false),
+            match MappingInfo::parse_from_line(line, linux_gate_loc, mappings.last_mut())
+                .expect("failed to read mapping info")
+            {
+                MappingInfoParsingResult::Success(map) => mappings.push(map),
+                MappingInfoParsingResult::SkipLine => continue,
             }
         }
         assert_eq!(mappings.len(), 23);
@@ -437,10 +436,11 @@ mod tests {
         let (lines, linux_gate_loc) = get_lines_and_loc();
         // Only /usr/bin/cat and [heap]
         for line in lines[0..=6].iter() {
-            match MappingInfo::parse_from_line(&line, linux_gate_loc, mappings.last_mut()) {
-                Ok(MappingInfoParsingResult::Success(map)) => mappings.push(map),
-                Ok(MappingInfoParsingResult::SkipLine) => continue,
-                Err(_) => assert!(false),
+            match MappingInfo::parse_from_line(line, linux_gate_loc, mappings.last_mut())
+                .expect("failed to read mapping info")
+            {
+                MappingInfoParsingResult::Success(map) => mappings.push(map),
+                MappingInfoParsingResult::SkipLine => continue,
             }
         }
 
@@ -578,10 +578,11 @@ mod tests {
         let linux_gate_loc = 0x7ffe091bf000;
         let mut mappings: Vec<MappingInfo> = Vec::new();
         for line in lines {
-            match MappingInfo::parse_from_line(&line, linux_gate_loc, mappings.last_mut()) {
-                Ok(MappingInfoParsingResult::Success(map)) => mappings.push(map),
-                Ok(MappingInfoParsingResult::SkipLine) => continue,
-                Err(_) => assert!(false),
+            match MappingInfo::parse_from_line(line, linux_gate_loc, mappings.last_mut())
+                .expect("failed to read mapping info")
+            {
+                MappingInfoParsingResult::Success(map) => mappings.push(map),
+                MappingInfoParsingResult::SkipLine => continue,
             }
         }
         assert_eq!(mappings.len(), 1);
@@ -603,10 +604,11 @@ mod tests {
         let linux_gate_loc = 0x7ffe091bf000;
         let mut mappings: Vec<MappingInfo> = Vec::new();
         for line in lines {
-            match MappingInfo::parse_from_line(&line, linux_gate_loc, mappings.last_mut()) {
-                Ok(MappingInfoParsingResult::Success(map)) => mappings.push(map),
-                Ok(MappingInfoParsingResult::SkipLine) => continue,
-                Err(x) => panic!("{:?}", x),
+            match MappingInfo::parse_from_line(line, linux_gate_loc, mappings.last_mut())
+                .expect("failed to read mapping info")
+            {
+                MappingInfoParsingResult::Success(map) => mappings.push(map),
+                MappingInfoParsingResult::SkipLine => continue,
             }
         }
         assert_eq!(mappings.len(), 1);
@@ -637,10 +639,11 @@ mod tests {
         let linux_gate_loc = 0x7ffe091bf000;
         let mut mappings: Vec<MappingInfo> = Vec::new();
         for line in lines {
-            match MappingInfo::parse_from_line(&line, linux_gate_loc, mappings.last_mut()) {
-                Ok(MappingInfoParsingResult::Success(map)) => mappings.push(map),
-                Ok(MappingInfoParsingResult::SkipLine) => continue,
-                Err(_) => assert!(false),
+            match MappingInfo::parse_from_line(line, linux_gate_loc, mappings.last_mut())
+                .expect("failed to read mapping info")
+            {
+                MappingInfoParsingResult::Success(map) => mappings.push(map),
+                MappingInfoParsingResult::SkipLine => continue,
             }
         }
         assert_eq!(mappings.len(), 4);

@@ -299,15 +299,15 @@ impl MinidumpWriter {
         // we should have a mostly-intact dump
         dir_section.write_to_file(buffer, None)?;
 
-        let dirent = thread_list_stream::write(self, buffer, &dumper)?;
+        let dirent = thread_list_stream::write(self, buffer, dumper)?;
         // Write section to file
         dir_section.write_to_file(buffer, Some(dirent))?;
 
         let dirent = mappings::write(self, buffer, dumper)?;
         // Write section to file
         dir_section.write_to_file(buffer, Some(dirent))?;
 
-        let _ = app_memory::write(self, buffer)?;
+        app_memory::write(self, buffer)?;
         // Write section to file
         dir_section.write_to_file(buffer, None)?;
 
@@ -413,6 +413,7 @@ impl MinidumpWriter {
         Ok(())
     }
 
+    #[allow(clippy::unused_self)]
     fn write_file(
         &self,
         buffer: &mut DumpBuf,

@@ -73,6 +73,7 @@ impl PtraceDumper {
         Ok(())
     }
 
+    #[cfg_attr(not(target_os = "android"), allow(clippy::unused_self))]
     pub fn late_init(&mut self) -> Result<(), InitError> {
         #[cfg(target_os = "android")]
         {
@@ -216,7 +217,7 @@ impl PtraceDumper {
                         .ok();
                     (tid, name)
                 })
-                .for_each(|(tid, name)| self.threads.push(Thread { tid, name }))
+                .for_each(|(tid, name)| self.threads.push(Thread { tid, name }));
         }
         Ok(())
     }
@@ -274,8 +275,7 @@ impl PtraceDumper {
             let line = line.map_err(errmap)?;
             match MappingInfo::parse_from_line(&line, linux_gate_loc, self.mappings.last_mut()) {
                 Ok(MappingInfoParsingResult::Success(map)) => self.mappings.push(map),
-                Ok(MappingInfoParsingResult::SkipLine) => continue,
-                Err(_) => continue,
+                Ok(MappingInfoParsingResult::SkipLine) | Err(_) => continue,
             }
         }
 
@@ -354,11 +354,11 @@ impl PtraceDumper {
         let defaced;
         #[cfg(target_pointer_width = "64")]
         {
-            defaced = 0x0defaced0defacedusize.to_ne_bytes()
+            defaced = 0x0defaced0defacedusize.to_ne_bytes();
         }
         #[cfg(target_pointer_width = "32")]
         {
-            defaced = 0x0defacedusize.to_ne_bytes()
+            defaced = 0x0defacedusize.to_ne_bytes();
         };
         // the bitfield length is 2^test_bits long.
         let test_bits = 11;
@@ -448,26 +448,19 @@ impl PtraceDumper {
 
     // Find the mapping which the given memory address falls in.
     pub fn find_mapping(&self, address: usize) -> Option<&MappingInfo> {
-        for map in &self.mappings {
-            if address >= map.start_address && address - map.start_address < map.size {
-                return Some(&map);
-            }
-        }
-        None
+        self.mappings
+            .iter()
+            .find(|map| address >= map.start_address && address - map.start_address < map.size)
     }
 
     // Find the mapping which the given memory address falls in. Uses the
     // unadjusted mapping address range from the kernel, rather than the
     // biased range.
     pub fn find_mapping_no_bias(&self, address: usize) -> Option<&MappingInfo> {
-        for map in &self.mappings {
-            if address >= map.system_mapping_info.start_address
+        self.mappings.iter().find(|map| {
+            address >= map.system_mapping_info.start_address
                 && address < map.system_mapping_info.end_address
-            {
-                return Some(&map);
-            }
-        }
-        None
+        })
     }
 
     fn parse_build_id<'data>(
@@ -493,41 +486,42 @@ impl PtraceDumper {
 
     pub fn elf_file_identifier_from_mapped_file(mem_slice: &[u8]) -> Result<Vec<u8>, DumperError> {
         let elf_obj = elf::Elf::parse(mem_slice)?;
-        match Self::parse_build_id(&elf_obj, mem_slice) {
+
+        if let Some(build_id) = Self::parse_build_id(&elf_obj, mem_slice) {
             // Look for a build id note first.
-            Some(build_id) => Ok(build_id.to_vec()),
+            Ok(build_id.to_vec())
+        } else {
             // Fall back on hashing the first page of the text section.
-            None => {
-                // Attempt to locate the .text section of an ELF binary and generate
-                // a simple hash by XORing the first page worth of bytes into |result|.
-                for section in elf_obj.section_headers {
-                    if section.sh_type != elf::section_header::SHT_PROGBITS {
-                        continue;
-                    }
-                    if section.sh_flags & u64::from(elf::section_header::SHF_ALLOC) != 0 {
-                        if section.sh_flags & u64::from(elf::section_header::SHF_EXECINSTR) != 0 {
-                            let text_section = &mem_slice[section.sh_offset as usize..]
-                                [..section.sh_size as usize];
-                            // Only provide mem::size_of(MDGUID) bytes to keep identifiers produced by this
-                            // function backwards-compatible.
-                            let max_len = std::cmp::min(text_section.len(), 4096);
-                            let mut result = vec![0u8; std::mem::size_of::<GUID>()];
-                            let mut offset = 0;
-                            while offset < max_len {
-                                for idx in 0..std::mem::size_of::<GUID>() {
-                                    if offset + idx >= text_section.len() {
-                                        break;
-                                    }
-                                    result[idx] ^= text_section[offset + idx];
-                                }
-                                offset += std::mem::size_of::<GUID>();
+
+            // Attempt to locate the .text section of an ELF binary and generate
+            // a simple hash by XORing the first page worth of bytes into |result|.
+            for section in elf_obj.section_headers {
+                if section.sh_type != elf::section_header::SHT_PROGBITS {
+                    continue;
+                }
+                if section.sh_flags & u64::from(elf::section_header::SHF_ALLOC) != 0
+                    && section.sh_flags & u64::from(elf::section_header::SHF_EXECINSTR) != 0
+                {
+                    let text_section =
+                        &mem_slice[section.sh_offset as usize..][..section.sh_size as usize];
+                    // Only provide mem::size_of(MDGUID) bytes to keep identifiers produced by this
+                    // function backwards-compatible.
+                    let max_len = std::cmp::min(text_section.len(), 4096);
+                    let mut result = vec![0u8; std::mem::size_of::<GUID>()];
+                    let mut offset = 0;
+                    while offset < max_len {
+                        for idx in 0..std::mem::size_of::<GUID>() {
+                            if offset + idx >= text_section.len() {
+                                break;
                             }
-                            return Ok(result);
+                            result[idx] ^= text_section[offset + idx];
                         }
+                        offset += std::mem::size_of::<GUID>();
                     }
+                    return Ok(result);
                 }
-                Err(DumperError::NoBuildIDFound)
             }
+            Err(DumperError::NoBuildIDFound)
         }
     }
 
@@ -564,7 +558,7 @@ impl PtraceDumper {
             }
         }
         let new_name = MappingInfo::handle_deleted_file_in_mapping(
-            &mapping.name.as_ref().unwrap_or(&String::new()),
+            mapping.name.as_deref().unwrap_or_default(),
             pid,
         )?;