From 0d8292781c531c94725205480a0c39338c34c7c2 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 14 Mar 2022 21:48:41 +0100 Subject: [PATCH] Bump dependencies, fix all clippy lints --- Cargo.toml | 6 +- src/linux/crash_context.rs | 2 +- src/linux/dso_debug.rs | 4 +- src/linux/dumper_cpu_info/x86_mips.rs | 24 +++---- src/linux/maps_reader.rs | 65 +++++++++-------- src/linux/minidump_writer.rs | 5 +- src/linux/ptrace_dumper.rs | 88 +++++++++++------------ src/linux/sections/exception_stream.rs | 4 +- src/linux/sections/mappings.rs | 11 ++- src/linux/sections/thread_list_stream.rs | 2 +- src/linux/sections/thread_names_stream.rs | 2 +- src/linux/thread_info.rs | 12 ++-- src/linux/thread_info/x86.rs | 6 +- tests/common/mod.rs | 2 +- tests/minidump_writer.rs | 5 +- 15 files changed, 116 insertions(+), 122 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 8fe64dfb..e92da703 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,7 +8,7 @@ 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" @@ -16,8 +16,8 @@ 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" diff --git a/src/linux/crash_context.rs b/src/linux/crash_context.rs index 97fe1e76..4802ad87 100644 --- a/src/linux/crash_context.rs +++ b/src/linux/crash_context.rs @@ -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)] diff --git a/src/linux/dso_debug.rs b/src/linux/dso_debug.rs index da00b68b..eb3cc396 100644 --- a/src/linux/dso_debug.rs +++ b/src/linux/dso_debug.rs @@ -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::::alloc_array(buffer, dso_vec.len())?; diff --git a/src/linux/dumper_cpu_info/x86_mips.rs b/src/linux/dumper_cpu_info/x86_mips.rs index cd36dbc6..131df34f 100644 --- a/src/linux/dumper_cpu_info/x86_mips.rs +++ b/src/linux/dumper_cpu_info/x86_mips.rs @@ -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(); } } } diff --git a/src/linux/maps_reader.rs b/src/linux/maps_reader.rs index c8435f96..8078b163 100644 --- a/src/linux/maps_reader.rs +++ b/src/linux/maps_reader.rs @@ -182,7 +182,7 @@ impl MappingInfo { } pub fn get_mmap(name: &Option, offset: usize) -> Result { - 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,7 +298,6 @@ 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 @@ -306,15 +305,14 @@ impl MappingInfo { // 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 = 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 = 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 = 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); diff --git a/src/linux/minidump_writer.rs b/src/linux/minidump_writer.rs index c3249607..b67792ea 100644 --- a/src/linux/minidump_writer.rs +++ b/src/linux/minidump_writer.rs @@ -299,7 +299,7 @@ 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))?; @@ -307,7 +307,7 @@ impl MinidumpWriter { // 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, diff --git a/src/linux/ptrace_dumper.rs b/src/linux/ptrace_dumper.rs index 987e94de..50e9748d 100644 --- a/src/linux/ptrace_dumper.rs +++ b/src/linux/ptrace_dumper.rs @@ -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, 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::()]; - let mut offset = 0; - while offset < max_len { - for idx in 0..std::mem::size_of::() { - if offset + idx >= text_section.len() { - break; - } - result[idx] ^= text_section[offset + idx]; - } - offset += std::mem::size_of::(); + + // 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::()]; + let mut offset = 0; + while offset < max_len { + for idx in 0..std::mem::size_of::() { + if offset + idx >= text_section.len() { + break; } - return Ok(result); + result[idx] ^= text_section[offset + idx]; } + offset += std::mem::size_of::(); } + 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, )?; diff --git a/src/linux/sections/exception_stream.rs b/src/linux/sections/exception_stream.rs index 4de13888..8d2a7fd3 100644 --- a/src/linux/sections/exception_stream.rs +++ b/src/linux/sections/exception_stream.rs @@ -94,8 +94,8 @@ pub fn write( }; let thread_context = match config.crashing_thread_context { - CrashingThreadContext::CrashContextPlusAddress((ctx, _)) => ctx, - CrashingThreadContext::CrashContext(ctx) => ctx, + CrashingThreadContext::CrashContextPlusAddress((ctx, _)) + | CrashingThreadContext::CrashContext(ctx) => ctx, CrashingThreadContext::None => MDLocationDescriptor { data_size: 0, rva: 0, diff --git a/src/linux/sections/mappings.rs b/src/linux/sections/mappings.rs index e2e96a9a..7eb002d0 100644 --- a/src/linux/sections/mappings.rs +++ b/src/linux/sections/mappings.rs @@ -41,7 +41,7 @@ pub fn write( for user in &config.user_mapping_list { // GUID was provided by caller. let module = fill_raw_module(buffer, &user.mapping, &user.identifier)?; - modules.push(module) + modules.push(module); } let list_header = MemoryWriter::::alloc_with_val(buffer, modules.len() as u32)?; @@ -64,10 +64,9 @@ fn fill_raw_module( mapping: &MappingInfo, identifier: &[u8], ) -> Result { - let cv_record: MDLocationDescriptor; - if identifier.is_empty() { + let cv_record = if identifier.is_empty() { // Just zeroes - cv_record = Default::default(); + Default::default() } else { let cv_signature = crate::minidump_format::format::CvSignature::Elf as u32; let array_size = std::mem::size_of_val(&cv_signature) + identifier.len(); @@ -81,8 +80,8 @@ fn fill_raw_module( { sig_section.set_value_at(buffer, *val, index)?; } - cv_record = sig_section.location(); - } + sig_section.location() + }; let (file_path, _) = mapping .get_mapping_effective_name_and_path() diff --git a/src/linux/sections/thread_list_stream.rs b/src/linux/sections/thread_list_stream.rs index ae333eed..e4607784 100644 --- a/src/linux/sections/thread_list_stream.rs +++ b/src/linux/sections/thread_list_stream.rs @@ -232,7 +232,7 @@ fn fill_thread_stack( buffer.write_all(&stack_bytes); thread.stack.start_of_memory_range = stack as u64; thread.stack.memory = stack_location; - config.memory_blocks.push(thread.stack.clone()); + config.memory_blocks.push(thread.stack); } Ok(()) } diff --git a/src/linux/sections/thread_names_stream.rs b/src/linux/sections/thread_names_stream.rs index cf060d5f..bd8682b2 100644 --- a/src/linux/sections/thread_names_stream.rs +++ b/src/linux/sections/thread_names_stream.rs @@ -21,7 +21,7 @@ pub fn write( for (idx, item) in dumper.threads.iter().enumerate() { if let Some(name) = &item.name { - let pos = write_string_to_location(buffer, &name)?; + let pos = write_string_to_location(buffer, name)?; let thread = MDRawThreadName { thread_id: item.tid.try_into()?, thread_name_rva: pos.rva.into(), diff --git a/src/linux/thread_info.rs b/src/linux/thread_info.rs index e4fc4af1..86b0c44c 100644 --- a/src/linux/thread_info.rs +++ b/src/linux/thread_info.rs @@ -69,13 +69,13 @@ trait CommonThreadInfo { tgid = l .get(6..) .ok_or_else(|| ThreadInfoError::InvalidProcStatusFile(tid, l.clone()))? - .parse::()? + .parse::()?; } "PPid:\t" => { ppid = l .get(6..) .ok_or_else(|| ThreadInfoError::InvalidProcStatusFile(tid, l.clone()))? - .parse::()? + .parse::()?; } _ => continue, } @@ -106,7 +106,7 @@ trait CommonThreadInfo { request as ptrace::RequestType, libc::pid_t::from(pid), flag.unwrap_or(NT_Elf::NT_NONE), - data.as_mut_ptr() as *const _ as *const libc::c_void, + data.as_mut_ptr(), ) }; Errno::result(res)?; @@ -123,9 +123,9 @@ trait CommonThreadInfo { flag: Option, pid: nix::unistd::Pid, ) -> Result { - let mut data = std::mem::MaybeUninit::uninit(); + let mut data = std::mem::MaybeUninit::::uninit(); let io = libc::iovec { - iov_base: data.as_mut_ptr() as *mut libc::c_void, + iov_base: data.as_mut_ptr().cast(), iov_len: std::mem::size_of::(), }; let res = unsafe { @@ -133,7 +133,7 @@ trait CommonThreadInfo { request as ptrace::RequestType, libc::pid_t::from(pid), flag.unwrap_or(NT_Elf::NT_NONE), - &io as *const _ as *const libc::c_void, + &io as *const _, ) }; Errno::result(res)?; diff --git a/src/linux/thread_info/x86.rs b/src/linux/thread_info/x86.rs index 5e08bf52..82652700 100644 --- a/src/linux/thread_info/x86.rs +++ b/src/linux/thread_info/x86.rs @@ -94,18 +94,18 @@ impl ThreadInfoX86 { let debug_offset = memoffset::offset_of!(user, u_debugreg); let elem_offset = size_of_val(&dregs[0]); - for idx in 0..NUM_DEBUG_REGISTERS { + for (idx, dreg) in dregs.iter_mut().enumerate() { let chunk = Self::peek_user( tid, (debug_offset + idx * elem_offset) as ptrace::AddressType, )?; #[cfg(target_arch = "x86_64")] { - dregs[idx] = chunk as u64; // libc / ptrace is very messy wrt int types used... + *dreg = chunk as u64; // libc / ptrace is very messy wrt int types used... } #[cfg(target_arch = "x86")] { - dregs[idx] = chunk as i32; // libc / ptrace is very messy wrt int types used... + *dreg = chunk as i32; // libc / ptrace is very messy wrt int types used... } } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 3775bcc3..bb262601 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -86,7 +86,7 @@ pub fn start_child_and_return(command: &str) -> Child { .arg("--bin") .arg("test") .arg("--") - .arg(format!("{}", command)) + .arg(command) .stdout(Stdio::piped()) .spawn() .expect("failed to execute child"); diff --git a/tests/minidump_writer.rs b/tests/minidump_writer.rs index ad8d2bea..2daf998b 100644 --- a/tests/minidump_writer.rs +++ b/tests/minidump_writer.rs @@ -594,10 +594,7 @@ fn test_sanitized_stacks_helper(context: Context) { let start = mem.rva as usize; let end = (mem.rva + mem.data_size) as usize; let slice = &dump_array.as_slice()[start..end]; - assert!(slice - .windows(defaced.len()) - .position(|window| window == defaced) - .is_some()); + assert!(slice.windows(defaced.len()).any(|window| window == defaced)); } } #[test]