From 6bc6d73f93ba4b82ed4cecc9243c40296bce0e80 Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Fri, 11 Oct 2024 16:11:16 -0400 Subject: [PATCH 1/6] WIP: Add soft errors to minidump Fixes #31 --- Cargo.lock | 23 ++- Cargo.toml | 2 +- src/bin/test.rs | 54 ++++-- src/error_list.rs | 101 ++++++++++ src/lib.rs | 2 + src/linux/auxv/mod.rs | 24 ++- src/linux/errors.rs | 56 ++++-- src/linux/minidump_writer.rs | 125 ++++++++++--- src/linux/ptrace_dumper.rs | 234 +++++++++++++++++------- src/linux/sections/systeminfo_stream.rs | 14 +- tests/linux_minidump_writer.rs | 2 - tests/ptrace_dumper.rs | 24 ++- 12 files changed, 511 insertions(+), 150 deletions(-) create mode 100644 src/error_list.rs diff --git a/Cargo.lock b/Cargo.lock index f58e7f26..db2cc26a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -181,7 +181,7 @@ dependencies = [ "circular", "debugid", "futures-util", - "minidump-common", + "minidump-common 0.22.0 (registry+https://github.com/rust-lang/crates.io-index)", "nom", "range-map", "thiserror", @@ -1191,7 +1191,7 @@ dependencies = [ "debugid", "encoding_rs", "memmap2", - "minidump-common", + "minidump-common 0.22.0 (registry+https://github.com/rust-lang/crates.io-index)", "num-traits", "procfs-core", "range-map", @@ -1202,6 +1202,19 @@ dependencies = [ "uuid", ] +[[package]] +name = "minidump-common" +version = "0.22.0" +dependencies = [ + "bitflags 2.6.0", + "debugid", + "num-derive", + "num-traits", + "range-map", + "scroll 0.12.0", + "smart-default", +] + [[package]] name = "minidump-common" version = "0.22.0" @@ -1228,7 +1241,7 @@ dependencies = [ "debugid", "futures-util", "minidump", - "minidump-common", + "minidump-common 0.22.0 (registry+https://github.com/rust-lang/crates.io-index)", "minidump-unwind", "scroll 0.12.0", "serde", @@ -1250,7 +1263,7 @@ dependencies = [ "futures-util", "memmap2", "minidump", - "minidump-common", + "minidump-common 0.22.0 (registry+https://github.com/rust-lang/crates.io-index)", "object", "scroll 0.12.0", "tracing", @@ -1275,7 +1288,7 @@ dependencies = [ "memmap2", "memoffset", "minidump", - "minidump-common", + "minidump-common 0.22.0", "minidump-processor", "minidump-unwind", "nix", diff --git a/Cargo.toml b/Cargo.toml index f78be2a7..949f2f73 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,7 +15,7 @@ cfg-if = "1.0" crash-context = "0.6" log = "0.4" memoffset = "0.9" -minidump-common = "0.22" +minidump-common = { path = "../rust-minidump/minidump-common" } scroll = "0.12" tempfile = "3.8" thiserror = "1.0" diff --git a/src/bin/test.rs b/src/bin/test.rs index 6713852c..64c96649 100644 --- a/src/bin/test.rs +++ b/src/bin/test.rs @@ -26,13 +26,15 @@ mod linux { fn test_setup() -> Result<()> { let ppid = getppid(); - PtraceDumper::new(ppid.as_raw(), STOP_TIMEOUT, Default::default())?; + let (_dumper, _soft_errors) = + PtraceDumper::new_report_soft_errors(ppid.as_raw(), STOP_TIMEOUT, Default::default())?; Ok(()) } fn test_thread_list() -> Result<()> { let ppid = getppid(); - let dumper = PtraceDumper::new(ppid.as_raw(), STOP_TIMEOUT, Default::default())?; + let (dumper, _soft_errors) = + PtraceDumper::new_report_soft_errors(ppid.as_raw(), STOP_TIMEOUT, Default::default())?; test!(!dumper.threads.is_empty(), "No threads"); test!( dumper @@ -59,8 +61,12 @@ mod linux { use minidump_writer::mem_reader::MemReader; let ppid = getppid().as_raw(); - let mut dumper = PtraceDumper::new(ppid, STOP_TIMEOUT, Default::default())?; - dumper.suspend_threads()?; + let (mut dumper, _soft_errors) = + PtraceDumper::new_report_soft_errors(ppid, STOP_TIMEOUT, Default::default())?; + + if let Some(soft_errors) = dumper.suspend_threads().some() { + return Err(soft_errors.into()); + } // We support 3 different methods of reading memory from another // process, ensure they all function and give the same results @@ -113,13 +119,17 @@ mod linux { test!(heap_res == expected_heap, "heap var not correct"); - dumper.resume_threads()?; + if let Some(soft_errors) = dumper.resume_threads().some() { + return Err(soft_errors.into()); + } + Ok(()) } fn test_find_mappings(addr1: usize, addr2: usize) -> Result<()> { let ppid = getppid(); - let dumper = PtraceDumper::new(ppid.as_raw(), STOP_TIMEOUT, Default::default())?; + let (dumper, _soft_errors) = + PtraceDumper::new_report_soft_errors(ppid.as_raw(), STOP_TIMEOUT, Default::default())?; dumper .find_mapping(addr1) .ok_or("No mapping for addr1 found")?; @@ -136,8 +146,12 @@ mod linux { let ppid = getppid().as_raw(); let exe_link = format!("/proc/{ppid}/exe"); let exe_name = std::fs::read_link(exe_link)?.into_os_string(); - let mut dumper = PtraceDumper::new(ppid, STOP_TIMEOUT, Default::default())?; - dumper.suspend_threads()?; + let (mut dumper, _soft_errors) = + PtraceDumper::new_report_soft_errors(ppid, STOP_TIMEOUT, Default::default())?; + if let Some(soft_errors) = dumper.suspend_threads().some() { + return Err(soft_errors.into()); + } + let mut found_exe = None; for (idx, mapping) in dumper.mappings.iter().enumerate() { if mapping.name.as_ref().map(|x| x.into()).as_ref() == Some(&exe_name) { @@ -147,7 +161,9 @@ mod linux { } let idx = found_exe.unwrap(); let module_reader::BuildId(id) = dumper.from_process_memory_for_index(idx)?; - dumper.resume_threads()?; + if let Some(soft_errors) = dumper.resume_threads().some() { + return Err(soft_errors.into()); + } assert!(!id.is_empty()); assert!(id.iter().any(|&x| x > 0)); Ok(()) @@ -155,7 +171,11 @@ mod linux { fn test_merged_mappings(path: String, mapped_mem: usize, mem_size: usize) -> Result<()> { // Now check that PtraceDumper interpreted the mappings properly. - let dumper = PtraceDumper::new(getppid().as_raw(), STOP_TIMEOUT, Default::default())?; + let (dumper, _soft_errors) = PtraceDumper::new_report_soft_errors( + getppid().as_raw(), + STOP_TIMEOUT, + Default::default(), + )?; let mut mapping_count = 0; for map in &dumper.mappings { if map @@ -177,17 +197,22 @@ mod linux { fn test_linux_gate_mapping_id() -> Result<()> { let ppid = getppid().as_raw(); - let mut dumper = PtraceDumper::new(ppid, STOP_TIMEOUT, Default::default())?; + let (mut dumper, _soft_errors) = + PtraceDumper::new_report_soft_errors(ppid, STOP_TIMEOUT, Default::default())?; let mut found_linux_gate = false; for mapping in dumper.mappings.clone() { if mapping.name == Some(LINUX_GATE_LIBRARY_NAME.into()) { found_linux_gate = true; - dumper.suspend_threads()?; + if let Some(soft_errors) = dumper.suspend_threads().some() { + return Err(soft_errors.into()); + } let module_reader::BuildId(id) = PtraceDumper::from_process_memory_for_mapping(&mapping, ppid)?; test!(!id.is_empty(), "id-vec is empty"); test!(id.iter().any(|&x| x > 0), "all id elements are 0"); - dumper.resume_threads()?; + if let Some(soft_errors) = dumper.resume_threads().some() { + return Err(soft_errors.into()); + } break; } } @@ -197,7 +222,8 @@ mod linux { fn test_mappings_include_linux_gate() -> Result<()> { let ppid = getppid().as_raw(); - let dumper = PtraceDumper::new(ppid, STOP_TIMEOUT, Default::default())?; + let (dumper, _soft_errors) = + PtraceDumper::new_report_soft_errors(ppid, STOP_TIMEOUT, Default::default())?; let linux_gate_loc = dumper.auxv.get_linux_gate_address().unwrap(); test!(linux_gate_loc != 0, "linux_gate_loc == 0"); let mut found_linux_gate = false; diff --git a/src/error_list.rs b/src/error_list.rs new file mode 100644 index 00000000..d10f2426 --- /dev/null +++ b/src/error_list.rs @@ -0,0 +1,101 @@ +//! Handling of "soft errors" while generating the minidump + +/// Encapsulates a list of "soft error"s +/// +/// A "soft error" is an error that is encounted while generating the minidump that doesn't +/// totally prevent the minidump from being useful, but it may have missing or invalid +/// information. +/// +/// It should be returned by a function when the function was able to at-least partially achieve +/// its goals, and when further use of functions in the same API is permissible and can still be +/// at-least partially functional. +/// +/// Admittedly, this concept makes layers of abstraction a bit more difficult, as something that +/// is considered "soft" by one layer may be a deal-breaker for the layer above it, or visa-versa +/// -- an error that a lower layer considers a total failure might just be a nuissance for the layer +/// above it. +/// +/// An example of the former might be the act of suspending all the threads -- The `PTraceDumper`` +/// API will actually work just fine even if none of the threads are suspended, so it only returns +/// a soft error; however, the dumper itself considers it to be a critical failure if not even one +/// thread could be stopped. +/// +/// An example of the latter might trying to stop the process -- Being unable to send SIGSTOP to +/// the process would be considered a critical failure by `stop_process()`, but merely an +/// inconvenience by the code that's calling it. +#[must_use] +pub struct SoftErrorList { + errors: Vec, +} + +impl SoftErrorList { + /// Returns `Some(Self)` if the list contains at least one soft error + pub fn some(self) -> Option { + if !self.is_empty() { + Some(self) + } else { + None + } + } + /// Returns `true` if the list is empty + pub fn is_empty(&self) -> bool { + self.errors.is_empty() + } + /// Add a soft error to the list + pub fn push(&mut self, error: E) { + self.errors.push(error); + } +} + +impl Default for SoftErrorList { + fn default() -> Self { + Self { errors: Vec::new() } + } +} + +impl SoftErrorList { + // Helper function for the Debug and Display traits + fn fmt(&self, f: &mut std::fmt::Formatter<'_>, write_sources: bool) -> std::fmt::Result { + writeln!(f, "one or more soft errors occurred:")?; + writeln!(f)?; + for (i, e) in self.errors.iter().enumerate() { + writeln!(f, " {i}:")?; + + for line in e.to_string().lines() { + writeln!(f, " {line}")?; + } + + writeln!(f)?; + + if write_sources { + let mut source = e.source(); + while let Some(e) = source { + writeln!(f, " caused by:")?; + + for line in e.to_string().lines() { + writeln!(f, " {line}")?; + } + + writeln!(f)?; + + source = e.source(); + } + } + } + Ok(()) + } +} + +impl std::fmt::Debug for SoftErrorList { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.fmt(f, true) + } +} + +impl std::fmt::Display for SoftErrorList { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.fmt(f, false) + } +} + +impl std::error::Error for SoftErrorList {} diff --git a/src/lib.rs b/src/lib.rs index a76291d0..d397d25c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -19,3 +19,5 @@ pub mod minidump_format; pub mod dir_section; pub mod mem_writer; + +mod error_list; diff --git a/src/linux/auxv/mod.rs b/src/linux/auxv/mod.rs index 403ab114..8834f7fb 100644 --- a/src/linux/auxv/mod.rs +++ b/src/linux/auxv/mod.rs @@ -1,4 +1,6 @@ +use crate::error_list::SoftErrorList; pub use reader::ProcfsAuxvIter; + use { crate::Pid, std::{fs::File, io::BufReader}, @@ -79,17 +81,27 @@ pub struct AuxvDumpInfo { } impl AuxvDumpInfo { - pub fn try_filling_missing_info(&mut self, pid: Pid) -> Result<(), AuxvError> { + pub fn try_filling_missing_info( + &mut self, + pid: Pid, + ) -> Result, AuxvError> { if self.is_complete() { - return Ok(()); + return Ok(SoftErrorList::default()); } let auxv_path = format!("/proc/{pid}/auxv"); let auxv_file = File::open(&auxv_path).map_err(|e| AuxvError::OpenError(auxv_path, e))?; - for AuxvPair { key, value } in - ProcfsAuxvIter::new(BufReader::new(auxv_file)).filter_map(Result::ok) - { + let mut soft_errors = SoftErrorList::default(); + + for pair_result in ProcfsAuxvIter::new(BufReader::new(auxv_file)) { + let AuxvPair { key, value } = match pair_result { + Ok(pair) => pair, + Err(e) => { + soft_errors.push(e); + continue; + } + }; let dest_field = match key { consts::AT_PHNUM => &mut self.program_header_count, consts::AT_PHDR => &mut self.program_header_address, @@ -102,7 +114,7 @@ impl AuxvDumpInfo { } } - Ok(()) + Ok(soft_errors) } pub fn get_program_header_count(&self) -> Option { self.program_header_count diff --git a/src/linux/errors.rs b/src/linux/errors.rs index 982445a0..9166cf1c 100644 --- a/src/linux/errors.rs +++ b/src/linux/errors.rs @@ -1,26 +1,12 @@ use crate::{ - dir_section::FileWriterError, maps_reader::MappingInfo, mem_writer::MemoryWriterError, Pid, + dir_section::FileWriterError, error_list::SoftErrorList, maps_reader::MappingInfo, + mem_writer::MemoryWriterError, Pid, }; use goblin; -use nix::errno::Errno; use std::ffi::OsString; use thiserror::Error; -#[derive(Debug, Error)] -pub enum InitError { - #[error("failed to read auxv")] - ReadAuxvFailed(crate::auxv::AuxvError), - #[error("IO error for file {0}")] - IOError(String, #[source] std::io::Error), - #[error("crash thread does not reference principal mapping")] - PrincipalMappingNotReferenced, - #[error("Failed Android specific late init")] - AndroidLateInitError(#[from] AndroidError), - #[error("Failed to read the page size")] - PageSizeError(#[from] Errno), - #[error("Ptrace does not function within the same process")] - CannotPtraceSameProcess, -} +use super::ptrace_dumper::InitError; #[derive(Error, Debug)] pub enum MapsReaderError { @@ -110,8 +96,6 @@ pub enum DumperError { CopyFromProcessError(#[from] CopyFromProcessError), #[error("Skipped thread {0} due to it being part of the seccomp sandbox's trusted code")] DetachSkippedThread(Pid), - #[error("No threads left to suspend out of {0}")] - SuspendNoThreadsLeft(usize), #[error("No mapping for stack pointer found")] NoStackPointerMapping, #[error("Failed slice conversion")] @@ -180,6 +164,8 @@ pub enum SectionSystemInfoError { MemoryWriterError(#[from] MemoryWriterError), #[error("Failed to get CPU Info")] CpuInfoError(#[from] CpuInfoError), + #[error("Failed trying to write CPU information")] + WriteCpuInformationFailed(#[source] CpuInfoError), } #[derive(Debug, Error)] @@ -250,6 +236,38 @@ pub enum WriterError { FileWriterError(#[from] FileWriterError), #[error("Failed to get current timestamp when writing header of minidump")] SystemTimeError(#[from] std::time::SystemTimeError), + #[error("Errors occurred while initializing PTraceDumper")] + InitErrors(#[source] SoftErrorList), + #[error("Errors occurred while suspending threads")] + SuspendThreadsErrors(#[source] SoftErrorList), + #[error("Crash thread does not reference principal mapping")] + PrincipalMappingNotReferenced, + #[error("Errors occurred while writing system info")] + WriteSystemInfoErrors(#[source] SoftErrorList), + #[error("Failed writing cpuinfo")] + WriteCpuInfoFailed(#[source] MemoryWriterError), + #[error("Failed writing thread proc status")] + WriteThreadProcStatusFailed(#[source] MemoryWriterError), + #[error("Failed writing OS Release Information")] + WriteOsReleaseInfoFailed(#[source] MemoryWriterError), + #[error("Failed writing process command line")] + WriteCommandLineFailed(#[source] MemoryWriterError), + #[error("Writing process environment failed")] + WriteEnvironmentFailed(#[source] MemoryWriterError), + #[error("Failed to write auxv file")] + WriteAuxvFailed(#[source] MemoryWriterError), + #[error("Failed to write maps file")] + WriteMapsFailed(#[source] MemoryWriterError), + #[error("Failed writing DSO Debug Stream")] + WriteDSODebugStreamFailed(#[source] SectionDsoDebugError), + #[error("Failed writing limits file")] + WriteLimitsFailed(#[source] MemoryWriterError), + #[error("Failed writing handle data stream")] + WriteHandleDataStreamFailed(#[source] SectionHandleDataStreamError), + #[error("Failed writing handle data stream direction entry")] + WriteHandleDataStreamDirentFailed(#[source] FileWriterError), + #[error("No threads left to suspend out of {0}")] + SuspendNoThreadsLeft(usize), } #[derive(Debug, Error)] diff --git a/src/linux/minidump_writer.rs b/src/linux/minidump_writer.rs index ba6b82ec..72d6800c 100644 --- a/src/linux/minidump_writer.rs +++ b/src/linux/minidump_writer.rs @@ -2,11 +2,12 @@ pub use crate::linux::auxv::{AuxvType, DirectAuxvDumpInfo}; use crate::{ auxv::AuxvDumpInfo, dir_section::{DirSection, DumpBuf}, + error_list::SoftErrorList, linux::{ app_memory::AppMemoryList, crash_context::CrashContext, dso_debug, - errors::{InitError, WriterError}, + errors::WriterError, maps_reader::{MappingInfo, MappingList}, ptrace_dumper::PtraceDumper, sections::*, @@ -144,8 +145,27 @@ impl MinidumpWriter { .clone() .map(AuxvDumpInfo::from) .unwrap_or_default(); - let mut dumper = PtraceDumper::new(self.process_id, self.stop_timeout, auxv)?; - dumper.suspend_threads()?; + + let mut soft_errors = SoftErrorList::default(); + + let (mut dumper, init_soft_errors) = + PtraceDumper::new_report_soft_errors(self.process_id, self.stop_timeout, auxv)?; + + if !init_soft_errors.is_empty() { + soft_errors.push(WriterError::InitErrors(init_soft_errors)); + } + + let threads_count = dumper.threads.len(); + + if let Some(suspend_soft_errors) = dumper.suspend_threads().some() { + if dumper.threads.is_empty() { + // TBH I'm not sure this even needs to be a hard error. Is a minidump without any + // thread info still at-least a little useful? + return Err(WriterError::SuspendNoThreadsLeft(threads_count)); + } + soft_errors.push(WriterError::SuspendThreadsErrors(suspend_soft_errors)); + } + dumper.late_init()?; if self.skip_stacks_if_mapping_unreferenced { @@ -154,16 +174,15 @@ impl MinidumpWriter { } if !self.crash_thread_references_principal_mapping(&dumper) { - return Err(InitError::PrincipalMappingNotReferenced.into()); + soft_errors.push(WriterError::PrincipalMappingNotReferenced); } } let mut buffer = Buffer::with_capacity(0); - self.generate_dump(&mut buffer, &mut dumper, destination)?; + self.generate_dump(&mut buffer, &mut dumper, soft_errors, destination)?; - // dumper would resume threads in drop() automatically, - // but in case there is an error, we want to catch it - dumper.resume_threads()?; + // TODO - Record these errors? Or maybe we don't care? + let _resume_soft_errors = dumper.resume_threads(); Ok(buffer.into()) } @@ -226,11 +245,12 @@ impl MinidumpWriter { &mut self, buffer: &mut DumpBuf, dumper: &mut PtraceDumper, + mut soft_errors: SoftErrorList, destination: &mut (impl Write + Seek), ) -> Result<()> { // A minidump file contains a number of tagged streams. This is the number // of streams which we write. - let num_writers = 17u32; + let num_writers = 19u32; let mut header_section = MemoryWriter::::alloc(buffer)?; @@ -270,9 +290,13 @@ impl MinidumpWriter { let dirent = exception_stream::write(self, buffer)?; dir_section.write_to_file(buffer, Some(dirent))?; - let dirent = systeminfo_stream::write(buffer)?; + let (dirent, systeminfo_soft_errors) = systeminfo_stream::write(buffer)?; dir_section.write_to_file(buffer, Some(dirent))?; + if !systeminfo_soft_errors.is_empty() { + soft_errors.push(WriterError::WriteSystemInfoErrors(systeminfo_soft_errors)); + } + let dirent = memory_info_list_stream::write(self, buffer)?; dir_section.write_to_file(buffer, Some(dirent))?; @@ -281,7 +305,10 @@ impl MinidumpWriter { stream_type: MDStreamType::LinuxCpuInfo as u32, location, }, - Err(_) => Default::default(), + Err(e) => { + soft_errors.push(WriterError::WriteCpuInfoFailed(e)); + Default::default() + } }; dir_section.write_to_file(buffer, Some(dirent))?; @@ -291,7 +318,10 @@ impl MinidumpWriter { stream_type: MDStreamType::LinuxProcStatus as u32, location, }, - Err(_) => Default::default(), + Err(e) => { + soft_errors.push(WriterError::WriteThreadProcStatusFailed(e)); + Default::default() + } }; dir_section.write_to_file(buffer, Some(dirent))?; @@ -303,7 +333,10 @@ impl MinidumpWriter { stream_type: MDStreamType::LinuxLsbRelease as u32, location, }, - Err(_) => Default::default(), + Err(e) => { + soft_errors.push(WriterError::WriteOsReleaseInfoFailed(e)); + Default::default() + } }; dir_section.write_to_file(buffer, Some(dirent))?; @@ -313,7 +346,10 @@ impl MinidumpWriter { stream_type: MDStreamType::LinuxCmdLine as u32, location, }, - Err(_) => Default::default(), + Err(e) => { + soft_errors.push(WriterError::WriteCommandLineFailed(e)); + Default::default() + } }; dir_section.write_to_file(buffer, Some(dirent))?; @@ -323,7 +359,10 @@ impl MinidumpWriter { stream_type: MDStreamType::LinuxEnviron as u32, location, }, - Err(_) => Default::default(), + Err(e) => { + soft_errors.push(WriterError::WriteEnvironmentFailed(e)); + Default::default() + } }; dir_section.write_to_file(buffer, Some(dirent))?; @@ -332,7 +371,10 @@ impl MinidumpWriter { stream_type: MDStreamType::LinuxAuxv as u32, location, }, - Err(_) => Default::default(), + Err(e) => { + soft_errors.push(WriterError::WriteAuxvFailed(e)); + Default::default() + } }; dir_section.write_to_file(buffer, Some(dirent))?; @@ -341,12 +383,21 @@ impl MinidumpWriter { stream_type: MDStreamType::LinuxMaps as u32, location, }, - Err(_) => Default::default(), + Err(e) => { + soft_errors.push(WriterError::WriteMapsFailed(e)); + Default::default() + } }; dir_section.write_to_file(buffer, Some(dirent))?; - let dirent = dso_debug::write_dso_debug_stream(buffer, self.process_id, &dumper.auxv) - .unwrap_or_default(); + let dirent = match dso_debug::write_dso_debug_stream(buffer, self.process_id, &dumper.auxv) + { + Ok(dirent) => dirent, + Err(e) => { + soft_errors.push(WriterError::WriteDSODebugStreamFailed(e)); + Default::default() + } + }; dir_section.write_to_file(buffer, Some(dirent))?; let dirent = match self.write_file(buffer, &format!("/proc/{}/limits", self.blamed_thread)) @@ -355,17 +406,33 @@ impl MinidumpWriter { stream_type: MDStreamType::MozLinuxLimits as u32, location, }, - Err(_) => Default::default(), + Err(e) => { + soft_errors.push(WriterError::WriteLimitsFailed(e)); + Default::default() + } }; dir_section.write_to_file(buffer, Some(dirent))?; let dirent = thread_names_stream::write(buffer, dumper)?; dir_section.write_to_file(buffer, Some(dirent))?; - // This section is optional, so we ignore errors when writing it - if let Ok(dirent) = handle_data_stream::write(self, buffer) { - let _ = dir_section.write_to_file(buffer, Some(dirent)); - } + let dirent = match handle_data_stream::write(self, buffer) { + Ok(dirent) => dirent, + Err(e) => { + soft_errors.push(WriterError::WriteHandleDataStreamFailed(e)); + Default::default() + } + }; + dir_section.write_to_file(buffer, Some(dirent))?; + + // If this fails, there's really nothing we can do about that (other than ignore it). + let dirent = write_soft_errors(buffer, soft_errors) + .map(|location| MDRawDirectory { + stream_type: MDStreamType::MozSoftErrors as u32, + location, + }) + .unwrap_or_default(); + dir_section.write_to_file(buffer, Some(dirent))?; // If you add more directory entries, don't forget to update num_writers, above. Ok(()) @@ -383,3 +450,13 @@ impl MinidumpWriter { Ok(section.location()) } } + +fn write_soft_errors( + buffer: &mut DumpBuf, + soft_errors: SoftErrorList, +) -> Result { + let soft_error_list_str = format!("{soft_errors:?}"); + + let section = MemoryArrayWriter::write_bytes(buffer, soft_error_list_str.as_bytes()); + Ok(section.location()) +} diff --git a/src/linux/ptrace_dumper.rs b/src/linux/ptrace_dumper.rs index 66f36564..28379063 100644 --- a/src/linux/ptrace_dumper.rs +++ b/src/linux/ptrace_dumper.rs @@ -1,15 +1,18 @@ #[cfg(target_os = "android")] use crate::linux::android::late_process_mappings; -use crate::linux::{ - auxv::AuxvDumpInfo, - errors::{DumperError, InitError, ThreadInfoError}, - maps_reader::MappingInfo, - module_reader, - thread_info::ThreadInfo, - Pid, -}; #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] use crate::thread_info; +use crate::{ + error_list::SoftErrorList, + linux::{ + auxv::AuxvDumpInfo, + errors::{DumperError, ThreadInfoError}, + maps_reader::MappingInfo, + module_reader, + thread_info::ThreadInfo, + Pid, + }, +}; use nix::{ errno::Errno, sys::{ptrace, signal, wait}, @@ -19,11 +22,17 @@ use procfs_core::{ FromRead, ProcError, }; use std::{ + ffi::OsString, path, result::Result, time::{Duration, Instant}, }; +use super::{ + auxv::AuxvError, + errors::{AndroidError, MapsReaderError}, +}; + #[derive(Debug, Clone)] pub struct Thread { pub tid: Pid, @@ -55,7 +64,45 @@ impl Drop for PtraceDumper { } #[derive(Debug, thiserror::Error)] -enum StopProcessError { +pub enum InitError { + #[error("failed to read auxv")] + ReadAuxvFailed(#[source] crate::auxv::AuxvError), + #[error("IO error for file {0}")] + IOError(String, #[source] std::io::Error), + #[error("Failed Android specific late init")] + AndroidLateInitError(#[from] AndroidError), + #[error("Failed to read the page size")] + PageSizeError(#[from] Errno), + #[error("Ptrace does not function within the same process")] + CannotPtraceSameProcess, + #[error("Failed to stop the target process")] + StopProcessFailed(#[source] StopProcessError), + #[error("Errors occurred while filling missing Auxv info")] + FillMissingAuxvInfoErrors(#[source] SoftErrorList), + #[error("Failed filling missing Auxv info")] + FillMissingAuxvInfoFailed(#[source] AuxvError), + #[error("Failed reading proc/pid/task entry for process")] + ReadProcessThreadEntryFailed(#[source] std::io::Error), + #[error("Process task entry `{0:?}` could not be parsed as a TID")] + ProcessTaskEntryNotTid(OsString), + #[error("Failed to read thread name")] + ReadThreadNameFailed(#[source] std::io::Error), + #[error("Proc task directory `{0:?}` is not a directory")] + ProcPidTaskNotDirectory(String), + #[error("Errors while enumerating threads")] + EnumerateThreadsErrors(#[source] SoftErrorList), + #[error("Failed to enumerate threads")] + EnumerateThreadsFailed(#[source] Box), + #[error("Failed to read process map file")] + ReadProcessMapFileFailed(#[source] ProcError), + #[error("Failed to aggregate process mappings")] + AggregateMappingsFailed(#[source] MapsReaderError), + #[error("Failed to enumerate process mappings")] + EnumerateMappingsFailed(#[source] Box), +} + +#[derive(Debug, thiserror::Error)] +pub enum StopProcessError { #[error("Failed to stop the process")] Stop(#[from] Errno), #[error("Failed to get the process state")] @@ -65,7 +112,7 @@ enum StopProcessError { } #[derive(Debug, thiserror::Error)] -enum ContinueProcessError { +pub enum ContinueProcessError { #[error("Failed to continue the process")] Continue(#[from] Errno), } @@ -88,7 +135,11 @@ fn ptrace_detach(child: Pid) -> Result<(), DumperError> { impl PtraceDumper { /// Constructs a dumper for extracting information from the specified process id - pub fn new(pid: Pid, stop_timeout: Duration, auxv: AuxvDumpInfo) -> Result { + pub fn new_report_soft_errors( + pid: Pid, + stop_timeout: Duration, + auxv: AuxvDumpInfo, + ) -> Result<(Self, SoftErrorList), InitError> { if pid == std::process::id() as _ { return Err(InitError::CannotPtraceSameProcess); } @@ -101,28 +152,56 @@ impl PtraceDumper { mappings: Vec::new(), page_size: 0, }; - dumper.init(stop_timeout)?; - Ok(dumper) + let soft_errors = dumper.init(stop_timeout)?; + Ok((dumper, soft_errors)) } // TODO: late_init for chromeos and android - pub fn init(&mut self, stop_timeout: Duration) -> Result<(), InitError> { + pub fn init(&mut self, stop_timeout: Duration) -> Result, InitError> { + let mut soft_errors = SoftErrorList::default(); + // Stopping the process is best-effort. if let Err(e) = self.stop_process(stop_timeout) { - log::warn!("failed to stop process {}: {e}", self.pid); + soft_errors.push(InitError::StopProcessFailed(e)); } - if let Err(e) = self.auxv.try_filling_missing_info(self.pid) { - log::warn!("failed trying to fill in missing auxv info: {e}"); + // Even if we completely fail to fill in any additional Auxv info, we can still press + // forward. + match self.auxv.try_filling_missing_info(self.pid) { + Ok(auxv_soft_errors) if !auxv_soft_errors.is_empty() => { + soft_errors.push(InitError::FillMissingAuxvInfoErrors(auxv_soft_errors)); + } + Err(e) => { + soft_errors.push(InitError::FillMissingAuxvInfoFailed(e)); + } + _ => (), + } + + // If we completely fail to enumerate any threads... Some information is still better than + // no information! + match self.enumerate_threads() { + Ok(enumerate_soft_errors) if !enumerate_soft_errors.is_empty() => { + soft_errors.push(InitError::EnumerateThreadsErrors(enumerate_soft_errors)); + } + Err(e) => { + soft_errors.push(InitError::EnumerateThreadsFailed(Box::new(e))); + } + _ => (), + } + + // Same with mappings -- Some information is still better than no information! + match self.enumerate_mappings() { + Ok(()) => (), + Err(e) => { + soft_errors.push(InitError::EnumerateMappingsFailed(Box::new(e))); + } } - self.enumerate_threads()?; - self.enumerate_mappings()?; self.page_size = nix::unistd::sysconf(nix::unistd::SysconfVar::PAGE_SIZE)? .expect("page size apparently unlimited: doesn't make sense.") as usize; - Ok(()) + Ok(soft_errors) } #[cfg_attr(not(target_os = "android"), allow(clippy::unused_self))] @@ -207,36 +286,39 @@ impl PtraceDumper { ptrace_detach(child) } - pub fn suspend_threads(&mut self) -> Result<(), DumperError> { - let threads_count = self.threads.len(); + pub fn suspend_threads(&mut self) -> SoftErrorList { + let mut soft_errors = SoftErrorList::default(); + // Iterate over all threads and try to suspend them. // If the thread either disappeared before we could attach to it, or if // it was part of the seccomp sandbox's trusted code, it is OK to // silently drop it from the minidump. - self.threads.retain(|x| Self::suspend_thread(x.tid).is_ok()); + self.threads.retain(|x| match Self::suspend_thread(x.tid) { + Ok(()) => true, + Err(e) => { + soft_errors.push(e); + false + } + }); - if self.threads.is_empty() { - Err(DumperError::SuspendNoThreadsLeft(threads_count)) - } else { - self.threads_suspended = true; - Ok(()) - } + self.threads_suspended = true; + soft_errors } - pub fn resume_threads(&mut self) -> Result<(), DumperError> { - let mut result = Ok(()); + pub fn resume_threads(&mut self) -> SoftErrorList { + let mut soft_errors = SoftErrorList::default(); if self.threads_suspended { for thread in &self.threads { match Self::resume_thread(thread.tid) { - Ok(_) => {} - x => { - result = x; + Ok(()) => (), + Err(e) => { + soft_errors.push(e); } } } } self.threads_suspended = false; - result + soft_errors } /// Send SIGSTOP to the process so that we can get a consistent state. @@ -273,31 +355,46 @@ impl PtraceDumper { /// Parse /proc/$pid/task to list all the threads of the process identified by /// pid. - fn enumerate_threads(&mut self) -> Result<(), InitError> { + fn enumerate_threads(&mut self) -> Result, InitError> { + let mut soft_errors = SoftErrorList::default(); + let pid = self.pid; let filename = format!("/proc/{}/task", pid); let task_path = path::PathBuf::from(&filename); - if task_path.is_dir() { - std::fs::read_dir(task_path) - .map_err(|e| InitError::IOError(filename, e))? - .filter_map(|entry| entry.ok()) // Filter out bad entries - .filter_map(|entry| { - entry - .file_name() // Parse name to Pid, filter out those that are unparsable - .to_str() - .and_then(|name| name.parse::().ok()) - }) - .map(|tid| { - // Read the thread-name (if there is any) - let name = std::fs::read_to_string(format!("/proc/{}/task/{}/comm", pid, tid)) - // NOTE: This is a bit wasteful as it does two allocations in order to trim, but leaving it for now - .map(|s| s.trim_end().to_string()) - .ok(); - (tid, name) - }) - .for_each(|(tid, name)| self.threads.push(Thread { tid, name })); + if !task_path.is_dir() { + return Err(InitError::ProcPidTaskNotDirectory(filename)); } - Ok(()) + + for entry in std::fs::read_dir(task_path).map_err(|e| InitError::IOError(filename, e))? { + let entry = match entry { + Ok(entry) => entry, + Err(e) => { + soft_errors.push(InitError::ReadProcessThreadEntryFailed(e)); + continue; + } + }; + let file_name = entry.file_name(); + let tid = match file_name.to_str().and_then(|name| name.parse::().ok()) { + Some(tid) => tid, + None => { + soft_errors.push(InitError::ProcessTaskEntryNotTid(file_name)); + continue; + } + }; + + // Read the thread-name (if there is any) + let name = match std::fs::read_to_string(format!("/proc/{}/task/{}/comm", pid, tid)) { + Ok(name) => Some(name.trim_end().to_string()), + Err(e) => { + soft_errors.push(InitError::ReadThreadNameFailed(e)); + None + } + }; + + self.threads.push(Thread { tid, name }); + } + + Ok(soft_errors) } fn enumerate_mappings(&mut self) -> Result<(), InitError> { @@ -309,22 +406,21 @@ impl PtraceDumper { // See http://www.trilithium.com/johan/2005/08/linux-gate/ for more // information. let linux_gate_loc = self.auxv.get_linux_gate_address().unwrap_or_default(); - // Although the initial executable is usually the first mapping, it's not - // guaranteed (see http://crosbug.com/25355); therefore, try to use the - // actual entry point to find the mapping. - let entry_point_loc = self.auxv.get_entry_address().unwrap_or_default(); - let filename = format!("/proc/{}/maps", self.pid); - let errmap = |e| InitError::IOError(filename.clone(), e); - let maps_path = path::PathBuf::from(&filename); - let maps_file = std::fs::File::open(maps_path).map_err(errmap)?; + let maps_path = format!("/proc/{}/maps", self.pid); + let maps_file = + std::fs::File::open(&maps_path).map_err(|e| InitError::IOError(maps_path, e))?; use procfs_core::FromRead; - self.mappings = procfs_core::process::MemoryMaps::from_read(maps_file) - .ok() - .and_then(|maps| MappingInfo::aggregate(maps, linux_gate_loc).ok()) - .unwrap_or_default(); + let maps = procfs_core::process::MemoryMaps::from_read(maps_file) + .map_err(InitError::ReadProcessMapFileFailed)?; + + self.mappings = MappingInfo::aggregate(maps, linux_gate_loc) + .map_err(InitError::AggregateMappingsFailed)?; - if entry_point_loc != 0 { + // Although the initial executable is usually the first mapping, it's not + // guaranteed (see http://crosbug.com/25355); therefore, try to use the + // actual entry point to find the mapping. + if let Some(entry_point_loc) = self.auxv.get_entry_address() { let mut swap_idx = None; for (idx, module) in self.mappings.iter().enumerate() { // If this module contains the entry-point, and it's not already the first diff --git a/src/linux/sections/systeminfo_stream.rs b/src/linux/sections/systeminfo_stream.rs index a298c00d..5e48b170 100644 --- a/src/linux/sections/systeminfo_stream.rs +++ b/src/linux/sections/systeminfo_stream.rs @@ -1,7 +1,10 @@ use super::*; -use crate::linux::dumper_cpu_info as dci; +use crate::{error_list::SoftErrorList, linux::dumper_cpu_info as dci}; +use errors::SectionSystemInfoError; -pub fn write(buffer: &mut DumpBuf) -> Result { +pub fn write( + buffer: &mut DumpBuf, +) -> Result<(MDRawDirectory, SoftErrorList), SectionSystemInfoError> { let mut info_section = MemoryWriter::::alloc(buffer)?; let dirent = MDRawDirectory { stream_type: MDStreamType::SystemInfoStream as u32, @@ -16,8 +19,11 @@ pub fn write(buffer: &mut DumpBuf) -> Result()], defaced); - dumper.resume_threads().expect("Failed to resume threads"); + assert!( + dumper.resume_threads().is_empty(), + "Failed to resume threads" + ); child.kill().expect("Failed to kill process"); // Reap child From 26d4afc6f14a5feed6f88e2accc18037f198c2df Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Tue, 19 Nov 2024 16:36:03 -0500 Subject: [PATCH 2/6] Address review feedback, improve ergonomics, add testing --- .cargo/config.toml | 3 + Cargo.lock | 31 +- Cargo.toml | 10 +- src/bin/test.rs | 88 ++++-- src/dir_section.rs | 9 +- src/error_list.rs | 357 ++++++++++++++++++---- src/fail_enabled/active.rs | 154 ++++++++++ src/fail_enabled/inactive.rs | 8 + src/fail_enabled/mod.rs | 36 +++ src/lib.rs | 4 + src/linux/auxv/mod.rs | 31 +- src/linux/dumper_cpu_info/x86_mips.rs | 5 + src/linux/errors.rs | 219 ++++++++++--- src/linux/maps_reader.rs | 10 +- src/linux/minidump_writer.rs | 45 +-- src/linux/ptrace_dumper.rs | 137 +++++---- src/linux/sections/systeminfo_stream.rs | 8 +- src/mem_writer.rs | 31 +- tests/linux_minidump_writer_soft_error.rs | 119 ++++++++ tests/ptrace_dumper.rs | 38 +-- 20 files changed, 1077 insertions(+), 266 deletions(-) create mode 100644 src/fail_enabled/active.rs create mode 100644 src/fail_enabled/inactive.rs create mode 100644 src/fail_enabled/mod.rs create mode 100644 tests/linux_minidump_writer_soft_error.rs diff --git a/.cargo/config.toml b/.cargo/config.toml index e32d5cfd..28a9d026 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -14,3 +14,6 @@ linker = "x86_64-linux-android30-clang" # By default the linker _doesn't_ generate a build-id, however we want one for our tests. rustflags = ["-C", "link-args=-Wl,--build-id=sha1"] runner = [".cargo/android-runner.sh"] + +[patch.crates-io] +minidump-common = { path = "../rust-minidump/minidump-common" } \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index db2cc26a..f5f51aef 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -160,6 +160,9 @@ name = "bitflags" version = "2.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b048fb63fd8b5923fc5aa7b340d8e156aec7ec02f0c78fa8a6ddc2613f6f71de" +dependencies = [ + "serde", +] [[package]] name = "block-buffer" @@ -181,7 +184,7 @@ dependencies = [ "circular", "debugid", "futures-util", - "minidump-common 0.22.0 (registry+https://github.com/rust-lang/crates.io-index)", + "minidump-common", "nom", "range-map", "thiserror", @@ -1191,7 +1194,7 @@ dependencies = [ "debugid", "encoding_rs", "memmap2", - "minidump-common 0.22.0 (registry+https://github.com/rust-lang/crates.io-index)", + "minidump-common", "num-traits", "procfs-core", "range-map", @@ -1215,21 +1218,6 @@ dependencies = [ "smart-default", ] -[[package]] -name = "minidump-common" -version = "0.22.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "95a2b640f80e5514f49509ff1f97fb24693f95ef5be5ed810d70df4283a68acc" -dependencies = [ - "bitflags 2.6.0", - "debugid", - "num-derive", - "num-traits", - "range-map", - "scroll 0.12.0", - "smart-default", -] - [[package]] name = "minidump-processor" version = "0.22.0" @@ -1241,7 +1229,7 @@ dependencies = [ "debugid", "futures-util", "minidump", - "minidump-common 0.22.0 (registry+https://github.com/rust-lang/crates.io-index)", + "minidump-common", "minidump-unwind", "scroll 0.12.0", "serde", @@ -1263,7 +1251,7 @@ dependencies = [ "futures-util", "memmap2", "minidump", - "minidump-common 0.22.0 (registry+https://github.com/rust-lang/crates.io-index)", + "minidump-common", "object", "scroll 0.12.0", "tracing", @@ -1288,12 +1276,14 @@ dependencies = [ "memmap2", "memoffset", "minidump", - "minidump-common 0.22.0", + "minidump-common", "minidump-processor", "minidump-unwind", "nix", "procfs-core", "scroll 0.12.0", + "serde", + "serde_json", "similar-asserts", "tempfile", "thiserror", @@ -1619,6 +1609,7 @@ checksum = "2d3554923a69f4ce04c4a754260c338f505ce22642d3830e049a399fc2059a29" dependencies = [ "bitflags 2.6.0", "hex", + "serde", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 949f2f73..fdeb6d59 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,6 +8,10 @@ homepage = "https://github.com/rust-minidump/minidump-writer" edition = "2021" license = "MIT" +[features] +default = ["fail-enabled"] +fail-enabled = [] + [dependencies] bitflags = "2.4" byteorder = "1.4" @@ -15,8 +19,10 @@ cfg-if = "1.0" crash-context = "0.6" log = "0.4" memoffset = "0.9" -minidump-common = { path = "../rust-minidump/minidump-common" } +minidump-common = "0.22" scroll = "0.12" +serde = { version = "1.0.208", features = ["derive"] } +serde_json = "1.0.116" tempfile = "3.8" thiserror = "1.0" @@ -36,7 +42,7 @@ nix = { version = "0.29", default-features = false, features = [ ] } # Used for parsing procfs info. # default-features is disabled since it pulls in chrono -procfs-core = { version = "0.16", default-features = false } +procfs-core = { version = "0.16", default-features = false, features = ["serde1"] } [target.'cfg(target_os = "windows")'.dependencies] bitflags = "2.4" diff --git a/src/bin/test.rs b/src/bin/test.rs index 64c96649..d287dc1e 100644 --- a/src/bin/test.rs +++ b/src/bin/test.rs @@ -8,7 +8,7 @@ pub type Result = std::result::Result; mod linux { use super::*; use minidump_writer::{ - minidump_writer::STOP_TIMEOUT, module_reader, ptrace_dumper::PtraceDumper, + minidump_writer::STOP_TIMEOUT, module_reader, ptrace_dumper::PtraceDumper, SoftErrorList, LINUX_GATE_LIBRARY_NAME, }; use nix::{ @@ -26,15 +26,23 @@ mod linux { fn test_setup() -> Result<()> { let ppid = getppid(); - let (_dumper, _soft_errors) = - PtraceDumper::new_report_soft_errors(ppid.as_raw(), STOP_TIMEOUT, Default::default())?; + let _dumper = PtraceDumper::new_report_soft_errors( + ppid.as_raw(), + STOP_TIMEOUT, + Default::default(), + SoftErrorList::null_sublist(), + )?; Ok(()) } fn test_thread_list() -> Result<()> { let ppid = getppid(); - let (dumper, _soft_errors) = - PtraceDumper::new_report_soft_errors(ppid.as_raw(), STOP_TIMEOUT, Default::default())?; + let dumper = PtraceDumper::new_report_soft_errors( + ppid.as_raw(), + STOP_TIMEOUT, + Default::default(), + SoftErrorList::null_sublist(), + )?; test!(!dumper.threads.is_empty(), "No threads"); test!( dumper @@ -61,10 +69,16 @@ mod linux { use minidump_writer::mem_reader::MemReader; let ppid = getppid().as_raw(); - let (mut dumper, _soft_errors) = - PtraceDumper::new_report_soft_errors(ppid, STOP_TIMEOUT, Default::default())?; + let mut dumper = PtraceDumper::new_report_soft_errors( + ppid, + STOP_TIMEOUT, + Default::default(), + SoftErrorList::null_sublist(), + )?; - if let Some(soft_errors) = dumper.suspend_threads().some() { + let mut soft_errors = SoftErrorList::default(); + dumper.suspend_threads(soft_errors.inserted_sublist()); + if !soft_errors.is_empty() { return Err(soft_errors.into()); } @@ -119,7 +133,9 @@ mod linux { test!(heap_res == expected_heap, "heap var not correct"); - if let Some(soft_errors) = dumper.resume_threads().some() { + let mut soft_errors = SoftErrorList::default(); + dumper.resume_threads(soft_errors.inserted_sublist()); + if !soft_errors.is_empty() { return Err(soft_errors.into()); } @@ -128,8 +144,12 @@ mod linux { fn test_find_mappings(addr1: usize, addr2: usize) -> Result<()> { let ppid = getppid(); - let (dumper, _soft_errors) = - PtraceDumper::new_report_soft_errors(ppid.as_raw(), STOP_TIMEOUT, Default::default())?; + let dumper = PtraceDumper::new_report_soft_errors( + ppid.as_raw(), + STOP_TIMEOUT, + Default::default(), + SoftErrorList::null_sublist(), + )?; dumper .find_mapping(addr1) .ok_or("No mapping for addr1 found")?; @@ -146,9 +166,15 @@ mod linux { let ppid = getppid().as_raw(); let exe_link = format!("/proc/{ppid}/exe"); let exe_name = std::fs::read_link(exe_link)?.into_os_string(); - let (mut dumper, _soft_errors) = - PtraceDumper::new_report_soft_errors(ppid, STOP_TIMEOUT, Default::default())?; - if let Some(soft_errors) = dumper.suspend_threads().some() { + let mut dumper = PtraceDumper::new_report_soft_errors( + ppid, + STOP_TIMEOUT, + Default::default(), + SoftErrorList::null_sublist(), + )?; + let mut soft_errors = SoftErrorList::default(); + dumper.suspend_threads(soft_errors.inserted_sublist()); + if !soft_errors.is_empty() { return Err(soft_errors.into()); } @@ -161,9 +187,12 @@ mod linux { } let idx = found_exe.unwrap(); let module_reader::BuildId(id) = dumper.from_process_memory_for_index(idx)?; - if let Some(soft_errors) = dumper.resume_threads().some() { + let mut soft_errors = SoftErrorList::default(); + dumper.resume_threads(soft_errors.inserted_sublist()); + if !soft_errors.is_empty() { return Err(soft_errors.into()); } + assert!(!id.is_empty()); assert!(id.iter().any(|&x| x > 0)); Ok(()) @@ -171,10 +200,11 @@ mod linux { fn test_merged_mappings(path: String, mapped_mem: usize, mem_size: usize) -> Result<()> { // Now check that PtraceDumper interpreted the mappings properly. - let (dumper, _soft_errors) = PtraceDumper::new_report_soft_errors( + let dumper = PtraceDumper::new_report_soft_errors( getppid().as_raw(), STOP_TIMEOUT, Default::default(), + SoftErrorList::null_sublist(), )?; let mut mapping_count = 0; for map in &dumper.mappings { @@ -197,20 +227,28 @@ mod linux { fn test_linux_gate_mapping_id() -> Result<()> { let ppid = getppid().as_raw(); - let (mut dumper, _soft_errors) = - PtraceDumper::new_report_soft_errors(ppid, STOP_TIMEOUT, Default::default())?; + let mut dumper = PtraceDumper::new_report_soft_errors( + ppid, + STOP_TIMEOUT, + Default::default(), + SoftErrorList::null_sublist(), + )?; let mut found_linux_gate = false; for mapping in dumper.mappings.clone() { if mapping.name == Some(LINUX_GATE_LIBRARY_NAME.into()) { found_linux_gate = true; - if let Some(soft_errors) = dumper.suspend_threads().some() { + let mut soft_errors = SoftErrorList::default(); + dumper.suspend_threads(soft_errors.inserted_sublist()); + if !soft_errors.is_empty() { return Err(soft_errors.into()); } let module_reader::BuildId(id) = PtraceDumper::from_process_memory_for_mapping(&mapping, ppid)?; test!(!id.is_empty(), "id-vec is empty"); test!(id.iter().any(|&x| x > 0), "all id elements are 0"); - if let Some(soft_errors) = dumper.resume_threads().some() { + let mut soft_errors = SoftErrorList::default(); + dumper.resume_threads(soft_errors.inserted_sublist()); + if !soft_errors.is_empty() { return Err(soft_errors.into()); } break; @@ -222,8 +260,12 @@ mod linux { fn test_mappings_include_linux_gate() -> Result<()> { let ppid = getppid().as_raw(); - let (dumper, _soft_errors) = - PtraceDumper::new_report_soft_errors(ppid, STOP_TIMEOUT, Default::default())?; + let dumper = PtraceDumper::new_report_soft_errors( + ppid, + STOP_TIMEOUT, + Default::default(), + SoftErrorList::null_sublist(), + )?; let linux_gate_loc = dumper.auxv.get_linux_gate_address().unwrap(); test!(linux_gate_loc != 0, "linux_gate_loc == 0"); let mut found_linux_gate = false; @@ -231,7 +273,7 @@ mod linux { if mapping.name == Some(LINUX_GATE_LIBRARY_NAME.into()) { found_linux_gate = true; test!( - linux_gate_loc == mapping.start_address.try_into()?, + usize::try_from(linux_gate_loc)? == mapping.start_address, "linux_gate_loc != start_address" ); diff --git a/src/dir_section.rs b/src/dir_section.rs index ced5a2d1..4515081d 100644 --- a/src/dir_section.rs +++ b/src/dir_section.rs @@ -1,4 +1,5 @@ use crate::{ + error_list::serializers::*, mem_writer::{Buffer, MemoryArrayWriter, MemoryWriterError}, minidump_format::MDRawDirectory, }; @@ -6,10 +7,14 @@ use std::io::{Error, Seek, Write}; pub type DumpBuf = Buffer; -#[derive(Debug, thiserror::Error)] +#[derive(Debug, thiserror::Error, serde::Serialize)] pub enum FileWriterError { #[error("IO error")] - IOError(#[from] Error), + IOError( + #[from] + #[serde(serialize_with = "serialize_io_error")] + Error, + ), #[error("Failed to write to memory")] MemoryWriterError(#[from] MemoryWriterError), } diff --git a/src/error_list.rs b/src/error_list.rs index d10f2426..417cf7e4 100644 --- a/src/error_list.rs +++ b/src/error_list.rs @@ -1,50 +1,107 @@ -//! Handling of "soft errors" while generating the minidump +//! Encapsulates a list of "soft error"s +//! +//! A "soft error" is an error that is encounted while generating the minidump that doesn't +//! totally prevent the minidump from being useful, but it may have missing or invalid +//! information. +//! +//! It should be returned by a function when the function was able to at-least partially achieve +//! its goals, and when further use of functions in the same API is permissible and can still be +//! at-least partially functional. +//! +//! Admittedly, this concept makes layers of abstraction a bit more difficult, as something that +//! is considered "soft" by one layer may be a deal-breaker for the layer above it, or visa-versa +//! -- an error that a lower layer considers a total failure might just be a nuissance for the layer +//! above it. +//! +//! An example of the former might be the act of suspending all the threads -- The `PTraceDumper`` +//! API will actually work just fine even if none of the threads are suspended, so it only returns +//! a soft error; however, the dumper itself considers it to be a critical failure if not even one +//! thread could be stopped. +//! +//! An example of the latter might trying to stop the process -- Being unable to send SIGSTOP to +//! the process would be considered a critical failure by `stop_process()`, but merely an +//! inconvenience by the code that's calling it. -/// Encapsulates a list of "soft error"s -/// -/// A "soft error" is an error that is encounted while generating the minidump that doesn't -/// totally prevent the minidump from being useful, but it may have missing or invalid -/// information. -/// -/// It should be returned by a function when the function was able to at-least partially achieve -/// its goals, and when further use of functions in the same API is permissible and can still be -/// at-least partially functional. -/// -/// Admittedly, this concept makes layers of abstraction a bit more difficult, as something that -/// is considered "soft" by one layer may be a deal-breaker for the layer above it, or visa-versa -/// -- an error that a lower layer considers a total failure might just be a nuissance for the layer -/// above it. -/// -/// An example of the former might be the act of suspending all the threads -- The `PTraceDumper`` -/// API will actually work just fine even if none of the threads are suspended, so it only returns -/// a soft error; however, the dumper itself considers it to be a critical failure if not even one -/// thread could be stopped. -/// -/// An example of the latter might trying to stop the process -- Being unable to send SIGSTOP to -/// the process would be considered a critical failure by `stop_process()`, but merely an -/// inconvenience by the code that's calling it. -#[must_use] +use serde::Serialize; + +/// Holds a list of soft errors. See module-level docs. +#[derive(Debug)] pub struct SoftErrorList { errors: Vec, } -impl SoftErrorList { - /// Returns `Some(Self)` if the list contains at least one soft error - pub fn some(self) -> Option { - if !self.is_empty() { - Some(self) - } else { - None +impl SoftErrorList<()> { + /// Create a sublist that will never be used + /// + /// Useful when calling a function that returns soft errors, but the caller doesn't care. + pub fn null_sublist() -> SoftErrorSublist<'static, T> { + SoftErrorSublist { + list: SoftErrorList::default(), + sink: None, } } - /// Returns `true` if the list is empty +} + +impl SoftErrorList { + /// Returns true if there are no errors in the list pub fn is_empty(&self) -> bool { self.errors.is_empty() } - /// Add a soft error to the list + /// Returns the number of errors in the list + pub fn len(&self) -> usize { + self.errors.len() + } + /// Add a new error to the end of the list pub fn push(&mut self, error: E) { self.errors.push(error); } + /// Immutable iteration of the list items + pub fn iter(&self) -> impl Iterator { + self.errors.iter() + } + /// Mutable iteration of the list items + pub fn iter_mut(&mut self) -> impl Iterator { + self.errors.iter_mut() + } + /// Create a sublist that will be inserted directly into the caller's error list + /// + /// Useful for a group of highly-cohesive functions that should all return one list of soft + /// errors + pub fn inserted_sublist<'a>(&'a mut self) -> SoftErrorSublist<'a, E> { + SoftErrorSublist { + list: SoftErrorList::default(), + sink: Some(Box::new(SimplePush { target: self })), + } + } + /// Create a sublist that will be mapped into a single error in the caller's error list + /// + /// This is useful to bridge abstraction boundaries, where an entire list of soft errors that + /// occurred during a subfunction are wrapped up in a single error item on the caller's side + /// and pushed into the caller's error list. + pub fn map_sublist<'a, T, F>(&'a mut self, map_fn: F) -> SoftErrorSublist<'a, T> + where + F: FnOnce(SoftErrorList) -> E + 'a, + { + SoftErrorSublist { + list: SoftErrorList::default(), + sink: Some(Box::new(MapPush { + map_fn, + target: self, + })), + } + } +} + +impl SoftErrorList { + pub fn to_json(&self) -> Result { + serde_json::to_value(self) + } +} + +impl Serialize for SoftErrorList { + fn serialize(&self, serializer: S) -> Result { + self.errors.serialize(serializer) + } } impl Default for SoftErrorList { @@ -53,9 +110,8 @@ impl Default for SoftErrorList { } } -impl SoftErrorList { - // Helper function for the Debug and Display traits - fn fmt(&self, f: &mut std::fmt::Formatter<'_>, write_sources: bool) -> std::fmt::Result { +impl std::fmt::Display for SoftErrorList { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { writeln!(f, "one or more soft errors occurred:")?; writeln!(f)?; for (i, e) in self.errors.iter().enumerate() { @@ -67,35 +123,228 @@ impl SoftErrorList { writeln!(f)?; - if write_sources { - let mut source = e.source(); - while let Some(e) = source { - writeln!(f, " caused by:")?; + let mut source = e.source(); + while let Some(e) = source { + writeln!(f, " caused by:")?; - for line in e.to_string().lines() { - writeln!(f, " {line}")?; - } + for line in e.to_string().lines() { + writeln!(f, " {line}")?; + } - writeln!(f)?; + writeln!(f)?; - source = e.source(); - } + source = e.source(); } } Ok(()) } } -impl std::fmt::Debug for SoftErrorList { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.fmt(f, true) +impl std::error::Error for SoftErrorList {} + +impl IntoIterator for SoftErrorList { + type Item = as IntoIterator>::Item; + type IntoIter = as IntoIterator>::IntoIter; + fn into_iter(self) -> Self::IntoIter { + self.errors.into_iter() } } -impl std::fmt::Display for SoftErrorList { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.fmt(f, false) +/// A sublist that will be merged into the caller's error list on [Drop] +/// +/// Every sublist holds a reference to the caller's error list for its lifetime. When the sublist +/// goes out of scope, it will be merged into the caller's error list using whatever strategy the +/// caller asked for. Current strategies are: "do nothing and silently drop", "directly push every +/// sublist error into caller error", and "map sublist into a single item in caller's error list". +pub struct SoftErrorSublist<'a, E> { + list: SoftErrorList, + sink: Option + 'a>>, +} + +/// Will move the sublist into whatever [ErrorListSink] was passed in during creation +impl<'a, E> Drop for SoftErrorSublist<'a, E> { + fn drop(&mut self) { + if !self.list.is_empty() { + let list = std::mem::take(&mut self.list); + let sink = self.sink.take().unwrap(); + sink.sink(list); + } } } -impl std::error::Error for SoftErrorList {} +impl<'a, E> std::ops::Deref for SoftErrorSublist<'a, E> { + type Target = SoftErrorList; + fn deref(&self) -> &Self::Target { + &self.list + } +} + +impl<'a, E> std::ops::DerefMut for SoftErrorSublist<'a, E> { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.list + } +} + +/// Something that can accept a [SoftErrorList] +/// +/// Will be implemented by the different strategies for merging a sublist into its caller's +/// object. +trait ErrorListSink { + fn sink(self: Box, list: SoftErrorList); +} + +/// An ErrorListSink that will use a mapping function to convert the list to the caller's error +/// type and push it on their list. +struct MapPush<'a, F, TargetErr> { + map_fn: F, + target: &'a mut SoftErrorList, +} + +impl<'a, F, SourceErr, TargetErr> ErrorListSink for MapPush<'a, F, TargetErr> +where + F: FnOnce(SoftErrorList) -> TargetErr, +{ + fn sink(self: Box, list: SoftErrorList) { + let target_error = (self.map_fn)(list); + self.target.push(target_error); + } +} + +/// An ErrorListSink that will simply push all items in the list onto the caller's error list +/// without any conversion. +struct SimplePush<'a, E> { + target: &'a mut SoftErrorList, +} + +impl<'a, E> ErrorListSink for SimplePush<'a, E> { + fn sink(self: Box, list: SoftErrorList) { + self.target.errors.extend(list.errors); + } +} + +/// Functions used by Serde to serialize types that we don't own (and thus can't implement +/// [Serialize] for) +pub mod serializers { + use serde::Serializer; + /// Useful for types that implement [Error][std::error::Error] and don't need any special + /// treatment. + fn serialize_generic_error( + error: &E, + serializer: S, + ) -> Result { + // I guess we'll have to see if it's more useful to store the debug representation of a + // foreign error type or something else (like maybe iterating its error chain into a + // list?) + let dbg = format!("{error:#?}"); + serializer.serialize_str(&dbg) + } + /// Serialize [std::io::Error] + pub fn serialize_io_error( + error: &std::io::Error, + serializer: S, + ) -> Result { + serialize_generic_error(error, serializer) + } + /// Serialize [goblin::error::Error] + pub fn serialize_goblin_error( + error: &goblin::error::Error, + serializer: S, + ) -> Result { + serialize_generic_error(error, serializer) + } + /// Serialize [nix::Error] + pub fn serialize_nix_error( + error: &nix::Error, + serializer: S, + ) -> Result { + serialize_generic_error(error, serializer) + } + /// Serialize [procfs_core::ProcError] + pub fn serialize_proc_error( + error: &procfs_core::ProcError, + serializer: S, + ) -> Result { + serialize_generic_error(error, serializer) + } + /// Serialize [std::string::FromUtf8Error] + pub fn serialize_from_utf8_error( + error: &std::string::FromUtf8Error, + serializer: S, + ) -> Result { + serialize_generic_error(error, serializer) + } + /// Serialize [std::time::SystemTimeError] + pub fn serialize_system_time_error( + error: &std::time::SystemTimeError, + serializer: S, + ) -> Result { + serialize_generic_error(error, serializer) + } + /// Serialize [scroll::Error] + pub fn serialize_scroll_error( + error: &scroll::Error, + serializer: S, + ) -> Result { + serialize_generic_error(error, serializer) + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[derive(Debug)] + enum OuterError { + Outer, + Middle(SoftErrorList), + } + + #[derive(Debug)] + enum MiddleError { + Middle, + Inner(SoftErrorList), + } + + #[derive(Debug)] + enum InnerError { + Foobar, + } + + #[test] + fn basic() { + let mut soft_errors = SoftErrorList::default(); + soft_errors.push(OuterError::Outer); + middle(soft_errors.map_sublist(OuterError::Middle)); + soft_errors.push(OuterError::Outer); + + // Check outer + let mut outer_it = soft_errors.into_iter(); + assert!(matches!(outer_it.next(), Some(OuterError::Outer))); + let Some(OuterError::Middle(middle)) = outer_it.next() else { + panic!(); + }; + assert!(matches!(outer_it.next(), Some(OuterError::Outer))); + + // Check middle + let mut middle_it = middle.into_iter(); + assert!(matches!(middle_it.next(), Some(MiddleError::Middle))); + let Some(MiddleError::Inner(inner)) = middle_it.next() else { + panic!(); + }; + assert!(matches!(middle_it.next(), Some(MiddleError::Middle))); + + // Check inner + let mut inner_it = inner.into_iter(); + assert!(matches!(inner_it.next(), Some(InnerError::Foobar))); + } + + fn middle(mut soft_errors: SoftErrorSublist<'_, MiddleError>) { + soft_errors.push(MiddleError::Middle); + inner(soft_errors.map_sublist(MiddleError::Inner)); + soft_errors.push(MiddleError::Middle); + } + + fn inner(mut soft_errors: SoftErrorSublist<'_, InnerError>) { + soft_errors.push(InnerError::Foobar); + } +} diff --git a/src/fail_enabled/active.rs b/src/fail_enabled/active.rs new file mode 100644 index 00000000..aece8278 --- /dev/null +++ b/src/fail_enabled/active.rs @@ -0,0 +1,154 @@ +use std::sync::{LazyLock, Mutex, MutexGuard}; + +/// Evaluates to the second argument if fail is enabled, otherwise the third argument. +#[macro_export] +macro_rules! if_fail_enabled_else(($n: ident, $enabled: expr, $disabled: expr $(,)?) => {{ + if $crate::fail_enabled::Config::get().fail_enabled(crate::fail_enabled::FailName::$n) { + $enabled + } else { + $disabled + } +}}); + +/// Executes the given statement if fail is enabled +#[macro_export] +macro_rules! if_fail_enabled(($n: ident, $e: expr $(,)?) => {{ + $crate::if_fail_enabled_else!($n, $e, ()); +}}); + +/// Returns the given error type (converted with into()) if fail is enabled +#[macro_export] +macro_rules! return_err_if_fail_enabled(($n: ident, $f: expr $(,)?) => {{ + crate::if_fail_enabled!($n, return Err($f.into())); +}}); + +/// Defines a set of flags that can be safely read and written from multiple threads +macro_rules! atomic_flags(($s: ident { + $($f: ident,)+ +}) => { + /// The names of the supported atomic flags + #[derive(Clone, Copy, Debug, PartialEq, Eq)] + pub enum $n { + $($f,)+ + } + + impl $n { + /// The total number of flags + pub const COUNT: usize = last_ident!($n, $($f),+) as usize + 1; + } + + /// An array of AtomicBool that holds the values for all the flags + #[derive(Debug, Default)] + pub struct $s([core::sync::atomic::AtomicBool; $n::COUNT]); + + impl $s { + /// Determine whether a flag is enabled + pub fn get(&self, flag: $n) -> bool { + self.0[flag as usize].load(core::sync::atomic::Ordering::Acquire) + } + /// Set whether a flag is enabled + pub fn set(&self, flag: $n, value: bool) { + self.0[flag as usize].store(value, core::sync::atomic::Ordering::Release) + } + /// Disable all flags + pub fn clear(&self) { + for flag in &self.0 { + flag.store(false, core::sync::atomic::Ordering::Release); + } + } + /// Test whether all flags are disabled + pub fn all_clear(&self) -> bool { + for flag in &self.0 { + let value = flag.load(core::sync::atomic::Ordering::Acquire); + if value { + return false; + } + } + true + } + } +}); + +/// Retrieves the last identifier passed to it +/// +/// Useful to determine the name of the last entry in an enum. +macro_rules! last_ident { + {$n: ident, $first: ident, $($tail: ident),+} => { + last_ident!($n, $($tail),+) + }; + {$n: ident, $last: ident} => { + $n::$last + }; +} + +atomic_flags!(FailEnabledFlags { + StopProcess, + FillMissingAuxvInfo, + ThreadName, + SuspendThreads, + CpuInfoFileOpen, +}); + +/// Configuration for the fail_enabled module +/// +/// Generally there will only be one of these that can be obtained via [Config::get()] +#[derive(Debug, Default)] +pub struct Config { + fail_enabled_flags: FailEnabledFlags, + client_mutex: Mutex<()>, +} + +impl Config { + /// Get a reference to the global object + pub fn get() -> &'static Config { + static INSTANCE: LazyLock = LazyLock::new(Config::default); + &INSTANCE + } + /// Return an exclusive client that a test can use to set its fail config + /// + /// As long as the returned object is held, no other test can mutate the fail enabled flags + pub fn client(&self) -> FailClient<'_> { + // We don't care if the lock gets poisoned, since this mutex isn't protecting any data + let _guard = match self.client_mutex.lock() { + Ok(guard) => guard, + Err(e) => e.into_inner(), + }; + assert!( + self.fail_enabled_flags.all_clear(), + "flags were not properly cleared by last client" + ); + FailClient { + config: self, + _guard, + } + } + /// Check to see if a given fail is enabled + /// + /// Used by all the macros in this module + pub(crate) fn fail_enabled(&self, fail: FailName) -> bool { + self.fail_enabled_flags.get(fail) + } +} + +/// An exclusive client that can change the fail flags +/// +/// It is protected by a mutex so that only one test may ever hold this at a time. +#[derive(Debug)] +pub struct FailClient<'a> { + config: &'a Config, + _guard: MutexGuard<'a, ()>, +} + +impl<'a> FailClient<'a> { + /// Change whether a fail is enabled or not + pub fn set_fail_enabled(&self, fail: FailName, enabled: bool) { + self.config.fail_enabled_flags.set(fail, enabled); + } +} + +/// Will disable all fails +impl<'a> Drop for FailClient<'a> { + fn drop(&mut self) { + self.config.fail_enabled_flags.clear(); + } +} diff --git a/src/fail_enabled/inactive.rs b/src/fail_enabled/inactive.rs new file mode 100644 index 00000000..69a2c5b1 --- /dev/null +++ b/src/fail_enabled/inactive.rs @@ -0,0 +1,8 @@ +#[macro_export] +macro_rules! if_fail_enabled_else(($n: ident, $enabled: expr, $disabled: expr $(,)?) => {$disabled}); + +#[macro_export] +macro_rules! if_fail_enabled(($n: ident, $e: expr $(,)?) => {}); + +#[macro_export] +macro_rules! return_err_if_fail_enabled(($n: ident, $f: expr $(,)?) => {}); diff --git a/src/fail_enabled/mod.rs b/src/fail_enabled/mod.rs new file mode 100644 index 00000000..613fea26 --- /dev/null +++ b/src/fail_enabled/mod.rs @@ -0,0 +1,36 @@ +//! Allows testing code to configure intentional failures at various codepaths +//! +//! It is often useful during testing to force certain codepaths to return an error even if they +//! would have otherwise succeeded. +//! +//! There are several macros that will be defined at the crate level that can be used by code that +//! should be tested: +//! +//! - `if_fail_enabled_else!(name, expr_if_enabled, expr_if_disabled)`: Evaluates to the first +//! expression if fail `name` is enabled, otherwise evaluates to the second expression. +//! +//! - `if_fail_enabled!(name, stmt)`: If fail `name` is enabled, will execute the given +//! statement. +//! +//! - `return_err_if_fail_enabled(name, err_expr)`: If fail `name` is enabled, will perform a +//! `return Err(err_expr.into())`. +//! +//! All of the above macros are no-op if the `fail-enabled` feature is not enabled. +//! +//! # Testing Code +//! +//! Testing code will generally use the global [Config] object via [Config::get]. Calling the +//! [client][Config::client] function will return a [FailClient] object. This object is +//! protected by a mutex so that only one such object can exist in the process at a time. This +//! is necessary because tests in the same source file run concurrently with each other. +//! +//! When the [FailClient] is dropped, all enabled fails will be disabled. This ensures the next +//! test will start with a fresh state. + +#[cfg(feature = "fail-enabled")] +mod active; +#[cfg(feature = "fail-enabled")] +pub use active::*; + +#[cfg(not(feature = "fail-enabled"))] +mod inactive; diff --git a/src/lib.rs b/src/lib.rs index d397d25c..fec5251b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,3 +1,5 @@ +pub use error_list::SoftErrorList; + cfg_if::cfg_if! { if #[cfg(any(target_os = "linux", target_os = "android"))] { mod linux; @@ -20,4 +22,6 @@ pub mod minidump_format; pub mod dir_section; pub mod mem_writer; +pub mod fail_enabled; + mod error_list; diff --git a/src/linux/auxv/mod.rs b/src/linux/auxv/mod.rs index 8834f7fb..27a309b1 100644 --- a/src/linux/auxv/mod.rs +++ b/src/linux/auxv/mod.rs @@ -1,4 +1,4 @@ -use crate::error_list::SoftErrorList; +use crate::error_list::{serializers::*, SoftErrorSublist}; pub use reader::ProcfsAuxvIter; use { @@ -84,16 +84,15 @@ impl AuxvDumpInfo { pub fn try_filling_missing_info( &mut self, pid: Pid, - ) -> Result, AuxvError> { + mut soft_errors: SoftErrorSublist<'_, AuxvError>, + ) -> Result<(), AuxvError> { if self.is_complete() { - return Ok(SoftErrorList::default()); + return Ok(()); } let auxv_path = format!("/proc/{pid}/auxv"); let auxv_file = File::open(&auxv_path).map_err(|e| AuxvError::OpenError(auxv_path, e))?; - let mut soft_errors = SoftErrorList::default(); - for pair_result in ProcfsAuxvIter::new(BufReader::new(auxv_file)) { let AuxvPair { key, value } = match pair_result { Ok(pair) => pair, @@ -114,7 +113,12 @@ impl AuxvDumpInfo { } } - Ok(soft_errors) + crate::if_fail_enabled!( + FillMissingAuxvInfo, + soft_errors.push(AuxvError::InvalidFormat) + ); + + Ok(()) } pub fn get_program_header_count(&self) -> Option { self.program_header_count @@ -136,14 +140,23 @@ impl AuxvDumpInfo { } } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum AuxvError { #[error("Failed to open file {0}")] - OpenError(String, #[source] std::io::Error), + OpenError( + String, + #[source] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), #[error("No auxv entry found for PID {0}")] NoAuxvEntryFound(Pid), #[error("Invalid auxv format (should not hit EOF before AT_NULL)")] InvalidFormat, #[error("IO Error")] - IOError(#[from] std::io::Error), + IOError( + #[from] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), } diff --git a/src/linux/dumper_cpu_info/x86_mips.rs b/src/linux/dumper_cpu_info/x86_mips.rs index 0c03d8e5..988d32d1 100644 --- a/src/linux/dumper_cpu_info/x86_mips.rs +++ b/src/linux/dumper_cpu_info/x86_mips.rs @@ -44,6 +44,11 @@ pub fn write_cpu_information(sys_info: &mut MDRawSystemInfo) -> Result<()> { MDCPUArchitecture::PROCESSOR_ARCHITECTURE_AMD64 } as u16; + crate::return_err_if_fail_enabled!( + CpuInfoFileOpen, + std::io::Error::other("test requested cpuinfo file failure") + ); + let cpuinfo_file = std::fs::File::open(path::PathBuf::from("/proc/cpuinfo"))?; let mut vendor_id = String::new(); diff --git a/src/linux/errors.rs b/src/linux/errors.rs index 9166cf1c..367b2b4b 100644 --- a/src/linux/errors.rs +++ b/src/linux/errors.rs @@ -1,6 +1,9 @@ use crate::{ - dir_section::FileWriterError, error_list::SoftErrorList, maps_reader::MappingInfo, - mem_writer::MemoryWriterError, Pid, + dir_section::FileWriterError, + error_list::{serializers::*, SoftErrorList}, + maps_reader::MappingInfo, + mem_writer::MemoryWriterError, + Pid, }; use goblin; use std::ffi::OsString; @@ -8,10 +11,14 @@ use thiserror::Error; use super::ptrace_dumper::InitError; -#[derive(Error, Debug)] +#[derive(Error, Debug, serde::Serialize)] pub enum MapsReaderError { #[error("Couldn't parse as ELF file")] - ELFParsingFailed(#[from] goblin::error::Error), + ELFParsingFailed( + #[from] + #[serde(serialize_with = "serialize_goblin_error")] + goblin::error::Error, + ), #[error("No soname found (filename: {})", .0.to_string_lossy())] NoSoName(OsString, #[source] ModuleReaderError), @@ -19,79 +26,135 @@ pub enum MapsReaderError { #[error("Map entry malformed: No {0} found")] MapEntryMalformed(&'static str), #[error("Couldn't parse address")] - UnparsableInteger(#[from] std::num::ParseIntError), + UnparsableInteger( + #[from] + #[serde(skip)] + std::num::ParseIntError, + ), #[error("Linux gate location doesn't fit in the required integer type")] - LinuxGateNotConvertable(#[from] std::num::TryFromIntError), + LinuxGateNotConvertable( + #[from] + #[serde(skip)] + std::num::TryFromIntError, + ), // get_mmap() #[error("Not safe to open mapping {}", .0.to_string_lossy())] NotSafeToOpenMapping(OsString), #[error("IO Error")] - FileError(#[from] std::io::Error), + FileError( + #[from] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), #[error("Mmapped file empty or not an ELF file")] MmapSanityCheckFailed, #[error("Symlink does not match ({0} vs. {1})")] SymlinkError(std::path::PathBuf, std::path::PathBuf), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum CpuInfoError { #[error("IO error for file /proc/cpuinfo")] - IOError(#[from] std::io::Error), + IOError( + #[from] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), #[error("Not all entries of /proc/cpuinfo found!")] NotAllProcEntriesFound, #[error("Couldn't parse core from file")] - UnparsableInteger(#[from] std::num::ParseIntError), + UnparsableInteger( + #[from] + #[serde(skip)] + std::num::ParseIntError, + ), #[error("Couldn't parse cores: {0}")] UnparsableCores(String), } -#[derive(Error, Debug)] +#[derive(Error, Debug, serde::Serialize)] pub enum ThreadInfoError { #[error("Index out of bounds: Got {0}, only have {1}")] IndexOutOfBounds(usize, usize), #[error("Either ppid ({1}) or tgid ({2}) not found in {0}")] InvalidPid(String, Pid, Pid), #[error("IO error")] - IOError(#[from] std::io::Error), + IOError( + #[from] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), #[error("Couldn't parse address")] - UnparsableInteger(#[from] std::num::ParseIntError), + UnparsableInteger( + #[from] + #[serde(skip)] + std::num::ParseIntError, + ), #[error("nix::ptrace() error")] - PtraceError(#[from] nix::Error), + PtraceError( + #[from] + #[serde(serialize_with = "serialize_nix_error")] + nix::Error, + ), #[error("Invalid line in /proc/{0}/status: {1}")] InvalidProcStatusFile(Pid, String), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum AndroidError { #[error("Failed to copy memory from process")] CopyFromProcessError(#[from] DumperError), #[error("Failed slice conversion")] - TryFromSliceError(#[from] std::array::TryFromSliceError), + TryFromSliceError( + #[from] + #[serde(skip)] + std::array::TryFromSliceError, + ), #[error("No Android rel found")] NoRelFound, } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] #[error("Copy from process {child} failed (source {src}, offset: {offset}, length: {length})")] pub struct CopyFromProcessError { pub child: Pid, pub src: usize, pub offset: usize, pub length: usize, + #[serde(serialize_with = "serialize_nix_error")] pub source: nix::Error, } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum DumperError { #[error("Failed to get PAGE_SIZE from system")] - SysConfError(#[from] nix::Error), + SysConfError( + #[from] + #[serde(serialize_with = "serialize_nix_error")] + nix::Error, + ), #[error("wait::waitpid(Pid={0}) failed")] - WaitPidError(Pid, #[source] nix::Error), + WaitPidError( + Pid, + #[source] + #[serde(serialize_with = "serialize_nix_error")] + nix::Error, + ), #[error("nix::ptrace::attach(Pid={0}) failed")] - PtraceAttachError(Pid, #[source] nix::Error), + PtraceAttachError( + Pid, + #[source] + #[serde(serialize_with = "serialize_nix_error")] + nix::Error, + ), #[error("nix::ptrace::detach(Pid={0}) failed")] - PtraceDetachError(Pid, #[source] nix::Error), + PtraceDetachError( + Pid, + #[source] + #[serde(serialize_with = "serialize_nix_error")] + nix::Error, + ), #[error(transparent)] CopyFromProcessError(#[from] CopyFromProcessError), #[error("Skipped thread {0} due to it being part of the seccomp sandbox's trusted code")] @@ -99,20 +162,32 @@ pub enum DumperError { #[error("No mapping for stack pointer found")] NoStackPointerMapping, #[error("Failed slice conversion")] - TryFromSliceError(#[from] std::array::TryFromSliceError), + TryFromSliceError( + #[from] + #[serde(skip)] + std::array::TryFromSliceError, + ), #[error("Couldn't parse as ELF file")] - ELFParsingFailed(#[from] goblin::error::Error), + ELFParsingFailed( + #[from] + #[serde(serialize_with = "serialize_goblin_error")] + goblin::error::Error, + ), #[error("Could not read value from module")] ModuleReaderError(#[from] ModuleReaderError), #[error("Not safe to open mapping: {}", .0.to_string_lossy())] NotSafeToOpenMapping(OsString), #[error("Failed integer conversion")] - TryFromIntError(#[from] std::num::TryFromIntError), + TryFromIntError( + #[from] + #[serde(skip)] + std::num::TryFromIntError, + ), #[error("Maps reader error")] MapsReaderError(#[from] MapsReaderError), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum SectionAppMemoryError { #[error("Failed to copy memory from process")] CopyFromProcessError(#[from] DumperError), @@ -120,23 +195,31 @@ pub enum SectionAppMemoryError { MemoryWriterError(#[from] MemoryWriterError), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum SectionExceptionStreamError { #[error("Failed to write to memory")] MemoryWriterError(#[from] MemoryWriterError), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum SectionHandleDataStreamError { #[error("Failed to access file")] - IOError(#[from] std::io::Error), + IOError( + #[from] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), #[error("Failed to write to memory")] MemoryWriterError(#[from] MemoryWriterError), #[error("Failed integer conversion")] - TryFromIntError(#[from] std::num::TryFromIntError), + TryFromIntError( + #[from] + #[serde(skip)] + std::num::TryFromIntError, + ), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum SectionMappingsError { #[error("Failed to write to memory")] MemoryWriterError(#[from] MemoryWriterError), @@ -144,21 +227,25 @@ pub enum SectionMappingsError { GetEffectivePathError(MappingInfo, #[source] MapsReaderError), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum SectionMemInfoListError { #[error("Failed to write to memory")] MemoryWriterError(#[from] MemoryWriterError), #[error("Failed to read from procfs")] - ProcfsError(#[from] procfs_core::ProcError), + ProcfsError( + #[from] + #[serde(serialize_with = "serialize_proc_error")] + procfs_core::ProcError, + ), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum SectionMemListError { #[error("Failed to write to memory")] MemoryWriterError(#[from] MemoryWriterError), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum SectionSystemInfoError { #[error("Failed to write to memory")] MemoryWriterError(#[from] MemoryWriterError), @@ -168,31 +255,47 @@ pub enum SectionSystemInfoError { WriteCpuInformationFailed(#[source] CpuInfoError), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum SectionThreadListError { #[error("Failed to write to memory")] MemoryWriterError(#[from] MemoryWriterError), #[error("Failed integer conversion")] - TryFromIntError(#[from] std::num::TryFromIntError), + TryFromIntError( + #[from] + #[serde(skip)] + std::num::TryFromIntError, + ), #[error("Failed to copy memory from process")] CopyFromProcessError(#[from] DumperError), #[error("Failed to get thread info")] ThreadInfoError(#[from] ThreadInfoError), #[error("Failed to write to memory buffer")] - IOError(#[from] std::io::Error), + IOError( + #[from] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum SectionThreadNamesError { #[error("Failed integer conversion")] - TryFromIntError(#[from] std::num::TryFromIntError), + TryFromIntError( + #[from] + #[serde(skip)] + std::num::TryFromIntError, + ), #[error("Failed to write to memory")] MemoryWriterError(#[from] MemoryWriterError), #[error("Failed to write to memory buffer")] - IOError(#[from] std::io::Error), + IOError( + #[from] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum SectionDsoDebugError { #[error("Failed to write to memory")] MemoryWriterError(#[from] MemoryWriterError), @@ -201,10 +304,14 @@ pub enum SectionDsoDebugError { #[error("Failed to copy memory from process")] CopyFromProcessError(#[from] DumperError), #[error("Failed to copy memory from process")] - FromUTF8Error(#[from] std::string::FromUtf8Error), + FromUTF8Error( + #[from] + #[serde(serialize_with = "serialize_from_utf8_error")] + std::string::FromUtf8Error, + ), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum WriterError { #[error("Error during init phase")] InitError(#[from] InitError), @@ -235,11 +342,17 @@ pub enum WriterError { #[error("Failed to write to file")] FileWriterError(#[from] FileWriterError), #[error("Failed to get current timestamp when writing header of minidump")] - SystemTimeError(#[from] std::time::SystemTimeError), + SystemTimeError( + #[from] + #[serde(serialize_with = "serialize_system_time_error")] + std::time::SystemTimeError, + ), #[error("Errors occurred while initializing PTraceDumper")] InitErrors(#[source] SoftErrorList), #[error("Errors occurred while suspending threads")] SuspendThreadsErrors(#[source] SoftErrorList), + #[error("Errors occurred while resuming threads")] + ResumeThreadsErrors(#[source] SoftErrorList), #[error("Crash thread does not reference principal mapping")] PrincipalMappingNotReferenced, #[error("Errors occurred while writing system info")] @@ -268,14 +381,21 @@ pub enum WriterError { WriteHandleDataStreamDirentFailed(#[source] FileWriterError), #[error("No threads left to suspend out of {0}")] SuspendNoThreadsLeft(usize), + #[error("Failed to convert soft error list to JSON")] + ConvertToJsonFailed( + #[source] + #[serde(skip)] + serde_json::Error, + ), } -#[derive(Debug, Error)] +#[derive(Debug, Error, serde::Serialize)] pub enum ModuleReaderError { #[error("failed to read module file ({path}): {error}")] MapFile { path: std::path::PathBuf, #[source] + #[serde(serialize_with = "serialize_io_error")] error: std::io::Error, }, #[error("failed to read module memory: {length} bytes at {offset}{}: {error}", .start_address.map(|addr| format!(" (start address: {addr})")).unwrap_or_default())] @@ -284,10 +404,15 @@ pub enum ModuleReaderError { length: u64, start_address: Option, #[source] + #[serde(serialize_with = "serialize_nix_error")] error: nix::Error, }, #[error("failed to parse ELF memory: {0}")] - Parsing(#[from] goblin::error::Error), + Parsing( + #[from] + #[serde(serialize_with = "serialize_goblin_error")] + goblin::error::Error, + ), #[error("no build id notes in program headers")] NoProgramHeaderNote, #[error("no string table available to locate note sections")] diff --git a/src/linux/maps_reader.rs b/src/linux/maps_reader.rs index e023a21a..6e8f79ef 100644 --- a/src/linux/maps_reader.rs +++ b/src/linux/maps_reader.rs @@ -1,5 +1,4 @@ -use crate::auxv::AuxvType; -use crate::errors::MapsReaderError; +use crate::{auxv::AuxvType, errors::MapsReaderError}; use byteorder::{NativeEndian, ReadBytesExt}; use goblin::elf; use memmap2::{Mmap, MmapOptions}; @@ -13,7 +12,7 @@ pub const DELETED_SUFFIX: &[u8] = b" (deleted)"; type Result = std::result::Result; -#[derive(Debug, PartialEq, Eq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone, serde::Serialize)] pub struct SystemMappingInfo { pub start_address: usize, pub end_address: usize, @@ -21,7 +20,7 @@ pub struct SystemMappingInfo { // One of these is produced for each mapping in the process (i.e. line in // /proc/$x/maps). -#[derive(Debug, PartialEq, Eq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone, serde::Serialize)] pub struct MappingInfo { // On Android, relocation packing can mean that the reported start // address of the mapping must be adjusted by a bias in order to @@ -112,7 +111,8 @@ impl MappingInfo { let is_path = is_mapping_a_path(pathname.as_deref()); - if !is_path && linux_gate_loc != 0 && start_address == linux_gate_loc.try_into()? { + if !is_path && linux_gate_loc != 0 && start_address == usize::try_from(linux_gate_loc)? + { pathname = Some(LINUX_GATE_LIBRARY_NAME.into()); offset = 0; } diff --git a/src/linux/minidump_writer.rs b/src/linux/minidump_writer.rs index 72d6800c..14a56fd1 100644 --- a/src/linux/minidump_writer.rs +++ b/src/linux/minidump_writer.rs @@ -148,22 +148,21 @@ impl MinidumpWriter { let mut soft_errors = SoftErrorList::default(); - let (mut dumper, init_soft_errors) = - PtraceDumper::new_report_soft_errors(self.process_id, self.stop_timeout, auxv)?; - - if !init_soft_errors.is_empty() { - soft_errors.push(WriterError::InitErrors(init_soft_errors)); - } + let mut dumper = PtraceDumper::new_report_soft_errors( + self.process_id, + self.stop_timeout, + auxv, + soft_errors.map_sublist(WriterError::InitErrors), + )?; let threads_count = dumper.threads.len(); - if let Some(suspend_soft_errors) = dumper.suspend_threads().some() { - if dumper.threads.is_empty() { - // TBH I'm not sure this even needs to be a hard error. Is a minidump without any - // thread info still at-least a little useful? - return Err(WriterError::SuspendNoThreadsLeft(threads_count)); - } - soft_errors.push(WriterError::SuspendThreadsErrors(suspend_soft_errors)); + dumper.suspend_threads(soft_errors.map_sublist(WriterError::SuspendThreadsErrors)); + + if dumper.threads.is_empty() { + // TBH I'm not sure this even needs to be a hard error. Is a minidump without any + // thread info still at-least a little useful? + return Err(WriterError::SuspendNoThreadsLeft(threads_count)); } dumper.late_init()?; @@ -182,7 +181,8 @@ impl MinidumpWriter { self.generate_dump(&mut buffer, &mut dumper, soft_errors, destination)?; // TODO - Record these errors? Or maybe we don't care? - let _resume_soft_errors = dumper.resume_threads(); + let mut soft_errors = SoftErrorList::default(); + dumper.resume_threads(soft_errors.map_sublist(WriterError::ResumeThreadsErrors)); Ok(buffer.into()) } @@ -290,13 +290,12 @@ impl MinidumpWriter { let dirent = exception_stream::write(self, buffer)?; dir_section.write_to_file(buffer, Some(dirent))?; - let (dirent, systeminfo_soft_errors) = systeminfo_stream::write(buffer)?; + let dirent = systeminfo_stream::write( + buffer, + soft_errors.map_sublist(WriterError::WriteSystemInfoErrors), + )?; dir_section.write_to_file(buffer, Some(dirent))?; - if !systeminfo_soft_errors.is_empty() { - soft_errors.push(WriterError::WriteSystemInfoErrors(systeminfo_soft_errors)); - } - let dirent = memory_info_list_stream::write(self, buffer)?; dir_section.write_to_file(buffer, Some(dirent))?; @@ -455,8 +454,10 @@ fn write_soft_errors( buffer: &mut DumpBuf, soft_errors: SoftErrorList, ) -> Result { - let soft_error_list_str = format!("{soft_errors:?}"); - - let section = MemoryArrayWriter::write_bytes(buffer, soft_error_list_str.as_bytes()); + let soft_errors_json = soft_errors + .to_json() + .map_err(WriterError::ConvertToJsonFailed)?; + let soft_errors_json_str = format!("{soft_errors_json:#}"); + let section = MemoryArrayWriter::write_bytes(buffer, soft_errors_json_str.as_bytes()); Ok(section.location()) } diff --git a/src/linux/ptrace_dumper.rs b/src/linux/ptrace_dumper.rs index 28379063..c63adb5d 100644 --- a/src/linux/ptrace_dumper.rs +++ b/src/linux/ptrace_dumper.rs @@ -3,7 +3,7 @@ use crate::linux::android::late_process_mappings; #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] use crate::thread_info; use crate::{ - error_list::SoftErrorList, + error_list::{serializers::*, SoftErrorList, SoftErrorSublist}, linux::{ auxv::AuxvDumpInfo, errors::{DumperError, ThreadInfoError}, @@ -57,22 +57,31 @@ pub const AT_SYSINFO_EHDR: u64 = 33; impl Drop for PtraceDumper { fn drop(&mut self) { // Always try to resume all threads (e.g. in case of error) - let _ = self.resume_threads(); + self.resume_threads(SoftErrorList::null_sublist()); // Always allow the process to continue. let _ = self.continue_process(); } } -#[derive(Debug, thiserror::Error)] +#[derive(Debug, thiserror::Error, serde::Serialize)] pub enum InitError { #[error("failed to read auxv")] ReadAuxvFailed(#[source] crate::auxv::AuxvError), #[error("IO error for file {0}")] - IOError(String, #[source] std::io::Error), + IOError( + String, + #[source] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), #[error("Failed Android specific late init")] AndroidLateInitError(#[from] AndroidError), #[error("Failed to read the page size")] - PageSizeError(#[from] Errno), + PageSizeError( + #[from] + #[serde(serialize_with = "serialize_nix_error")] + nix::Error, + ), #[error("Ptrace does not function within the same process")] CannotPtraceSameProcess, #[error("Failed to stop the target process")] @@ -82,11 +91,19 @@ pub enum InitError { #[error("Failed filling missing Auxv info")] FillMissingAuxvInfoFailed(#[source] AuxvError), #[error("Failed reading proc/pid/task entry for process")] - ReadProcessThreadEntryFailed(#[source] std::io::Error), + ReadProcessThreadEntryFailed( + #[source] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), #[error("Process task entry `{0:?}` could not be parsed as a TID")] ProcessTaskEntryNotTid(OsString), #[error("Failed to read thread name")] - ReadThreadNameFailed(#[source] std::io::Error), + ReadThreadNameFailed( + #[source] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), #[error("Proc task directory `{0:?}` is not a directory")] ProcPidTaskNotDirectory(String), #[error("Errors while enumerating threads")] @@ -94,19 +111,31 @@ pub enum InitError { #[error("Failed to enumerate threads")] EnumerateThreadsFailed(#[source] Box), #[error("Failed to read process map file")] - ReadProcessMapFileFailed(#[source] ProcError), + ReadProcessMapFileFailed( + #[source] + #[serde(serialize_with = "serialize_proc_error")] + ProcError, + ), #[error("Failed to aggregate process mappings")] AggregateMappingsFailed(#[source] MapsReaderError), #[error("Failed to enumerate process mappings")] EnumerateMappingsFailed(#[source] Box), } -#[derive(Debug, thiserror::Error)] +#[derive(Debug, thiserror::Error, serde::Serialize)] pub enum StopProcessError { #[error("Failed to stop the process")] - Stop(#[from] Errno), + Stop( + #[from] + #[serde(serialize_with = "serialize_nix_error")] + nix::Error, + ), #[error("Failed to get the process state")] - State(#[from] ProcError), + State( + #[from] + #[serde(serialize_with = "serialize_proc_error")] + ProcError, + ), #[error("Timeout waiting for process to stop")] Timeout, } @@ -139,8 +168,9 @@ impl PtraceDumper { pid: Pid, stop_timeout: Duration, auxv: AuxvDumpInfo, - ) -> Result<(Self, SoftErrorList), InitError> { - if pid == std::process::id() as _ { + soft_errors: SoftErrorSublist<'_, InitError>, + ) -> Result { + if pid == std::process::id() as i32 { return Err(InitError::CannotPtraceSameProcess); } @@ -152,14 +182,16 @@ impl PtraceDumper { mappings: Vec::new(), page_size: 0, }; - let soft_errors = dumper.init(stop_timeout)?; - Ok((dumper, soft_errors)) + dumper.init(stop_timeout, soft_errors)?; + Ok(dumper) } // TODO: late_init for chromeos and android - pub fn init(&mut self, stop_timeout: Duration) -> Result, InitError> { - let mut soft_errors = SoftErrorList::default(); - + pub fn init( + &mut self, + stop_timeout: Duration, + mut soft_errors: SoftErrorSublist<'_, InitError>, + ) -> Result<(), InitError> { // Stopping the process is best-effort. if let Err(e) = self.stop_process(stop_timeout) { soft_errors.push(InitError::StopProcessFailed(e)); @@ -167,41 +199,31 @@ impl PtraceDumper { // Even if we completely fail to fill in any additional Auxv info, we can still press // forward. - match self.auxv.try_filling_missing_info(self.pid) { - Ok(auxv_soft_errors) if !auxv_soft_errors.is_empty() => { - soft_errors.push(InitError::FillMissingAuxvInfoErrors(auxv_soft_errors)); - } - Err(e) => { - soft_errors.push(InitError::FillMissingAuxvInfoFailed(e)); - } - _ => (), + if let Err(e) = self.auxv.try_filling_missing_info( + self.pid, + soft_errors.map_sublist(InitError::FillMissingAuxvInfoErrors), + ) { + soft_errors.push(InitError::FillMissingAuxvInfoFailed(e)); } // If we completely fail to enumerate any threads... Some information is still better than // no information! - match self.enumerate_threads() { - Ok(enumerate_soft_errors) if !enumerate_soft_errors.is_empty() => { - soft_errors.push(InitError::EnumerateThreadsErrors(enumerate_soft_errors)); - } - Err(e) => { - soft_errors.push(InitError::EnumerateThreadsFailed(Box::new(e))); - } - _ => (), + if let Err(e) = + self.enumerate_threads(soft_errors.map_sublist(InitError::EnumerateThreadsErrors)) + { + soft_errors.push(InitError::EnumerateThreadsFailed(Box::new(e))); } // Same with mappings -- Some information is still better than no information! - match self.enumerate_mappings() { - Ok(()) => (), - Err(e) => { - soft_errors.push(InitError::EnumerateMappingsFailed(Box::new(e))); - } + if let Err(e) = self.enumerate_mappings() { + soft_errors.push(InitError::EnumerateMappingsFailed(Box::new(e))); } self.page_size = nix::unistd::sysconf(nix::unistd::SysconfVar::PAGE_SIZE)? .expect("page size apparently unlimited: doesn't make sense.") as usize; - Ok(soft_errors) + Ok(()) } #[cfg_attr(not(target_os = "android"), allow(clippy::unused_self))] @@ -286,9 +308,7 @@ impl PtraceDumper { ptrace_detach(child) } - pub fn suspend_threads(&mut self) -> SoftErrorList { - let mut soft_errors = SoftErrorList::default(); - + pub fn suspend_threads(&mut self, mut soft_errors: SoftErrorSublist<'_, DumperError>) { // Iterate over all threads and try to suspend them. // If the thread either disappeared before we could attach to it, or if // it was part of the seccomp sandbox's trusted code, it is OK to @@ -302,11 +322,14 @@ impl PtraceDumper { }); self.threads_suspended = true; - soft_errors + + crate::if_fail_enabled!( + SuspendThreads, + soft_errors.push(DumperError::PtraceAttachError(1234, nix::Error::EPERM)) + ); } - pub fn resume_threads(&mut self) -> SoftErrorList { - let mut soft_errors = SoftErrorList::default(); + pub fn resume_threads(&mut self, mut soft_errors: SoftErrorSublist<'_, DumperError>) { if self.threads_suspended { for thread in &self.threads { match Self::resume_thread(thread.tid) { @@ -318,13 +341,14 @@ impl PtraceDumper { } } self.threads_suspended = false; - soft_errors } /// Send SIGSTOP to the process so that we can get a consistent state. /// /// This will block waiting for the process to stop until `timeout` has passed. fn stop_process(&mut self, timeout: Duration) -> Result<(), StopProcessError> { + crate::return_err_if_fail_enabled!(StopProcess, nix::Error::EPERM); + signal::kill(nix::unistd::Pid::from_raw(self.pid), Some(signal::SIGSTOP))?; // Something like waitpid for non-child processes would be better, but we have no such @@ -355,9 +379,10 @@ impl PtraceDumper { /// Parse /proc/$pid/task to list all the threads of the process identified by /// pid. - fn enumerate_threads(&mut self) -> Result, InitError> { - let mut soft_errors = SoftErrorList::default(); - + fn enumerate_threads( + &mut self, + mut soft_errors: SoftErrorSublist<'_, InitError>, + ) -> Result<(), InitError> { let pid = self.pid; let filename = format!("/proc/{}/task", pid); let task_path = path::PathBuf::from(&filename); @@ -383,7 +408,15 @@ impl PtraceDumper { }; // Read the thread-name (if there is any) - let name = match std::fs::read_to_string(format!("/proc/{}/task/{}/comm", pid, tid)) { + let name_result = crate::if_fail_enabled_else!( + ThreadName, + Err(std::io::Error::other( + "testing requested failure reading thread name" + )), + std::fs::read_to_string(format!("/proc/{}/task/{}/comm", pid, tid)) + ); + + let name = match name_result { Ok(name) => Some(name.trim_end().to_string()), Err(e) => { soft_errors.push(InitError::ReadThreadNameFailed(e)); @@ -394,7 +427,7 @@ impl PtraceDumper { self.threads.push(Thread { tid, name }); } - Ok(soft_errors) + Ok(()) } fn enumerate_mappings(&mut self) -> Result<(), InitError> { diff --git a/src/linux/sections/systeminfo_stream.rs b/src/linux/sections/systeminfo_stream.rs index 5e48b170..f8881b41 100644 --- a/src/linux/sections/systeminfo_stream.rs +++ b/src/linux/sections/systeminfo_stream.rs @@ -1,10 +1,11 @@ use super::*; -use crate::{error_list::SoftErrorList, linux::dumper_cpu_info as dci}; +use crate::{error_list::*, linux::dumper_cpu_info as dci}; use errors::SectionSystemInfoError; pub fn write( buffer: &mut DumpBuf, -) -> Result<(MDRawDirectory, SoftErrorList), SectionSystemInfoError> { + mut soft_errors: SoftErrorSublist<'_, SectionSystemInfoError>, +) -> Result { let mut info_section = MemoryWriter::::alloc(buffer)?; let dirent = MDRawDirectory { stream_type: MDStreamType::SystemInfoStream as u32, @@ -19,11 +20,10 @@ pub fn write( info.platform_id = platform_id as u32; info.csd_version_rva = os_version_loc.rva; - let mut soft_errors = SoftErrorList::default(); if let Err(e) = dci::write_cpu_information(&mut info) { soft_errors.push(SectionSystemInfoError::WriteCpuInformationFailed(e)); } info_section.set_value(buffer, info)?; - Ok((dirent, soft_errors)) + Ok(dirent) } diff --git a/src/mem_writer.rs b/src/mem_writer.rs index a703d2b1..ed68daca 100644 --- a/src/mem_writer.rs +++ b/src/mem_writer.rs @@ -1,14 +1,31 @@ -use crate::minidump_format::{MDLocationDescriptor, MDRVA}; -use scroll::ctx::{SizeWith, TryIntoCtx}; - -#[derive(Debug, thiserror::Error)] +use { + crate::{ + error_list::serializers::*, + minidump_format::{MDLocationDescriptor, MDRVA}, + }, + scroll::ctx::{SizeWith, TryIntoCtx}, +}; + +#[derive(Debug, thiserror::Error, serde::Serialize)] pub enum MemoryWriterError { #[error("IO error when writing to DumpBuf")] - IOError(#[from] std::io::Error), + IOError( + #[from] + #[serde(serialize_with = "serialize_io_error")] + std::io::Error, + ), #[error("Failed integer conversion")] - TryFromIntError(#[from] std::num::TryFromIntError), + TryFromIntError( + #[from] + #[serde(skip)] + std::num::TryFromIntError, + ), #[error("Failed to write to buffer")] - Scroll(#[from] scroll::Error), + Scroll( + #[from] + #[serde(serialize_with = "serialize_scroll_error")] + scroll::Error, + ), } type WriteResult = std::result::Result; diff --git a/tests/linux_minidump_writer_soft_error.rs b/tests/linux_minidump_writer_soft_error.rs new file mode 100644 index 00000000..326a959a --- /dev/null +++ b/tests/linux_minidump_writer_soft_error.rs @@ -0,0 +1,119 @@ +#![cfg(any(target_os = "linux", target_os = "android"))] +#![cfg(feature = "fail-enabled")] + +use { + common::*, + minidump::Minidump, + minidump_writer::{ + fail_enabled::{self, FailName}, + minidump_writer::MinidumpWriter, + }, + serde_json::{json, Value as JsonValue}, +}; + +mod common; + +#[test] +fn soft_error_stream() { + let mut child = start_child_and_wait_for_threads(1); + let pid = child.id() as i32; + + let mut tmpfile = tempfile::Builder::new() + .prefix("soft_error_stream") + .tempfile() + .unwrap(); + + let fail_config = fail_enabled::Config::get(); + let fail_client = fail_config.client(); + fail_client.set_fail_enabled(FailName::StopProcess, true); + + // Write a minidump + MinidumpWriter::new(pid, pid) + .dump(&mut tmpfile) + .expect("cound not write minidump"); + child.kill().expect("Failed to kill process"); + + // Ensure the minidump has a MemoryInfoListStream present and has at least one entry. + let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); + dump.get_raw_stream(minidump_common::format::MINIDUMP_STREAM_TYPE::MozSoftErrors.into()) + .expect("missing soft error stream"); +} + +#[test] +fn soft_error_stream_content() { + let expected_json = json!([ + {"InitErrors": [ + {"StopProcessFailed": {"Stop": "EPERM"}}, + {"FillMissingAuxvInfoErrors": ["InvalidFormat"]}, + {"EnumerateThreadsErrors": [ + {"ReadThreadNameFailed": "\ + Custom {\n \ + kind: Other,\n \ + error: \"testing requested failure reading thread name\",\n\ + }" + } + ]} + ]}, + {"SuspendThreadsErrors": [{"PtraceAttachError": [1234, "EPERM"]}]}, + {"WriteSystemInfoErrors": [ + {"WriteCpuInformationFailed": {"IOError": "\ + Custom {\n \ + kind: Other,\n \ + error: \"test requested cpuinfo file failure\",\n\ + }" + }} + ]} + ]); + + let mut child = start_child_and_wait_for_threads(1); + let pid = child.id() as i32; + + let mut tmpfile = tempfile::Builder::new() + .prefix("soft_error_stream_content") + .tempfile() + .unwrap(); + + let fail_config = fail_enabled::Config::get(); + let fail_client = fail_config.client(); + for name in [ + FailName::StopProcess, + FailName::FillMissingAuxvInfo, + FailName::ThreadName, + FailName::SuspendThreads, + FailName::CpuInfoFileOpen, + ] { + fail_client.set_fail_enabled(name, true); + } + + // Write a minidump + MinidumpWriter::new(pid, pid) + .dump(&mut tmpfile) + .expect("cound not write minidump"); + child.kill().expect("Failed to kill process"); + + // Ensure the minidump has a MemoryInfoListStream present and has at least one entry. + let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); + let contents = std::str::from_utf8( + dump.get_raw_stream(minidump_common::format::MINIDUMP_STREAM_TYPE::MozSoftErrors.into()) + .expect("missing soft error stream"), + ) + .expect("expected utf-8 stream"); + + let actual_json: JsonValue = serde_json::from_str(contents).expect("expected json"); + + if actual_json != expected_json { + panic!( + "\ + JSON mismatch:\n\ + =====Expected=====\n\ + \n\ + {expected_json:#}\n\ + \n\ + =====Actual=====\n\ + \n\ + {actual_json:#}\n\ + \n\ + " + ); + } +} diff --git a/tests/ptrace_dumper.rs b/tests/ptrace_dumper.rs index 361997be..616559e8 100644 --- a/tests/ptrace_dumper.rs +++ b/tests/ptrace_dumper.rs @@ -1,7 +1,7 @@ //! All of these tests are specific to ptrace #![cfg(any(target_os = "linux", target_os = "android"))] -use minidump_writer::ptrace_dumper::PtraceDumper; +use minidump_writer::{ptrace_dumper::PtraceDumper, SoftErrorList}; use nix::sys::mman::{mmap, MapFlags, ProtFlags}; use nix::sys::signal::Signal; use std::convert::TryInto; @@ -91,17 +91,18 @@ fn test_thread_list_from_parent() { let num_of_threads = 5; let mut child = start_child_and_wait_for_threads(num_of_threads); let pid = child.id() as i32; - let (mut dumper, _) = PtraceDumper::new_report_soft_errors( + let mut dumper = PtraceDumper::new_report_soft_errors( pid, minidump_writer::minidump_writer::STOP_TIMEOUT, Default::default(), + SoftErrorList::null_sublist(), ) .expect("Couldn't init dumper"); assert_eq!(dumper.threads.len(), num_of_threads); - assert!( - dumper.suspend_threads().is_empty(), - "failed to suspend all threads" - ); + + let mut soft_errors = SoftErrorList::default(); + dumper.suspend_threads(soft_errors.inserted_sublist()); + assert!(soft_errors.is_empty(), "failed to suspend all threads"); // let mut matching_threads = 0; for (idx, curr_thread) in dumper.threads.iter().enumerate() { @@ -149,10 +150,9 @@ fn test_thread_list_from_parent() { 0 }; */ } - assert!( - dumper.resume_threads().is_empty(), - "Failed to resume threads" - ); + let mut soft_errors = SoftErrorList::default(); + dumper.resume_threads(soft_errors.inserted_sublist()); + assert!(soft_errors.is_empty(), "Failed to resume threads"); child.kill().expect("Failed to kill process"); // Reap child @@ -278,17 +278,18 @@ fn test_sanitize_stack_copy() { let heap_addr = usize::from_str_radix(output.next().unwrap().trim_start_matches("0x"), 16) .expect("unable to parse mmap_addr"); - let (mut dumper, _) = PtraceDumper::new_report_soft_errors( + let mut dumper = PtraceDumper::new_report_soft_errors( pid, minidump_writer::minidump_writer::STOP_TIMEOUT, Default::default(), + SoftErrorList::null_sublist(), ) .expect("Couldn't init dumper"); assert_eq!(dumper.threads.len(), num_of_threads); - assert!( - dumper.suspend_threads().is_empty(), - "failed to suspend all threads" - ); + + let mut soft_errors = SoftErrorList::default(); + dumper.suspend_threads(soft_errors.inserted_sublist()); + assert!(soft_errors.is_empty(), "failed to suspend all threads"); let thread_info = dumper .get_thread_info_by_index(0) .expect("Couldn't find thread_info"); @@ -383,10 +384,9 @@ fn test_sanitize_stack_copy() { assert_eq!(simulated_stack[0..size_of::()], defaced); - assert!( - dumper.resume_threads().is_empty(), - "Failed to resume threads" - ); + let mut soft_errors = SoftErrorList::default(); + dumper.resume_threads(soft_errors.inserted_sublist()); + assert!(soft_errors.is_empty(), "Failed to resume threads"); child.kill().expect("Failed to kill process"); // Reap child From 917c4d043fec4a3d6a301a1e8a0a46500296a0f7 Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Fri, 6 Dec 2024 17:23:51 -0500 Subject: [PATCH 3/6] Use error-graph and failspot crates, cleanup patch --- Cargo.lock | 26 ++ Cargo.toml | 7 +- src/bin/test.rs | 193 ++++++------ src/dir_section.rs | 12 +- src/error_list.rs | 350 ---------------------- src/fail_enabled/active.rs | 154 ---------- src/fail_enabled/inactive.rs | 8 - src/fail_enabled/mod.rs | 36 --- src/lib.rs | 19 +- src/linux/auxv/mod.rs | 15 +- src/linux/dumper_cpu_info/x86_mips.rs | 18 +- src/linux/errors.rs | 28 +- src/linux/maps_reader.rs | 38 ++- src/linux/minidump_writer.rs | 61 ++-- src/linux/ptrace_dumper.rs | 140 +++++---- src/linux/sections/systeminfo_stream.rs | 9 +- src/mem_writer.rs | 2 +- src/serializers.rs | 65 ++++ tests/common/mod.rs | 16 + tests/linux_minidump_writer.rs | 8 + tests/linux_minidump_writer_soft_error.rs | 43 +-- tests/ptrace_dumper.rs | 74 +++-- 22 files changed, 462 insertions(+), 860 deletions(-) delete mode 100644 src/error_list.rs delete mode 100644 src/fail_enabled/active.rs delete mode 100644 src/fail_enabled/inactive.rs delete mode 100644 src/fail_enabled/mod.rs create mode 100644 src/serializers.rs diff --git a/Cargo.lock b/Cargo.lock index f5f51aef..52b45645 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -609,6 +609,24 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "error-graph" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b920e777967421aa5f9bf34f842c0ab6ba19b3bdb4a082946093860f5858879" +dependencies = [ + "serde", +] + +[[package]] +name = "failspot" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c942e64b20ecd39933d5ff938ca4fdb6ef0d298cc3855b231179a5ef0b24948d" +dependencies = [ + "flagset", +] + [[package]] name = "fallible-iterator" version = "0.2.0" @@ -627,6 +645,12 @@ version = "2.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9fc0510504f03c51ada170672ac806f1f105a88aa97a5281117e1ddc3368e51a" +[[package]] +name = "flagset" +version = "0.4.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b3ea1ec5f8307826a5b71094dd91fc04d4ae75d5709b20ad351c7fb4815c86ec" + [[package]] name = "flate2" version = "1.0.31" @@ -1268,6 +1292,8 @@ dependencies = [ "crash-context", "current_platform", "dump_syms", + "error-graph", + "failspot", "futures", "goblin", "libc", diff --git a/Cargo.toml b/Cargo.toml index fdeb6d59..4128cb28 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,15 +8,13 @@ homepage = "https://github.com/rust-minidump/minidump-writer" edition = "2021" license = "MIT" -[features] -default = ["fail-enabled"] -fail-enabled = [] - [dependencies] bitflags = "2.4" byteorder = "1.4" cfg-if = "1.0" crash-context = "0.6" +error-graph = { version = "0.1.1", features = ["serde"] } +failspot = "0.2.0" log = "0.4" memoffset = "0.9" minidump-common = "0.22" @@ -55,6 +53,7 @@ mach2 = "0.4" # We auto-detect what the test binary that is spawned for most tests should be # compiled for current_platform = "0.2" +failspot = { version = "0.2.0", features = ["enabled"] } # Minidump-processor is async so we need an executor futures = { version = "0.3", features = ["executor"] } minidump = "0.22" diff --git a/src/bin/test.rs b/src/bin/test.rs index d287dc1e..a2a43e34 100644 --- a/src/bin/test.rs +++ b/src/bin/test.rs @@ -6,14 +6,17 @@ pub type Result = std::result::Result; #[cfg(any(target_os = "linux", target_os = "android"))] mod linux { - use super::*; - use minidump_writer::{ - minidump_writer::STOP_TIMEOUT, module_reader, ptrace_dumper::PtraceDumper, SoftErrorList, - LINUX_GATE_LIBRARY_NAME, - }; - use nix::{ - sys::mman::{mmap_anonymous, MapFlags, ProtFlags}, - unistd::getppid, + use { + super::*, + error_graph::ErrorList, + minidump_writer::{ + minidump_writer::STOP_TIMEOUT, module_reader, ptrace_dumper::PtraceDumper, + LINUX_GATE_LIBRARY_NAME, + }, + nix::{ + sys::mman::{mmap_anonymous, MapFlags, ProtFlags}, + unistd::getppid, + }, }; macro_rules! test { @@ -24,25 +27,40 @@ mod linux { }; } + macro_rules! fail_on_soft_error(($n: ident, $e: expr) => {{ + let mut $n = ErrorList::default(); + let __result = $e; + if !$n.is_empty() { + return Err($n.into()); + } + __result + }}); + fn test_setup() -> Result<()> { let ppid = getppid(); - let _dumper = PtraceDumper::new_report_soft_errors( - ppid.as_raw(), - STOP_TIMEOUT, - Default::default(), - SoftErrorList::null_sublist(), - )?; + fail_on_soft_error!( + soft_errors, + PtraceDumper::new_report_soft_errors( + ppid.as_raw(), + STOP_TIMEOUT, + Default::default(), + &mut soft_errors, + )? + ); Ok(()) } fn test_thread_list() -> Result<()> { let ppid = getppid(); - let dumper = PtraceDumper::new_report_soft_errors( - ppid.as_raw(), - STOP_TIMEOUT, - Default::default(), - SoftErrorList::null_sublist(), - )?; + let dumper = fail_on_soft_error!( + soft_errors, + PtraceDumper::new_report_soft_errors( + ppid.as_raw(), + STOP_TIMEOUT, + Default::default(), + &mut soft_errors, + )? + ); test!(!dumper.threads.is_empty(), "No threads"); test!( dumper @@ -69,18 +87,17 @@ mod linux { use minidump_writer::mem_reader::MemReader; let ppid = getppid().as_raw(); - let mut dumper = PtraceDumper::new_report_soft_errors( - ppid, - STOP_TIMEOUT, - Default::default(), - SoftErrorList::null_sublist(), - )?; - - let mut soft_errors = SoftErrorList::default(); - dumper.suspend_threads(soft_errors.inserted_sublist()); - if !soft_errors.is_empty() { - return Err(soft_errors.into()); - } + let mut dumper = fail_on_soft_error!( + soft_errors, + PtraceDumper::new_report_soft_errors( + ppid, + STOP_TIMEOUT, + Default::default(), + &mut soft_errors + )? + ); + + fail_on_soft_error!(soft_errors, dumper.suspend_threads(&mut soft_errors)); // We support 3 different methods of reading memory from another // process, ensure they all function and give the same results @@ -133,23 +150,22 @@ mod linux { test!(heap_res == expected_heap, "heap var not correct"); - let mut soft_errors = SoftErrorList::default(); - dumper.resume_threads(soft_errors.inserted_sublist()); - if !soft_errors.is_empty() { - return Err(soft_errors.into()); - } + fail_on_soft_error!(soft_errors, dumper.resume_threads(&mut soft_errors)); Ok(()) } fn test_find_mappings(addr1: usize, addr2: usize) -> Result<()> { let ppid = getppid(); - let dumper = PtraceDumper::new_report_soft_errors( - ppid.as_raw(), - STOP_TIMEOUT, - Default::default(), - SoftErrorList::null_sublist(), - )?; + let dumper = fail_on_soft_error!( + soft_errors, + PtraceDumper::new_report_soft_errors( + ppid.as_raw(), + STOP_TIMEOUT, + Default::default(), + &mut soft_errors, + )? + ); dumper .find_mapping(addr1) .ok_or("No mapping for addr1 found")?; @@ -166,17 +182,18 @@ mod linux { let ppid = getppid().as_raw(); let exe_link = format!("/proc/{ppid}/exe"); let exe_name = std::fs::read_link(exe_link)?.into_os_string(); - let mut dumper = PtraceDumper::new_report_soft_errors( - ppid, - STOP_TIMEOUT, - Default::default(), - SoftErrorList::null_sublist(), - )?; - let mut soft_errors = SoftErrorList::default(); - dumper.suspend_threads(soft_errors.inserted_sublist()); - if !soft_errors.is_empty() { - return Err(soft_errors.into()); - } + + let mut dumper = fail_on_soft_error!( + soft_errors, + PtraceDumper::new_report_soft_errors( + ppid, + STOP_TIMEOUT, + Default::default(), + &mut soft_errors + )? + ); + + fail_on_soft_error!(soft_errors, dumper.suspend_threads(&mut soft_errors)); let mut found_exe = None; for (idx, mapping) in dumper.mappings.iter().enumerate() { @@ -187,11 +204,8 @@ mod linux { } let idx = found_exe.unwrap(); let module_reader::BuildId(id) = dumper.from_process_memory_for_index(idx)?; - let mut soft_errors = SoftErrorList::default(); - dumper.resume_threads(soft_errors.inserted_sublist()); - if !soft_errors.is_empty() { - return Err(soft_errors.into()); - } + + fail_on_soft_error!(soft_errors, dumper.resume_threads(&mut soft_errors)); assert!(!id.is_empty()); assert!(id.iter().any(|&x| x > 0)); @@ -200,12 +214,15 @@ mod linux { fn test_merged_mappings(path: String, mapped_mem: usize, mem_size: usize) -> Result<()> { // Now check that PtraceDumper interpreted the mappings properly. - let dumper = PtraceDumper::new_report_soft_errors( - getppid().as_raw(), - STOP_TIMEOUT, - Default::default(), - SoftErrorList::null_sublist(), - )?; + let dumper = fail_on_soft_error!( + soft_errors, + PtraceDumper::new_report_soft_errors( + getppid().as_raw(), + STOP_TIMEOUT, + Default::default(), + &mut soft_errors, + )? + ); let mut mapping_count = 0; for map in &dumper.mappings { if map @@ -227,30 +244,29 @@ mod linux { fn test_linux_gate_mapping_id() -> Result<()> { let ppid = getppid().as_raw(); - let mut dumper = PtraceDumper::new_report_soft_errors( - ppid, - STOP_TIMEOUT, - Default::default(), - SoftErrorList::null_sublist(), - )?; + let mut dumper = fail_on_soft_error!( + soft_errors, + PtraceDumper::new_report_soft_errors( + ppid, + STOP_TIMEOUT, + Default::default(), + &mut soft_errors + )? + ); let mut found_linux_gate = false; for mapping in dumper.mappings.clone() { if mapping.name == Some(LINUX_GATE_LIBRARY_NAME.into()) { found_linux_gate = true; - let mut soft_errors = SoftErrorList::default(); - dumper.suspend_threads(soft_errors.inserted_sublist()); - if !soft_errors.is_empty() { - return Err(soft_errors.into()); - } + + fail_on_soft_error!(soft_errors, dumper.suspend_threads(&mut soft_errors)); + let module_reader::BuildId(id) = PtraceDumper::from_process_memory_for_mapping(&mapping, ppid)?; test!(!id.is_empty(), "id-vec is empty"); test!(id.iter().any(|&x| x > 0), "all id elements are 0"); - let mut soft_errors = SoftErrorList::default(); - dumper.resume_threads(soft_errors.inserted_sublist()); - if !soft_errors.is_empty() { - return Err(soft_errors.into()); - } + + fail_on_soft_error!(soft_errors, dumper.resume_threads(&mut soft_errors)); + break; } } @@ -260,12 +276,15 @@ mod linux { fn test_mappings_include_linux_gate() -> Result<()> { let ppid = getppid().as_raw(); - let dumper = PtraceDumper::new_report_soft_errors( - ppid, - STOP_TIMEOUT, - Default::default(), - SoftErrorList::null_sublist(), - )?; + let dumper = fail_on_soft_error!( + soft_errors, + PtraceDumper::new_report_soft_errors( + ppid, + STOP_TIMEOUT, + Default::default(), + &mut soft_errors + )? + ); let linux_gate_loc = dumper.auxv.get_linux_gate_address().unwrap(); test!(linux_gate_loc != 0, "linux_gate_loc == 0"); let mut found_linux_gate = false; diff --git a/src/dir_section.rs b/src/dir_section.rs index 4515081d..2aaed65a 100644 --- a/src/dir_section.rs +++ b/src/dir_section.rs @@ -1,9 +1,11 @@ -use crate::{ - error_list::serializers::*, - mem_writer::{Buffer, MemoryArrayWriter, MemoryWriterError}, - minidump_format::MDRawDirectory, +use { + crate::{ + mem_writer::{Buffer, MemoryArrayWriter, MemoryWriterError}, + minidump_format::MDRawDirectory, + serializers::*, + }, + std::io::{Error, Seek, Write}, }; -use std::io::{Error, Seek, Write}; pub type DumpBuf = Buffer; diff --git a/src/error_list.rs b/src/error_list.rs deleted file mode 100644 index 417cf7e4..00000000 --- a/src/error_list.rs +++ /dev/null @@ -1,350 +0,0 @@ -//! Encapsulates a list of "soft error"s -//! -//! A "soft error" is an error that is encounted while generating the minidump that doesn't -//! totally prevent the minidump from being useful, but it may have missing or invalid -//! information. -//! -//! It should be returned by a function when the function was able to at-least partially achieve -//! its goals, and when further use of functions in the same API is permissible and can still be -//! at-least partially functional. -//! -//! Admittedly, this concept makes layers of abstraction a bit more difficult, as something that -//! is considered "soft" by one layer may be a deal-breaker for the layer above it, or visa-versa -//! -- an error that a lower layer considers a total failure might just be a nuissance for the layer -//! above it. -//! -//! An example of the former might be the act of suspending all the threads -- The `PTraceDumper`` -//! API will actually work just fine even if none of the threads are suspended, so it only returns -//! a soft error; however, the dumper itself considers it to be a critical failure if not even one -//! thread could be stopped. -//! -//! An example of the latter might trying to stop the process -- Being unable to send SIGSTOP to -//! the process would be considered a critical failure by `stop_process()`, but merely an -//! inconvenience by the code that's calling it. - -use serde::Serialize; - -/// Holds a list of soft errors. See module-level docs. -#[derive(Debug)] -pub struct SoftErrorList { - errors: Vec, -} - -impl SoftErrorList<()> { - /// Create a sublist that will never be used - /// - /// Useful when calling a function that returns soft errors, but the caller doesn't care. - pub fn null_sublist() -> SoftErrorSublist<'static, T> { - SoftErrorSublist { - list: SoftErrorList::default(), - sink: None, - } - } -} - -impl SoftErrorList { - /// Returns true if there are no errors in the list - pub fn is_empty(&self) -> bool { - self.errors.is_empty() - } - /// Returns the number of errors in the list - pub fn len(&self) -> usize { - self.errors.len() - } - /// Add a new error to the end of the list - pub fn push(&mut self, error: E) { - self.errors.push(error); - } - /// Immutable iteration of the list items - pub fn iter(&self) -> impl Iterator { - self.errors.iter() - } - /// Mutable iteration of the list items - pub fn iter_mut(&mut self) -> impl Iterator { - self.errors.iter_mut() - } - /// Create a sublist that will be inserted directly into the caller's error list - /// - /// Useful for a group of highly-cohesive functions that should all return one list of soft - /// errors - pub fn inserted_sublist<'a>(&'a mut self) -> SoftErrorSublist<'a, E> { - SoftErrorSublist { - list: SoftErrorList::default(), - sink: Some(Box::new(SimplePush { target: self })), - } - } - /// Create a sublist that will be mapped into a single error in the caller's error list - /// - /// This is useful to bridge abstraction boundaries, where an entire list of soft errors that - /// occurred during a subfunction are wrapped up in a single error item on the caller's side - /// and pushed into the caller's error list. - pub fn map_sublist<'a, T, F>(&'a mut self, map_fn: F) -> SoftErrorSublist<'a, T> - where - F: FnOnce(SoftErrorList) -> E + 'a, - { - SoftErrorSublist { - list: SoftErrorList::default(), - sink: Some(Box::new(MapPush { - map_fn, - target: self, - })), - } - } -} - -impl SoftErrorList { - pub fn to_json(&self) -> Result { - serde_json::to_value(self) - } -} - -impl Serialize for SoftErrorList { - fn serialize(&self, serializer: S) -> Result { - self.errors.serialize(serializer) - } -} - -impl Default for SoftErrorList { - fn default() -> Self { - Self { errors: Vec::new() } - } -} - -impl std::fmt::Display for SoftErrorList { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - writeln!(f, "one or more soft errors occurred:")?; - writeln!(f)?; - for (i, e) in self.errors.iter().enumerate() { - writeln!(f, " {i}:")?; - - for line in e.to_string().lines() { - writeln!(f, " {line}")?; - } - - writeln!(f)?; - - let mut source = e.source(); - while let Some(e) = source { - writeln!(f, " caused by:")?; - - for line in e.to_string().lines() { - writeln!(f, " {line}")?; - } - - writeln!(f)?; - - source = e.source(); - } - } - Ok(()) - } -} - -impl std::error::Error for SoftErrorList {} - -impl IntoIterator for SoftErrorList { - type Item = as IntoIterator>::Item; - type IntoIter = as IntoIterator>::IntoIter; - fn into_iter(self) -> Self::IntoIter { - self.errors.into_iter() - } -} - -/// A sublist that will be merged into the caller's error list on [Drop] -/// -/// Every sublist holds a reference to the caller's error list for its lifetime. When the sublist -/// goes out of scope, it will be merged into the caller's error list using whatever strategy the -/// caller asked for. Current strategies are: "do nothing and silently drop", "directly push every -/// sublist error into caller error", and "map sublist into a single item in caller's error list". -pub struct SoftErrorSublist<'a, E> { - list: SoftErrorList, - sink: Option + 'a>>, -} - -/// Will move the sublist into whatever [ErrorListSink] was passed in during creation -impl<'a, E> Drop for SoftErrorSublist<'a, E> { - fn drop(&mut self) { - if !self.list.is_empty() { - let list = std::mem::take(&mut self.list); - let sink = self.sink.take().unwrap(); - sink.sink(list); - } - } -} - -impl<'a, E> std::ops::Deref for SoftErrorSublist<'a, E> { - type Target = SoftErrorList; - fn deref(&self) -> &Self::Target { - &self.list - } -} - -impl<'a, E> std::ops::DerefMut for SoftErrorSublist<'a, E> { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.list - } -} - -/// Something that can accept a [SoftErrorList] -/// -/// Will be implemented by the different strategies for merging a sublist into its caller's -/// object. -trait ErrorListSink { - fn sink(self: Box, list: SoftErrorList); -} - -/// An ErrorListSink that will use a mapping function to convert the list to the caller's error -/// type and push it on their list. -struct MapPush<'a, F, TargetErr> { - map_fn: F, - target: &'a mut SoftErrorList, -} - -impl<'a, F, SourceErr, TargetErr> ErrorListSink for MapPush<'a, F, TargetErr> -where - F: FnOnce(SoftErrorList) -> TargetErr, -{ - fn sink(self: Box, list: SoftErrorList) { - let target_error = (self.map_fn)(list); - self.target.push(target_error); - } -} - -/// An ErrorListSink that will simply push all items in the list onto the caller's error list -/// without any conversion. -struct SimplePush<'a, E> { - target: &'a mut SoftErrorList, -} - -impl<'a, E> ErrorListSink for SimplePush<'a, E> { - fn sink(self: Box, list: SoftErrorList) { - self.target.errors.extend(list.errors); - } -} - -/// Functions used by Serde to serialize types that we don't own (and thus can't implement -/// [Serialize] for) -pub mod serializers { - use serde::Serializer; - /// Useful for types that implement [Error][std::error::Error] and don't need any special - /// treatment. - fn serialize_generic_error( - error: &E, - serializer: S, - ) -> Result { - // I guess we'll have to see if it's more useful to store the debug representation of a - // foreign error type or something else (like maybe iterating its error chain into a - // list?) - let dbg = format!("{error:#?}"); - serializer.serialize_str(&dbg) - } - /// Serialize [std::io::Error] - pub fn serialize_io_error( - error: &std::io::Error, - serializer: S, - ) -> Result { - serialize_generic_error(error, serializer) - } - /// Serialize [goblin::error::Error] - pub fn serialize_goblin_error( - error: &goblin::error::Error, - serializer: S, - ) -> Result { - serialize_generic_error(error, serializer) - } - /// Serialize [nix::Error] - pub fn serialize_nix_error( - error: &nix::Error, - serializer: S, - ) -> Result { - serialize_generic_error(error, serializer) - } - /// Serialize [procfs_core::ProcError] - pub fn serialize_proc_error( - error: &procfs_core::ProcError, - serializer: S, - ) -> Result { - serialize_generic_error(error, serializer) - } - /// Serialize [std::string::FromUtf8Error] - pub fn serialize_from_utf8_error( - error: &std::string::FromUtf8Error, - serializer: S, - ) -> Result { - serialize_generic_error(error, serializer) - } - /// Serialize [std::time::SystemTimeError] - pub fn serialize_system_time_error( - error: &std::time::SystemTimeError, - serializer: S, - ) -> Result { - serialize_generic_error(error, serializer) - } - /// Serialize [scroll::Error] - pub fn serialize_scroll_error( - error: &scroll::Error, - serializer: S, - ) -> Result { - serialize_generic_error(error, serializer) - } -} - -#[cfg(test)] -mod test { - use super::*; - - #[derive(Debug)] - enum OuterError { - Outer, - Middle(SoftErrorList), - } - - #[derive(Debug)] - enum MiddleError { - Middle, - Inner(SoftErrorList), - } - - #[derive(Debug)] - enum InnerError { - Foobar, - } - - #[test] - fn basic() { - let mut soft_errors = SoftErrorList::default(); - soft_errors.push(OuterError::Outer); - middle(soft_errors.map_sublist(OuterError::Middle)); - soft_errors.push(OuterError::Outer); - - // Check outer - let mut outer_it = soft_errors.into_iter(); - assert!(matches!(outer_it.next(), Some(OuterError::Outer))); - let Some(OuterError::Middle(middle)) = outer_it.next() else { - panic!(); - }; - assert!(matches!(outer_it.next(), Some(OuterError::Outer))); - - // Check middle - let mut middle_it = middle.into_iter(); - assert!(matches!(middle_it.next(), Some(MiddleError::Middle))); - let Some(MiddleError::Inner(inner)) = middle_it.next() else { - panic!(); - }; - assert!(matches!(middle_it.next(), Some(MiddleError::Middle))); - - // Check inner - let mut inner_it = inner.into_iter(); - assert!(matches!(inner_it.next(), Some(InnerError::Foobar))); - } - - fn middle(mut soft_errors: SoftErrorSublist<'_, MiddleError>) { - soft_errors.push(MiddleError::Middle); - inner(soft_errors.map_sublist(MiddleError::Inner)); - soft_errors.push(MiddleError::Middle); - } - - fn inner(mut soft_errors: SoftErrorSublist<'_, InnerError>) { - soft_errors.push(InnerError::Foobar); - } -} diff --git a/src/fail_enabled/active.rs b/src/fail_enabled/active.rs deleted file mode 100644 index aece8278..00000000 --- a/src/fail_enabled/active.rs +++ /dev/null @@ -1,154 +0,0 @@ -use std::sync::{LazyLock, Mutex, MutexGuard}; - -/// Evaluates to the second argument if fail is enabled, otherwise the third argument. -#[macro_export] -macro_rules! if_fail_enabled_else(($n: ident, $enabled: expr, $disabled: expr $(,)?) => {{ - if $crate::fail_enabled::Config::get().fail_enabled(crate::fail_enabled::FailName::$n) { - $enabled - } else { - $disabled - } -}}); - -/// Executes the given statement if fail is enabled -#[macro_export] -macro_rules! if_fail_enabled(($n: ident, $e: expr $(,)?) => {{ - $crate::if_fail_enabled_else!($n, $e, ()); -}}); - -/// Returns the given error type (converted with into()) if fail is enabled -#[macro_export] -macro_rules! return_err_if_fail_enabled(($n: ident, $f: expr $(,)?) => {{ - crate::if_fail_enabled!($n, return Err($f.into())); -}}); - -/// Defines a set of flags that can be safely read and written from multiple threads -macro_rules! atomic_flags(($s: ident { - $($f: ident,)+ -}) => { - /// The names of the supported atomic flags - #[derive(Clone, Copy, Debug, PartialEq, Eq)] - pub enum $n { - $($f,)+ - } - - impl $n { - /// The total number of flags - pub const COUNT: usize = last_ident!($n, $($f),+) as usize + 1; - } - - /// An array of AtomicBool that holds the values for all the flags - #[derive(Debug, Default)] - pub struct $s([core::sync::atomic::AtomicBool; $n::COUNT]); - - impl $s { - /// Determine whether a flag is enabled - pub fn get(&self, flag: $n) -> bool { - self.0[flag as usize].load(core::sync::atomic::Ordering::Acquire) - } - /// Set whether a flag is enabled - pub fn set(&self, flag: $n, value: bool) { - self.0[flag as usize].store(value, core::sync::atomic::Ordering::Release) - } - /// Disable all flags - pub fn clear(&self) { - for flag in &self.0 { - flag.store(false, core::sync::atomic::Ordering::Release); - } - } - /// Test whether all flags are disabled - pub fn all_clear(&self) -> bool { - for flag in &self.0 { - let value = flag.load(core::sync::atomic::Ordering::Acquire); - if value { - return false; - } - } - true - } - } -}); - -/// Retrieves the last identifier passed to it -/// -/// Useful to determine the name of the last entry in an enum. -macro_rules! last_ident { - {$n: ident, $first: ident, $($tail: ident),+} => { - last_ident!($n, $($tail),+) - }; - {$n: ident, $last: ident} => { - $n::$last - }; -} - -atomic_flags!(FailEnabledFlags { - StopProcess, - FillMissingAuxvInfo, - ThreadName, - SuspendThreads, - CpuInfoFileOpen, -}); - -/// Configuration for the fail_enabled module -/// -/// Generally there will only be one of these that can be obtained via [Config::get()] -#[derive(Debug, Default)] -pub struct Config { - fail_enabled_flags: FailEnabledFlags, - client_mutex: Mutex<()>, -} - -impl Config { - /// Get a reference to the global object - pub fn get() -> &'static Config { - static INSTANCE: LazyLock = LazyLock::new(Config::default); - &INSTANCE - } - /// Return an exclusive client that a test can use to set its fail config - /// - /// As long as the returned object is held, no other test can mutate the fail enabled flags - pub fn client(&self) -> FailClient<'_> { - // We don't care if the lock gets poisoned, since this mutex isn't protecting any data - let _guard = match self.client_mutex.lock() { - Ok(guard) => guard, - Err(e) => e.into_inner(), - }; - assert!( - self.fail_enabled_flags.all_clear(), - "flags were not properly cleared by last client" - ); - FailClient { - config: self, - _guard, - } - } - /// Check to see if a given fail is enabled - /// - /// Used by all the macros in this module - pub(crate) fn fail_enabled(&self, fail: FailName) -> bool { - self.fail_enabled_flags.get(fail) - } -} - -/// An exclusive client that can change the fail flags -/// -/// It is protected by a mutex so that only one test may ever hold this at a time. -#[derive(Debug)] -pub struct FailClient<'a> { - config: &'a Config, - _guard: MutexGuard<'a, ()>, -} - -impl<'a> FailClient<'a> { - /// Change whether a fail is enabled or not - pub fn set_fail_enabled(&self, fail: FailName, enabled: bool) { - self.config.fail_enabled_flags.set(fail, enabled); - } -} - -/// Will disable all fails -impl<'a> Drop for FailClient<'a> { - fn drop(&mut self) { - self.config.fail_enabled_flags.clear(); - } -} diff --git a/src/fail_enabled/inactive.rs b/src/fail_enabled/inactive.rs deleted file mode 100644 index 69a2c5b1..00000000 --- a/src/fail_enabled/inactive.rs +++ /dev/null @@ -1,8 +0,0 @@ -#[macro_export] -macro_rules! if_fail_enabled_else(($n: ident, $enabled: expr, $disabled: expr $(,)?) => {$disabled}); - -#[macro_export] -macro_rules! if_fail_enabled(($n: ident, $e: expr $(,)?) => {}); - -#[macro_export] -macro_rules! return_err_if_fail_enabled(($n: ident, $f: expr $(,)?) => {}); diff --git a/src/fail_enabled/mod.rs b/src/fail_enabled/mod.rs deleted file mode 100644 index 613fea26..00000000 --- a/src/fail_enabled/mod.rs +++ /dev/null @@ -1,36 +0,0 @@ -//! Allows testing code to configure intentional failures at various codepaths -//! -//! It is often useful during testing to force certain codepaths to return an error even if they -//! would have otherwise succeeded. -//! -//! There are several macros that will be defined at the crate level that can be used by code that -//! should be tested: -//! -//! - `if_fail_enabled_else!(name, expr_if_enabled, expr_if_disabled)`: Evaluates to the first -//! expression if fail `name` is enabled, otherwise evaluates to the second expression. -//! -//! - `if_fail_enabled!(name, stmt)`: If fail `name` is enabled, will execute the given -//! statement. -//! -//! - `return_err_if_fail_enabled(name, err_expr)`: If fail `name` is enabled, will perform a -//! `return Err(err_expr.into())`. -//! -//! All of the above macros are no-op if the `fail-enabled` feature is not enabled. -//! -//! # Testing Code -//! -//! Testing code will generally use the global [Config] object via [Config::get]. Calling the -//! [client][Config::client] function will return a [FailClient] object. This object is -//! protected by a mutex so that only one such object can exist in the process at a time. This -//! is necessary because tests in the same source file run concurrently with each other. -//! -//! When the [FailClient] is dropped, all enabled fails will be disabled. This ensures the next -//! test will start with a fresh state. - -#[cfg(feature = "fail-enabled")] -mod active; -#[cfg(feature = "fail-enabled")] -pub use active::*; - -#[cfg(not(feature = "fail-enabled"))] -mod inactive; diff --git a/src/lib.rs b/src/lib.rs index fec5251b..67181b4a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,3 @@ -pub use error_list::SoftErrorList; - cfg_if::cfg_if! { if #[cfg(any(target_os = "linux", target_os = "android"))] { mod linux; @@ -16,12 +14,19 @@ cfg_if::cfg_if! { } } -pub mod minidump_cpu; -pub mod minidump_format; - pub mod dir_section; pub mod mem_writer; +pub mod minidump_cpu; +pub mod minidump_format; -pub mod fail_enabled; +mod serializers; -mod error_list; +failspot::failspot_name! { + pub enum FailSpotName { + StopProcess, + FillMissingAuxvInfo, + ThreadName, + SuspendThreads, + CpuInfoFileOpen, + } +} diff --git a/src/linux/auxv/mod.rs b/src/linux/auxv/mod.rs index 27a309b1..527fc45b 100644 --- a/src/linux/auxv/mod.rs +++ b/src/linux/auxv/mod.rs @@ -1,8 +1,8 @@ -use crate::error_list::{serializers::*, SoftErrorSublist}; -pub use reader::ProcfsAuxvIter; - use { - crate::Pid, + self::reader::ProcfsAuxvIter, + crate::{serializers::*, Pid}, + error_graph::WriteErrorList, + failspot::failspot, std::{fs::File, io::BufReader}, thiserror::Error, }; @@ -84,7 +84,7 @@ impl AuxvDumpInfo { pub fn try_filling_missing_info( &mut self, pid: Pid, - mut soft_errors: SoftErrorSublist<'_, AuxvError>, + mut soft_errors: impl WriteErrorList, ) -> Result<(), AuxvError> { if self.is_complete() { return Ok(()); @@ -113,10 +113,7 @@ impl AuxvDumpInfo { } } - crate::if_fail_enabled!( - FillMissingAuxvInfo, - soft_errors.push(AuxvError::InvalidFormat) - ); + failspot!(FillMissingAuxvInfo soft_errors.push(AuxvError::InvalidFormat)); Ok(()) } diff --git a/src/linux/dumper_cpu_info/x86_mips.rs b/src/linux/dumper_cpu_info/x86_mips.rs index 988d32d1..025a97be 100644 --- a/src/linux/dumper_cpu_info/x86_mips.rs +++ b/src/linux/dumper_cpu_info/x86_mips.rs @@ -1,7 +1,11 @@ -use crate::errors::CpuInfoError; -use crate::minidump_format::*; -use std::io::{BufRead, BufReader}; -use std::path; +use { + crate::{errors::CpuInfoError, minidump_format::*}, + failspot::failspot, + std::{ + io::{BufRead, BufReader}, + path, + }, +}; type Result = std::result::Result; @@ -44,9 +48,9 @@ pub fn write_cpu_information(sys_info: &mut MDRawSystemInfo) -> Result<()> { MDCPUArchitecture::PROCESSOR_ARCHITECTURE_AMD64 } as u16; - crate::return_err_if_fail_enabled!( - CpuInfoFileOpen, - std::io::Error::other("test requested cpuinfo file failure") + failspot!( + CpuInfoFileOpen + bail(std::io::Error::other("test requested cpuinfo file failure")) ); let cpuinfo_file = std::fs::File::open(path::PathBuf::from("/proc/cpuinfo"))?; diff --git a/src/linux/errors.rs b/src/linux/errors.rs index 367b2b4b..f909fcbd 100644 --- a/src/linux/errors.rs +++ b/src/linux/errors.rs @@ -1,15 +1,13 @@ -use crate::{ - dir_section::FileWriterError, - error_list::{serializers::*, SoftErrorList}, - maps_reader::MappingInfo, - mem_writer::MemoryWriterError, - Pid, +use { + super::ptrace_dumper::InitError, + crate::{ + dir_section::FileWriterError, maps_reader::MappingInfo, mem_writer::MemoryWriterError, + serializers::*, Pid, + }, + error_graph::ErrorList, + std::ffi::OsString, + thiserror::Error, }; -use goblin; -use std::ffi::OsString; -use thiserror::Error; - -use super::ptrace_dumper::InitError; #[derive(Error, Debug, serde::Serialize)] pub enum MapsReaderError { @@ -348,15 +346,15 @@ pub enum WriterError { std::time::SystemTimeError, ), #[error("Errors occurred while initializing PTraceDumper")] - InitErrors(#[source] SoftErrorList), + InitErrors(#[source] ErrorList), #[error("Errors occurred while suspending threads")] - SuspendThreadsErrors(#[source] SoftErrorList), + SuspendThreadsErrors(#[source] ErrorList), #[error("Errors occurred while resuming threads")] - ResumeThreadsErrors(#[source] SoftErrorList), + ResumeThreadsErrors(#[source] ErrorList), #[error("Crash thread does not reference principal mapping")] PrincipalMappingNotReferenced, #[error("Errors occurred while writing system info")] - WriteSystemInfoErrors(#[source] SoftErrorList), + WriteSystemInfoErrors(#[source] ErrorList), #[error("Failed writing cpuinfo")] WriteCpuInfoFailed(#[source] MemoryWriterError), #[error("Failed writing thread proc status")] diff --git a/src/linux/maps_reader.rs b/src/linux/maps_reader.rs index 6e8f79ef..3113d35f 100644 --- a/src/linux/maps_reader.rs +++ b/src/linux/maps_reader.rs @@ -1,11 +1,17 @@ -use crate::{auxv::AuxvType, errors::MapsReaderError}; -use byteorder::{NativeEndian, ReadBytesExt}; -use goblin::elf; -use memmap2::{Mmap, MmapOptions}; -use procfs_core::process::{MMPermissions, MMapPath, MemoryMaps}; -use std::ffi::{OsStr, OsString}; -use std::os::unix::ffi::{OsStrExt, OsStringExt}; -use std::{fs::File, mem::size_of, path::PathBuf}; +use { + crate::{auxv::AuxvType, errors::MapsReaderError}, + byteorder::{NativeEndian, ReadBytesExt}, + goblin::elf, + memmap2::{Mmap, MmapOptions}, + procfs_core::process::{MMPermissions, MMapPath, MemoryMaps}, + std::{ + ffi::{OsStr, OsString}, + fs::File, + mem::size_of, + os::unix::ffi::{OsStrExt, OsStringExt}, + path::PathBuf, + }, +}; pub const LINUX_GATE_LIBRARY_NAME: &str = "linux-gate.so"; pub const DELETED_SUFFIX: &[u8] = b" (deleted)"; @@ -87,7 +93,10 @@ impl MappingInfo { self.start_address + self.size } - pub fn aggregate(memory_maps: MemoryMaps, linux_gate_loc: AuxvType) -> Result> { + pub fn aggregate( + memory_maps: MemoryMaps, + linux_gate_loc: Option, + ) -> Result> { let mut infos = Vec::::new(); for mm in memory_maps { @@ -111,10 +120,11 @@ impl MappingInfo { let is_path = is_mapping_a_path(pathname.as_deref()); - if !is_path && linux_gate_loc != 0 && start_address == usize::try_from(linux_gate_loc)? - { - pathname = Some(LINUX_GATE_LIBRARY_NAME.into()); - offset = 0; + if let Some(linux_gate_loc) = linux_gate_loc.map(|u| usize::try_from(u).unwrap()) { + if !is_path && start_address == linux_gate_loc { + pathname = Some(LINUX_GATE_LIBRARY_NAME.into()); + offset = 0; + } } if let Some(prev_module) = infos.last_mut() { @@ -451,7 +461,7 @@ mod tests { fn get_mappings_for(map: &str, linux_gate_loc: u64) -> Vec { MappingInfo::aggregate( MemoryMaps::from_read(map.as_bytes()).expect("failed to read mapping info"), - linux_gate_loc, + Some(linux_gate_loc), ) .unwrap_or_default() } diff --git a/src/linux/minidump_writer.rs b/src/linux/minidump_writer.rs index 14a56fd1..03eb89c6 100644 --- a/src/linux/minidump_writer.rs +++ b/src/linux/minidump_writer.rs @@ -1,24 +1,26 @@ pub use crate::linux::auxv::{AuxvType, DirectAuxvDumpInfo}; -use crate::{ - auxv::AuxvDumpInfo, - dir_section::{DirSection, DumpBuf}, - error_list::SoftErrorList, - linux::{ - app_memory::AppMemoryList, - crash_context::CrashContext, - dso_debug, - errors::WriterError, - maps_reader::{MappingInfo, MappingList}, - ptrace_dumper::PtraceDumper, - sections::*, +use { + crate::{ + auxv::AuxvDumpInfo, + dir_section::{DirSection, DumpBuf}, + linux::{ + app_memory::AppMemoryList, + crash_context::CrashContext, + dso_debug, + errors::WriterError, + maps_reader::{MappingInfo, MappingList}, + ptrace_dumper::PtraceDumper, + sections::*, + }, + mem_writer::{Buffer, MemoryArrayWriter, MemoryWriter, MemoryWriterError}, + minidump_format::*, + Pid, + }, + error_graph::{ErrorList, WriteErrorList}, + std::{ + io::{Seek, Write}, + time::Duration, }, - mem_writer::{Buffer, MemoryArrayWriter, MemoryWriter, MemoryWriterError}, - minidump_format::*, - Pid, -}; -use std::{ - io::{Seek, Write}, - time::Duration, }; pub enum CrashingThreadContext { @@ -146,18 +148,18 @@ impl MinidumpWriter { .map(AuxvDumpInfo::from) .unwrap_or_default(); - let mut soft_errors = SoftErrorList::default(); + let mut soft_errors = ErrorList::default(); let mut dumper = PtraceDumper::new_report_soft_errors( self.process_id, self.stop_timeout, auxv, - soft_errors.map_sublist(WriterError::InitErrors), + soft_errors.subwriter(WriterError::InitErrors), )?; let threads_count = dumper.threads.len(); - dumper.suspend_threads(soft_errors.map_sublist(WriterError::SuspendThreadsErrors)); + dumper.suspend_threads(soft_errors.subwriter(WriterError::SuspendThreadsErrors)); if dumper.threads.is_empty() { // TBH I'm not sure this even needs to be a hard error. Is a minidump without any @@ -181,8 +183,7 @@ impl MinidumpWriter { self.generate_dump(&mut buffer, &mut dumper, soft_errors, destination)?; // TODO - Record these errors? Or maybe we don't care? - let mut soft_errors = SoftErrorList::default(); - dumper.resume_threads(soft_errors.map_sublist(WriterError::ResumeThreadsErrors)); + dumper.resume_threads(error_graph::strategy::DontCare); Ok(buffer.into()) } @@ -245,7 +246,7 @@ impl MinidumpWriter { &mut self, buffer: &mut DumpBuf, dumper: &mut PtraceDumper, - mut soft_errors: SoftErrorList, + mut soft_errors: ErrorList, destination: &mut (impl Write + Seek), ) -> Result<()> { // A minidump file contains a number of tagged streams. This is the number @@ -292,7 +293,7 @@ impl MinidumpWriter { let dirent = systeminfo_stream::write( buffer, - soft_errors.map_sublist(WriterError::WriteSystemInfoErrors), + soft_errors.subwriter(WriterError::WriteSystemInfoErrors), )?; dir_section.write_to_file(buffer, Some(dirent))?; @@ -452,12 +453,10 @@ impl MinidumpWriter { fn write_soft_errors( buffer: &mut DumpBuf, - soft_errors: SoftErrorList, + soft_errors: ErrorList, ) -> Result { - let soft_errors_json = soft_errors - .to_json() - .map_err(WriterError::ConvertToJsonFailed)?; - let soft_errors_json_str = format!("{soft_errors_json:#}"); + let soft_errors_json_str = + serde_json::to_string_pretty(&soft_errors).map_err(WriterError::ConvertToJsonFailed)?; let section = MemoryArrayWriter::write_bytes(buffer, soft_errors_json_str.as_bytes()); Ok(section.location()) } diff --git a/src/linux/ptrace_dumper.rs b/src/linux/ptrace_dumper.rs index c63adb5d..d17f6ef2 100644 --- a/src/linux/ptrace_dumper.rs +++ b/src/linux/ptrace_dumper.rs @@ -1,37 +1,42 @@ +use { + super::{ + auxv::AuxvError, + errors::{AndroidError, MapsReaderError}, + }, + crate::{ + linux::{ + auxv::AuxvDumpInfo, + errors::{DumperError, ThreadInfoError}, + maps_reader::MappingInfo, + module_reader, + thread_info::ThreadInfo, + Pid, + }, + serializers::*, + }, + error_graph::{ErrorList, WriteErrorList}, + failspot::failspot, + nix::{ + errno::Errno, + sys::{ptrace, signal, wait}, + }, + procfs_core::{ + process::{MMPermissions, ProcState, Stat}, + FromRead, ProcError, + }, + std::{ + ffi::OsString, + path, + result::Result, + time::{Duration, Instant}, + }, + thiserror::Error, +}; + #[cfg(target_os = "android")] use crate::linux::android::late_process_mappings; #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] use crate::thread_info; -use crate::{ - error_list::{serializers::*, SoftErrorList, SoftErrorSublist}, - linux::{ - auxv::AuxvDumpInfo, - errors::{DumperError, ThreadInfoError}, - maps_reader::MappingInfo, - module_reader, - thread_info::ThreadInfo, - Pid, - }, -}; -use nix::{ - errno::Errno, - sys::{ptrace, signal, wait}, -}; -use procfs_core::{ - process::{MMPermissions, ProcState, Stat}, - FromRead, ProcError, -}; -use std::{ - ffi::OsString, - path, - result::Result, - time::{Duration, Instant}, -}; - -use super::{ - auxv::AuxvError, - errors::{AndroidError, MapsReaderError}, -}; #[derive(Debug, Clone)] pub struct Thread { @@ -57,13 +62,13 @@ pub const AT_SYSINFO_EHDR: u64 = 33; impl Drop for PtraceDumper { fn drop(&mut self) { // Always try to resume all threads (e.g. in case of error) - self.resume_threads(SoftErrorList::null_sublist()); + self.resume_threads(error_graph::strategy::DontCare); // Always allow the process to continue. let _ = self.continue_process(); } } -#[derive(Debug, thiserror::Error, serde::Serialize)] +#[derive(Debug, Error, serde::Serialize)] pub enum InitError { #[error("failed to read auxv")] ReadAuxvFailed(#[source] crate::auxv::AuxvError), @@ -87,7 +92,7 @@ pub enum InitError { #[error("Failed to stop the target process")] StopProcessFailed(#[source] StopProcessError), #[error("Errors occurred while filling missing Auxv info")] - FillMissingAuxvInfoErrors(#[source] SoftErrorList), + FillMissingAuxvInfoErrors(#[source] ErrorList), #[error("Failed filling missing Auxv info")] FillMissingAuxvInfoFailed(#[source] AuxvError), #[error("Failed reading proc/pid/task entry for process")] @@ -107,7 +112,7 @@ pub enum InitError { #[error("Proc task directory `{0:?}` is not a directory")] ProcPidTaskNotDirectory(String), #[error("Errors while enumerating threads")] - EnumerateThreadsErrors(#[source] SoftErrorList), + EnumerateThreadsErrors(#[source] ErrorList), #[error("Failed to enumerate threads")] EnumerateThreadsFailed(#[source] Box), #[error("Failed to read process map file")] @@ -168,7 +173,7 @@ impl PtraceDumper { pid: Pid, stop_timeout: Duration, auxv: AuxvDumpInfo, - soft_errors: SoftErrorSublist<'_, InitError>, + soft_errors: impl WriteErrorList, ) -> Result { if pid == std::process::id() as i32 { return Err(InitError::CannotPtraceSameProcess); @@ -190,7 +195,7 @@ impl PtraceDumper { pub fn init( &mut self, stop_timeout: Duration, - mut soft_errors: SoftErrorSublist<'_, InitError>, + mut soft_errors: impl WriteErrorList, ) -> Result<(), InitError> { // Stopping the process is best-effort. if let Err(e) = self.stop_process(stop_timeout) { @@ -201,7 +206,7 @@ impl PtraceDumper { // forward. if let Err(e) = self.auxv.try_filling_missing_info( self.pid, - soft_errors.map_sublist(InitError::FillMissingAuxvInfoErrors), + soft_errors.subwriter(InitError::FillMissingAuxvInfoErrors), ) { soft_errors.push(InitError::FillMissingAuxvInfoFailed(e)); } @@ -209,7 +214,7 @@ impl PtraceDumper { // If we completely fail to enumerate any threads... Some information is still better than // no information! if let Err(e) = - self.enumerate_threads(soft_errors.map_sublist(InitError::EnumerateThreadsErrors)) + self.enumerate_threads(soft_errors.subwriter(InitError::EnumerateThreadsErrors)) { soft_errors.push(InitError::EnumerateThreadsFailed(Box::new(e))); } @@ -308,7 +313,7 @@ impl PtraceDumper { ptrace_detach(child) } - pub fn suspend_threads(&mut self, mut soft_errors: SoftErrorSublist<'_, DumperError>) { + pub fn suspend_threads(&mut self, mut soft_errors: impl WriteErrorList) { // Iterate over all threads and try to suspend them. // If the thread either disappeared before we could attach to it, or if // it was part of the seccomp sandbox's trusted code, it is OK to @@ -323,13 +328,10 @@ impl PtraceDumper { self.threads_suspended = true; - crate::if_fail_enabled!( - SuspendThreads, - soft_errors.push(DumperError::PtraceAttachError(1234, nix::Error::EPERM)) - ); + failspot::failspot!(::SuspendThreads soft_errors.push(DumperError::PtraceAttachError(1234, nix::Error::EPERM))) } - pub fn resume_threads(&mut self, mut soft_errors: SoftErrorSublist<'_, DumperError>) { + pub fn resume_threads(&mut self, mut soft_errors: impl WriteErrorList) { if self.threads_suspended { for thread in &self.threads { match Self::resume_thread(thread.tid) { @@ -347,7 +349,7 @@ impl PtraceDumper { /// /// This will block waiting for the process to stop until `timeout` has passed. fn stop_process(&mut self, timeout: Duration) -> Result<(), StopProcessError> { - crate::return_err_if_fail_enabled!(StopProcess, nix::Error::EPERM); + failspot!(StopProcess bail(nix::Error::EPERM)); signal::kill(nix::unistd::Pid::from_raw(self.pid), Some(signal::SIGSTOP))?; @@ -381,7 +383,7 @@ impl PtraceDumper { /// pid. fn enumerate_threads( &mut self, - mut soft_errors: SoftErrorSublist<'_, InitError>, + mut soft_errors: impl WriteErrorList, ) -> Result<(), InitError> { let pid = self.pid; let filename = format!("/proc/{}/task", pid); @@ -408,13 +410,13 @@ impl PtraceDumper { }; // Read the thread-name (if there is any) - let name_result = crate::if_fail_enabled_else!( - ThreadName, + let name_result = failspot!(if ThreadName { Err(std::io::Error::other( - "testing requested failure reading thread name" - )), + "testing requested failure reading thread name", + )) + } else { std::fs::read_to_string(format!("/proc/{}/task/{}/comm", pid, tid)) - ); + }); let name = match name_result { Ok(name) => Some(name.trim_end().to_string()), @@ -438,38 +440,34 @@ impl PtraceDumper { // case its entry when creating the list of mappings. // See http://www.trilithium.com/johan/2005/08/linux-gate/ for more // information. - let linux_gate_loc = self.auxv.get_linux_gate_address().unwrap_or_default(); let maps_path = format!("/proc/{}/maps", self.pid); let maps_file = std::fs::File::open(&maps_path).map_err(|e| InitError::IOError(maps_path, e))?; - use procfs_core::FromRead; let maps = procfs_core::process::MemoryMaps::from_read(maps_file) .map_err(InitError::ReadProcessMapFileFailed)?; - self.mappings = MappingInfo::aggregate(maps, linux_gate_loc) + self.mappings = MappingInfo::aggregate(maps, self.auxv.get_linux_gate_address()) .map_err(InitError::AggregateMappingsFailed)?; // Although the initial executable is usually the first mapping, it's not // guaranteed (see http://crosbug.com/25355); therefore, try to use the // actual entry point to find the mapping. - if let Some(entry_point_loc) = self.auxv.get_entry_address() { - let mut swap_idx = None; - for (idx, module) in self.mappings.iter().enumerate() { - // If this module contains the entry-point, and it's not already the first - // one, then we need to make it be first. This is because the minidump - // format assumes the first module is the one that corresponds to the main - // executable (as codified in - // processor/minidump.cc:MinidumpModuleList::GetMainModule()). - if entry_point_loc >= module.start_address.try_into().unwrap() - && entry_point_loc < (module.start_address + module.size).try_into().unwrap() - { - swap_idx = Some(idx); - break; - } - } - if let Some(idx) = swap_idx { - self.mappings.swap(0, idx); + if let Some(entry_point_loc) = self + .auxv + .get_entry_address() + .map(|u| usize::try_from(u).unwrap()) + { + // If this module contains the entry-point, and it's not already the first + // one, then we need to make it be first. This is because the minidump + // format assumes the first module is the one that corresponds to the main + // executable (as codified in + // processor/minidump.cc:MinidumpModuleList::GetMainModule()). + if let Some(entry_mapping_idx) = self.mappings.iter().position(|mapping| { + (mapping.start_address..mapping.start_address + mapping.size) + .contains(&entry_point_loc) + }) { + self.mappings.swap(0, entry_mapping_idx); } } Ok(()) diff --git a/src/linux/sections/systeminfo_stream.rs b/src/linux/sections/systeminfo_stream.rs index f8881b41..62ca332c 100644 --- a/src/linux/sections/systeminfo_stream.rs +++ b/src/linux/sections/systeminfo_stream.rs @@ -1,10 +1,11 @@ -use super::*; -use crate::{error_list::*, linux::dumper_cpu_info as dci}; -use errors::SectionSystemInfoError; +use { + super::*, crate::linux::dumper_cpu_info as dci, error_graph::WriteErrorList, + errors::SectionSystemInfoError, +}; pub fn write( buffer: &mut DumpBuf, - mut soft_errors: SoftErrorSublist<'_, SectionSystemInfoError>, + mut soft_errors: impl WriteErrorList, ) -> Result { let mut info_section = MemoryWriter::::alloc(buffer)?; let dirent = MDRawDirectory { diff --git a/src/mem_writer.rs b/src/mem_writer.rs index ed68daca..ae38c79b 100644 --- a/src/mem_writer.rs +++ b/src/mem_writer.rs @@ -1,7 +1,7 @@ use { crate::{ - error_list::serializers::*, minidump_format::{MDLocationDescriptor, MDRVA}, + serializers::*, }, scroll::ctx::{SizeWith, TryIntoCtx}, }; diff --git a/src/serializers.rs b/src/serializers.rs new file mode 100644 index 00000000..94615235 --- /dev/null +++ b/src/serializers.rs @@ -0,0 +1,65 @@ +//! Functions used by Serde to serialize types that we don't own (and thus can't implement +//! [Serialize] for) + +use serde::Serializer; +/// Useful for types that implement [Error][std::error::Error] and don't need any special +/// treatment. +fn serialize_generic_error( + error: &E, + serializer: S, +) -> Result { + // I guess we'll have to see if it's more useful to store the debug representation of a + // foreign error type or something else (like maybe iterating its error chain into a + // list?) + let dbg = format!("{error:#?}"); + serializer.serialize_str(&dbg) +} +/// Serialize [std::io::Error] +pub fn serialize_io_error( + error: &std::io::Error, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} +/// Serialize [goblin::error::Error] +pub fn serialize_goblin_error( + error: &goblin::error::Error, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} +/// Serialize [nix::Error] +pub fn serialize_nix_error( + error: &nix::Error, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} +/// Serialize [procfs_core::ProcError] +pub fn serialize_proc_error( + error: &procfs_core::ProcError, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} +/// Serialize [std::string::FromUtf8Error] +pub fn serialize_from_utf8_error( + error: &std::string::FromUtf8Error, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} +/// Serialize [std::time::SystemTimeError] +pub fn serialize_system_time_error( + error: &std::time::SystemTimeError, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} +/// Serialize [scroll::Error] +pub fn serialize_scroll_error( + error: &scroll::Error, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} diff --git a/tests/common/mod.rs b/tests/common/mod.rs index cf258819..68926400 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -94,3 +94,19 @@ pub fn start_child_and_return(args: &[&str]) -> Child { .spawn() .expect("failed to execute child") } + +#[allow(unused)] +pub fn read_minidump_soft_errors_or_panic<'a, T>( + dump: &minidump::Minidump<'a, T>, +) -> serde_json::Value +where + T: std::ops::Deref + 'a, +{ + let contents = std::str::from_utf8( + dump.get_raw_stream(minidump_common::format::MINIDUMP_STREAM_TYPE::MozSoftErrors.into()) + .expect("missing soft error stream"), + ) + .expect("expected utf-8 stream"); + + serde_json::from_str(contents).expect("expected json") +} diff --git a/tests/linux_minidump_writer.rs b/tests/linux_minidump_writer.rs index d99d788a..f889140f 100644 --- a/tests/linux_minidump_writer.rs +++ b/tests/linux_minidump_writer.rs @@ -330,6 +330,14 @@ contextual_test! { let status = waitres.signal().expect("Child did not die due to signal"); assert_eq!(waitres.code(), None); assert_eq!(status, Signal::SIGKILL as i32); + + // Ensure the minidump has a MozSoftErrors present + let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); + let soft_error_json = read_minidump_soft_errors_or_panic(&dump); + + assert_eq!(soft_error_json, serde_json::json! { + ["PrincipalMappingNotReferenced"] + }); } } diff --git a/tests/linux_minidump_writer_soft_error.rs b/tests/linux_minidump_writer_soft_error.rs index 326a959a..62944128 100644 --- a/tests/linux_minidump_writer_soft_error.rs +++ b/tests/linux_minidump_writer_soft_error.rs @@ -1,14 +1,10 @@ #![cfg(any(target_os = "linux", target_os = "android"))] -#![cfg(feature = "fail-enabled")] use { common::*, minidump::Minidump, - minidump_writer::{ - fail_enabled::{self, FailName}, - minidump_writer::MinidumpWriter, - }, - serde_json::{json, Value as JsonValue}, + minidump_writer::{minidump_writer::MinidumpWriter, FailSpotName}, + serde_json::json, }; mod common; @@ -23,9 +19,8 @@ fn soft_error_stream() { .tempfile() .unwrap(); - let fail_config = fail_enabled::Config::get(); - let fail_client = fail_config.client(); - fail_client.set_fail_enabled(FailName::StopProcess, true); + let mut fail_client = FailSpotName::testing_client(); + fail_client.set_enabled(FailSpotName::StopProcess, true); // Write a minidump MinidumpWriter::new(pid, pid) @@ -33,10 +28,9 @@ fn soft_error_stream() { .expect("cound not write minidump"); child.kill().expect("Failed to kill process"); - // Ensure the minidump has a MemoryInfoListStream present and has at least one entry. + // Ensure the minidump has a MozSoftErrors present let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); - dump.get_raw_stream(minidump_common::format::MINIDUMP_STREAM_TYPE::MozSoftErrors.into()) - .expect("missing soft error stream"); + read_minidump_soft_errors_or_panic(&dump); } #[test] @@ -73,16 +67,15 @@ fn soft_error_stream_content() { .tempfile() .unwrap(); - let fail_config = fail_enabled::Config::get(); - let fail_client = fail_config.client(); + let mut fail_client = FailSpotName::testing_client(); for name in [ - FailName::StopProcess, - FailName::FillMissingAuxvInfo, - FailName::ThreadName, - FailName::SuspendThreads, - FailName::CpuInfoFileOpen, + FailSpotName::StopProcess, + FailSpotName::FillMissingAuxvInfo, + FailSpotName::ThreadName, + FailSpotName::SuspendThreads, + FailSpotName::CpuInfoFileOpen, ] { - fail_client.set_fail_enabled(name, true); + fail_client.set_enabled(name, true); } // Write a minidump @@ -91,15 +84,9 @@ fn soft_error_stream_content() { .expect("cound not write minidump"); child.kill().expect("Failed to kill process"); - // Ensure the minidump has a MemoryInfoListStream present and has at least one entry. + // Ensure the MozSoftErrors stream matches the expected JSON let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); - let contents = std::str::from_utf8( - dump.get_raw_stream(minidump_common::format::MINIDUMP_STREAM_TYPE::MozSoftErrors.into()) - .expect("missing soft error stream"), - ) - .expect("expected utf-8 stream"); - - let actual_json: JsonValue = serde_json::from_str(contents).expect("expected json"); + let actual_json = read_minidump_soft_errors_or_panic(&dump); if actual_json != expected_json { panic!( diff --git a/tests/ptrace_dumper.rs b/tests/ptrace_dumper.rs index 616559e8..b166c256 100644 --- a/tests/ptrace_dumper.rs +++ b/tests/ptrace_dumper.rs @@ -1,13 +1,20 @@ //! All of these tests are specific to ptrace #![cfg(any(target_os = "linux", target_os = "android"))] -use minidump_writer::{ptrace_dumper::PtraceDumper, SoftErrorList}; -use nix::sys::mman::{mmap, MapFlags, ProtFlags}; -use nix::sys::signal::Signal; -use std::convert::TryInto; -use std::io::{BufRead, BufReader}; -use std::mem::size_of; -use std::os::unix::process::ExitStatusExt; +use { + error_graph::ErrorList, + minidump_writer::ptrace_dumper::PtraceDumper, + nix::{ + sys::mman::{mmap, MapFlags, ProtFlags}, + sys::signal::Signal, + }, + std::{ + convert::TryInto, + io::{BufRead, BufReader}, + mem::size_of, + os::unix::process::ExitStatusExt, + }, +}; mod common; use common::*; @@ -23,6 +30,13 @@ macro_rules! disabled_on_ci_and_android { }; } +macro_rules! assert_no_soft_errors(($n: ident, $e: expr) => {{ + let mut $n = ErrorList::default(); + let __result = $e; + assert!($n.is_empty(), "{:?}", $n); + __result +}}); + #[test] fn test_setup() { spawn_child("setup", &[]); @@ -91,18 +105,21 @@ fn test_thread_list_from_parent() { let num_of_threads = 5; let mut child = start_child_and_wait_for_threads(num_of_threads); let pid = child.id() as i32; - let mut dumper = PtraceDumper::new_report_soft_errors( - pid, - minidump_writer::minidump_writer::STOP_TIMEOUT, - Default::default(), - SoftErrorList::null_sublist(), + + let mut dumper = assert_no_soft_errors!( + soft_errors, + PtraceDumper::new_report_soft_errors( + pid, + minidump_writer::minidump_writer::STOP_TIMEOUT, + Default::default(), + &mut soft_errors, + ) ) .expect("Couldn't init dumper"); + assert_eq!(dumper.threads.len(), num_of_threads); - let mut soft_errors = SoftErrorList::default(); - dumper.suspend_threads(soft_errors.inserted_sublist()); - assert!(soft_errors.is_empty(), "failed to suspend all threads"); + assert_no_soft_errors!(soft_errors, dumper.suspend_threads(&mut soft_errors)); // let mut matching_threads = 0; for (idx, curr_thread) in dumper.threads.iter().enumerate() { @@ -150,9 +167,7 @@ fn test_thread_list_from_parent() { 0 }; */ } - let mut soft_errors = SoftErrorList::default(); - dumper.resume_threads(soft_errors.inserted_sublist()); - assert!(soft_errors.is_empty(), "Failed to resume threads"); + assert_no_soft_errors!(soft_errors, dumper.resume_threads(&mut soft_errors)); child.kill().expect("Failed to kill process"); // Reap child @@ -278,18 +293,20 @@ fn test_sanitize_stack_copy() { let heap_addr = usize::from_str_radix(output.next().unwrap().trim_start_matches("0x"), 16) .expect("unable to parse mmap_addr"); - let mut dumper = PtraceDumper::new_report_soft_errors( - pid, - minidump_writer::minidump_writer::STOP_TIMEOUT, - Default::default(), - SoftErrorList::null_sublist(), + let mut dumper = assert_no_soft_errors!( + soft_errors, + PtraceDumper::new_report_soft_errors( + pid, + minidump_writer::minidump_writer::STOP_TIMEOUT, + Default::default(), + &mut soft_errors, + ) ) .expect("Couldn't init dumper"); assert_eq!(dumper.threads.len(), num_of_threads); - let mut soft_errors = SoftErrorList::default(); - dumper.suspend_threads(soft_errors.inserted_sublist()); - assert!(soft_errors.is_empty(), "failed to suspend all threads"); + assert_no_soft_errors!(soft_errors, dumper.suspend_threads(&mut soft_errors)); + let thread_info = dumper .get_thread_info_by_index(0) .expect("Couldn't find thread_info"); @@ -384,9 +401,8 @@ fn test_sanitize_stack_copy() { assert_eq!(simulated_stack[0..size_of::()], defaced); - let mut soft_errors = SoftErrorList::default(); - dumper.resume_threads(soft_errors.inserted_sublist()); - assert!(soft_errors.is_empty(), "Failed to resume threads"); + assert_no_soft_errors!(soft_errors, dumper.resume_threads(&mut soft_errors)); + child.kill().expect("Failed to kill process"); // Reap child From fcc36a730cdad78f7856ec0a2e552e0e5081ae5e Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Fri, 13 Dec 2024 17:43:34 -0500 Subject: [PATCH 4/6] Address feedback and realize that 17 + 1 = 18 --- src/linux/minidump_writer.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/linux/minidump_writer.rs b/src/linux/minidump_writer.rs index 03eb89c6..0c6e385a 100644 --- a/src/linux/minidump_writer.rs +++ b/src/linux/minidump_writer.rs @@ -162,9 +162,7 @@ impl MinidumpWriter { dumper.suspend_threads(soft_errors.subwriter(WriterError::SuspendThreadsErrors)); if dumper.threads.is_empty() { - // TBH I'm not sure this even needs to be a hard error. Is a minidump without any - // thread info still at-least a little useful? - return Err(WriterError::SuspendNoThreadsLeft(threads_count)); + soft_errors.push(WriterError::SuspendNoThreadsLeft(threads_count)); } dumper.late_init()?; @@ -182,9 +180,6 @@ impl MinidumpWriter { let mut buffer = Buffer::with_capacity(0); self.generate_dump(&mut buffer, &mut dumper, soft_errors, destination)?; - // TODO - Record these errors? Or maybe we don't care? - dumper.resume_threads(error_graph::strategy::DontCare); - Ok(buffer.into()) } @@ -251,7 +246,7 @@ impl MinidumpWriter { ) -> Result<()> { // A minidump file contains a number of tagged streams. This is the number // of streams which we write. - let num_writers = 19u32; + let num_writers = 18u32; let mut header_section = MemoryWriter::::alloc(buffer)?; @@ -425,6 +420,16 @@ impl MinidumpWriter { }; dir_section.write_to_file(buffer, Some(dirent))?; + // ======================================================================================== + // + // PAST THIS BANNER, THE THREADS ARE RUNNING IN THE TARGET PROCESS AGAIN. IF YOU NEED TO + // ADD NEW ENTRIES THAT ACCESS THE TARGET MEMORY, DO IT BEFORE HERE! + // + // ======================================================================================== + + // Collect any last-minute soft errors when trying to restart threads + dumper.resume_threads(soft_errors.subwriter(WriterError::ResumeThreadsErrors)); + // If this fails, there's really nothing we can do about that (other than ignore it). let dirent = write_soft_errors(buffer, soft_errors) .map(|location| MDRawDirectory { From 00c007bbed9070cead12f2e9682143f50de07208 Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Wed, 8 Jan 2025 13:34:26 -0500 Subject: [PATCH 5/6] Update minidump-common to 0.24 from crates.io --- .cargo/config.toml | 3 - Cargo.lock | 171 +++++++++++++++------- Cargo.toml | 8 +- src/linux.rs | 1 + src/linux/errors.rs | 2 +- src/linux/mem_reader.rs | 6 +- src/linux/module_reader.rs | 8 +- src/linux/ptrace_dumper.rs | 1 + src/linux/serializers.rs | 40 +++++ src/serializers.rs | 37 +---- tests/common/mod.rs | 19 +++ tests/linux_minidump_writer.rs | 57 ++++---- tests/linux_minidump_writer_soft_error.rs | 36 ++--- 13 files changed, 232 insertions(+), 157 deletions(-) create mode 100644 src/linux/serializers.rs diff --git a/.cargo/config.toml b/.cargo/config.toml index 28a9d026..e32d5cfd 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -14,6 +14,3 @@ linker = "x86_64-linux-android30-clang" # By default the linker _doesn't_ generate a build-id, however we want one for our tests. rustflags = ["-C", "link-args=-Wl,--build-id=sha1"] runner = [".cargo/android-runner.sh"] - -[patch.crates-io] -minidump-common = { path = "../rust-minidump/minidump-common" } \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index 52b45645..6d596d66 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 +version = 4 [[package]] name = "addr2line" @@ -36,7 +36,7 @@ dependencies = [ "cfg-if", "once_cell", "version_check", - "zerocopy", + "zerocopy 0.7.35", ] [[package]] @@ -175,9 +175,9 @@ dependencies = [ [[package]] name = "breakpad-symbols" -version = "0.22.0" +version = "0.24.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7e1ad3f5e2e5c8a42fccedd6792cc05968b39b69c3fe7b5544072ac052f3fe85" +checksum = "05cc04995b4f6f26dc9cc5989e93e42c373def047b4b057aaf8f48400b971d1e" dependencies = [ "async-trait", "cachemap2", @@ -187,7 +187,7 @@ dependencies = [ "minidump-common", "nom", "range-map", - "thiserror", + "thiserror 1.0.63", "tracing", ] @@ -678,15 +678,15 @@ dependencies = [ [[package]] name = "framehop" -version = "0.12.1" +version = "0.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0fd28d2036d4fd99e3629487baca659e5af1c5d554e320168613be79028610fc" +checksum = "59bdf491caf8284489b65c99366d0f88393709b8214e4ccff2f55d9892da7713" dependencies = [ "arrayvec", "cfg-if", "fallible-iterator 0.3.0", - "gimli 0.30.0", - "macho-unwind-info", + "gimli 0.31.0", + "macho-unwind-info 0.5.0", "pe-unwind-info", ] @@ -1094,7 +1094,7 @@ dependencies = [ "memchr", "prost", "prost-derive", - "thiserror", + "thiserror 1.0.63", ] [[package]] @@ -1106,7 +1106,7 @@ dependencies = [ "bitflags 2.6.0", "byteorder", "memchr", - "thiserror", + "thiserror 1.0.63", ] [[package]] @@ -1168,9 +1168,20 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6b6086acc74bc23f56b60e88bb082d505e23849d68d6c0f12bb6a7ad5c60e03e" dependencies = [ - "thiserror", - "zerocopy", - "zerocopy-derive", + "thiserror 1.0.63", + "zerocopy 0.7.35", + "zerocopy-derive 0.7.35", +] + +[[package]] +name = "macho-unwind-info" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bb4bdc8b0ce69932332cf76d24af69c3a155242af95c226b2ab6c2e371ed1149" +dependencies = [ + "thiserror 2.0.10", + "zerocopy 0.8.14", + "zerocopy-derive 0.8.14", ] [[package]] @@ -1211,19 +1222,19 @@ checksum = "6877bb514081ee2a7ff5ef9de3281f14a4dd4bceac4c09388074a6b5df8a139a" [[package]] name = "minidump" -version = "0.22.0" +version = "0.24.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "aefb80650628de087057ed167e3e1ef5bed65dc4b1bd28d47cd707c3848adce2" +checksum = "e03e301d414a75655d4ce80e6e3690fbfe70814b67c496c64c826ba558d18ec9" dependencies = [ "debugid", "encoding_rs", "memmap2", "minidump-common", "num-traits", - "procfs-core", + "procfs-core 0.17.0", "range-map", "scroll 0.12.0", - "thiserror", + "thiserror 1.0.63", "time", "tracing", "uuid", @@ -1231,7 +1242,9 @@ dependencies = [ [[package]] name = "minidump-common" -version = "0.22.0" +version = "0.24.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5273687f49325b3977f7d372a1bbe2e528694d18128de8dcac78d134448e83b4" dependencies = [ "bitflags 2.6.0", "debugid", @@ -1244,9 +1257,9 @@ dependencies = [ [[package]] name = "minidump-processor" -version = "0.22.0" +version = "0.24.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4d330a92d90c5699e8edd32f8036a1b5afadd6df000eb201fac258d149f8ca78" +checksum = "18a08b6c01056348ec77a4b5aa537ddbbf217c028496319cd54029b812ccf3fb" dependencies = [ "async-trait", "breakpad-symbols", @@ -1258,15 +1271,15 @@ dependencies = [ "scroll 0.12.0", "serde", "serde_json", - "thiserror", + "thiserror 1.0.63", "tracing", ] [[package]] name = "minidump-unwind" -version = "0.22.0" +version = "0.24.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "afb5af4cbb631c54fe8c0c058799e9ac95b31c6e282f1afaaaaad10c2c441fcb" +checksum = "c30454f5703c77433b4059bf5e196266b800b14223c55793ee636e49c8f9160e" dependencies = [ "async-trait", "breakpad-symbols", @@ -1306,13 +1319,13 @@ dependencies = [ "minidump-processor", "minidump-unwind", "nix", - "procfs-core", + "procfs-core 0.16.0", "scroll 0.12.0", "serde", "serde_json", "similar-asserts", "tempfile", - "thiserror", + "thiserror 1.0.63", "uuid", ] @@ -1497,7 +1510,7 @@ dependencies = [ "maybe-owned", "pdb", "range-collections 0.2.4", - "thiserror", + "thiserror 1.0.63", ] [[package]] @@ -1511,7 +1524,7 @@ dependencies = [ "maybe-owned", "pdb2", "range-collections 0.4.5", - "thiserror", + "thiserror 1.0.63", ] [[package]] @@ -1527,15 +1540,15 @@ dependencies = [ [[package]] name = "pe-unwind-info" -version = "0.2.3" +version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6ec3b43050c38ffb9de87e17d874e9956e3a9131b343c9b7b7002597727c3891" +checksum = "11fe3d7d11dde0fd142bf734ae5d645a4c62ede7c188bccc73dec5082359ff84" dependencies = [ "arrayvec", "bitflags 2.6.0", - "thiserror", - "zerocopy", - "zerocopy-derive", + "thiserror 1.0.63", + "zerocopy 0.7.35", + "zerocopy-derive 0.7.35", ] [[package]] @@ -1609,7 +1622,7 @@ version = "0.2.20" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "77957b295656769bb8ad2b6a6b09d897d94f05c41b069aede1fcdaa675eaea04" dependencies = [ - "zerocopy", + "zerocopy 0.7.35", ] [[package]] @@ -1620,9 +1633,9 @@ checksum = "925383efa346730478fb4838dbe9137d2a47675ad789c546d150a6e1dd4ab31c" [[package]] name = "proc-macro2" -version = "1.0.86" +version = "1.0.92" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5e719e8df665df0d1c8fbfd238015744736151d4445ec0836b8e628aae103b77" +checksum = "37d3544b3f2748c54e147655edb5025752e2303145b5aefb3c3ea2c78b973bb0" dependencies = [ "unicode-ident", ] @@ -1638,6 +1651,16 @@ dependencies = [ "serde", ] +[[package]] +name = "procfs-core" +version = "0.17.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "239df02d8349b06fc07398a3a1697b06418223b1c7725085e801e7c0fc6a12ec" +dependencies = [ + "bitflags 2.6.0", + "hex", +] + [[package]] name = "prost" version = "0.12.6" @@ -1673,7 +1696,7 @@ dependencies = [ "rustc-hash", "rustls", "socket2", - "thiserror", + "thiserror 1.0.63", "tokio", "tracing", ] @@ -1690,7 +1713,7 @@ dependencies = [ "rustc-hash", "rustls", "slab", - "thiserror", + "thiserror 1.0.63", "tinyvec", "tracing", ] @@ -1802,7 +1825,7 @@ checksum = "bd283d9651eeda4b2a83a43c1c91b266c40fd76ecd39a50a8c630ae69dc72891" dependencies = [ "getrandom", "libredox", - "thiserror", + "thiserror 1.0.63", ] [[package]] @@ -2011,7 +2034,7 @@ dependencies = [ "gimli 0.30.0", "linux-perf-data", "lzma-rs", - "macho-unwind-info", + "macho-unwind-info 0.4.0", "memchr", "msvc-demangler", "nom", @@ -2021,12 +2044,12 @@ dependencies = [ "rustc-demangle", "scala-native-demangle", "srcsrv", - "thiserror", + "thiserror 1.0.63", "uuid", "yoke", "yoke-derive", - "zerocopy", - "zerocopy-derive", + "zerocopy 0.7.35", + "zerocopy-derive 0.7.35", ] [[package]] @@ -2215,7 +2238,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "022437a70e522e49b1952cb1d923589d629cb4aee97eb56d65ce938c04e8ac70" dependencies = [ "memchr", - "thiserror", + "thiserror 1.0.63", ] [[package]] @@ -2270,7 +2293,7 @@ checksum = "e3b4273aee7b62c172f1723eb06dda7462f951760a79524734fb1da4cf3842a2" dependencies = [ "symbolic-common", "symbolic-debuginfo", - "thiserror", + "thiserror 1.0.63", ] [[package]] @@ -2312,7 +2335,7 @@ dependencies = [ "smallvec", "symbolic-common", "symbolic-ppdb", - "thiserror", + "thiserror 1.0.63", "wasmparser", "zip", "zstd", @@ -2342,7 +2365,7 @@ dependencies = [ "serde", "serde_json", "symbolic-common", - "thiserror", + "thiserror 1.0.63", "uuid", "watto", ] @@ -2361,15 +2384,15 @@ dependencies = [ "http", "reqwest", "scopeguard", - "thiserror", + "thiserror 1.0.63", "tokio", ] [[package]] name = "syn" -version = "2.0.74" +version = "2.0.95" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1fceb41e3d546d0bd83421d3409b1460cc7444cd389341a4c880fe7a042cb3d7" +checksum = "46f71c0377baf4ef1cc3e3402ded576dccc315800fbc62dfc7fe04b009773b4a" dependencies = [ "proc-macro2", "quote", @@ -2411,7 +2434,16 @@ version = "1.0.63" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c0342370b38b6a11b6cc11d6a805569958d54cfa061a29969c3b5ce2ea405724" dependencies = [ - "thiserror-impl", + "thiserror-impl 1.0.63", +] + +[[package]] +name = "thiserror" +version = "2.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a3ac7f54ca534db81081ef1c1e7f6ea8a3ef428d2fc069097c079443d24124d3" +dependencies = [ + "thiserror-impl 2.0.10", ] [[package]] @@ -2425,6 +2457,17 @@ dependencies = [ "syn", ] +[[package]] +name = "thiserror-impl" +version = "2.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9e9465d30713b56a37ede7185763c3492a91be2f5fa68d958c44e41ab9248beb" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "time" version = "0.3.36" @@ -2763,7 +2806,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6746b5315e417144282a047ebb82260d45c92d09bf653fa9ec975e3809be942b" dependencies = [ "leb128", - "thiserror", + "thiserror 1.0.63", ] [[package]] @@ -2986,7 +3029,16 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1b9b4fd18abc82b8136838da5d50bae7bdea537c574d8dc1a34ed098d6c166f0" dependencies = [ "byteorder", - "zerocopy-derive", + "zerocopy-derive 0.7.35", +] + +[[package]] +name = "zerocopy" +version = "0.8.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a367f292d93d4eab890745e75a778da40909cab4d6ff8173693812f79c4a2468" +dependencies = [ + "zerocopy-derive 0.8.14", ] [[package]] @@ -3000,6 +3052,17 @@ dependencies = [ "syn", ] +[[package]] +name = "zerocopy-derive" +version = "0.8.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3931cb58c62c13adec22e38686b559c86a30565e16ad6e8510a337cedc611e1" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "zerofrom" version = "0.1.4" @@ -3025,7 +3088,7 @@ dependencies = [ "flate2", "indexmap", "memchr", - "thiserror", + "thiserror 1.0.63", "zopfli", ] diff --git a/Cargo.toml b/Cargo.toml index 4128cb28..5a959c31 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,7 +17,7 @@ error-graph = { version = "0.1.1", features = ["serde"] } failspot = "0.2.0" log = "0.4" memoffset = "0.9" -minidump-common = "0.22" +minidump-common = "0.24" scroll = "0.12" serde = { version = "1.0.208", features = ["derive"] } serde_json = "1.0.116" @@ -56,14 +56,14 @@ current_platform = "0.2" failspot = { version = "0.2.0", features = ["enabled"] } # Minidump-processor is async so we need an executor futures = { version = "0.3", features = ["executor"] } -minidump = "0.22" +minidump = "0.24" memmap2 = "0.9" [target.'cfg(target_os = "macos")'.dev-dependencies] # We dump symbols for the `test` executable so that we can validate that minidumps # created by this crate can be processed by minidump-processor dump_syms = { version = "2.2", default-features = false } -minidump-processor = { version = "0.22", default-features = false } -minidump-unwind = { version = "0.22", features = ["debuginfo"] } +minidump-processor = { version = "0.24", default-features = false } +minidump-unwind = { version = "0.24", features = ["debuginfo"] } similar-asserts = "1.5" uuid = "1.4" diff --git a/src/linux.rs b/src/linux.rs index febdeabe..dc605084 100644 --- a/src/linux.rs +++ b/src/linux.rs @@ -15,6 +15,7 @@ pub mod minidump_writer; pub mod module_reader; pub mod ptrace_dumper; pub(crate) mod sections; +mod serializers; pub mod thread_info; pub use maps_reader::LINUX_GATE_LIBRARY_NAME; diff --git a/src/linux/errors.rs b/src/linux/errors.rs index f909fcbd..95322f10 100644 --- a/src/linux/errors.rs +++ b/src/linux/errors.rs @@ -1,5 +1,5 @@ use { - super::ptrace_dumper::InitError, + super::{ptrace_dumper::InitError, serializers::*}, crate::{ dir_section::FileWriterError, maps_reader::MappingInfo, mem_writer::MemoryWriterError, serializers::*, Pid, diff --git a/src/linux/mem_reader.rs b/src/linux/mem_reader.rs index e4a50411..9d04285d 100644 --- a/src/linux/mem_reader.rs +++ b/src/linux/mem_reader.rs @@ -258,7 +258,7 @@ impl PtraceDumper { src: usize, length: usize, ) -> Result, crate::errors::DumperError> { - let length = std::num::NonZeroUsize::new(length).ok_or_else(|| { + let length = std::num::NonZeroUsize::new(length).ok_or( crate::errors::DumperError::CopyFromProcessError(CopyFromProcessError { src, child: pid, @@ -268,8 +268,8 @@ impl PtraceDumper { // as EINVAL could also come from the syscalls that actually read // memory as well which could be confusing source: nix::errno::Errno::EINVAL, - }) - })?; + }), + )?; let mut mem = MemReader::new(pid); Ok(mem.read_to_vec(src, length)?) diff --git a/src/linux/module_reader.rs b/src/linux/module_reader.rs index f40e28cc..ff4c6aec 100644 --- a/src/linux/module_reader.rs +++ b/src/linux/module_reader.rs @@ -36,7 +36,7 @@ impl<'buf> From<&'buf [u8]> for ProcessMemory<'buf> { } } -impl<'buf> From for ProcessMemory<'buf> { +impl From for ProcessMemory<'_> { fn from(value: ProcessReader) -> Self { Self::Process(value) } @@ -207,7 +207,7 @@ impl<'a> DynIter<'a> { } } -impl<'a> Iterator for DynIter<'a> { +impl Iterator for DynIter<'_> { type Item = Result; fn next(&mut self) -> Option { @@ -427,9 +427,9 @@ impl<'buf> ModuleReader<'buf> { let name = self .module_memory .read(strtab_offset + name_offset, strtab_size - name_offset)?; - return CStr::from_bytes_until_nul(&name) + CStr::from_bytes_until_nul(&name) .map(|s| s.to_string_lossy().into_owned()) - .map_err(|_| Error::StrTabNoNulByte); + .map_err(|_| Error::StrTabNoNulByte) } fn section_offset(&self, header: &elf::SectionHeader) -> u64 { diff --git a/src/linux/ptrace_dumper.rs b/src/linux/ptrace_dumper.rs index d17f6ef2..7b04a2f5 100644 --- a/src/linux/ptrace_dumper.rs +++ b/src/linux/ptrace_dumper.rs @@ -2,6 +2,7 @@ use { super::{ auxv::AuxvError, errors::{AndroidError, MapsReaderError}, + serializers::*, }, crate::{ linux::{ diff --git a/src/linux/serializers.rs b/src/linux/serializers.rs new file mode 100644 index 00000000..73b33e2e --- /dev/null +++ b/src/linux/serializers.rs @@ -0,0 +1,40 @@ +//! Functions used by Serde to serialize types that we don't own (and thus can't implement +//! [Serialize] for) + +use {crate::serializers::*, serde::Serializer}; + +/// Serialize [goblin::error::Error] +pub fn serialize_goblin_error( + error: &goblin::error::Error, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} +/// Serialize [nix::Error] +pub fn serialize_nix_error( + error: &nix::Error, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} +/// Serialize [procfs_core::ProcError] +pub fn serialize_proc_error( + error: &procfs_core::ProcError, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} +/// Serialize [std::string::FromUtf8Error] +pub fn serialize_from_utf8_error( + error: &std::string::FromUtf8Error, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} +/// Serialize [std::time::SystemTimeError] +pub fn serialize_system_time_error( + error: &std::time::SystemTimeError, + serializer: S, +) -> Result { + serialize_generic_error(error, serializer) +} diff --git a/src/serializers.rs b/src/serializers.rs index 94615235..a42313da 100644 --- a/src/serializers.rs +++ b/src/serializers.rs @@ -4,7 +4,7 @@ use serde::Serializer; /// Useful for types that implement [Error][std::error::Error] and don't need any special /// treatment. -fn serialize_generic_error( +pub fn serialize_generic_error( error: &E, serializer: S, ) -> Result { @@ -21,41 +21,6 @@ pub fn serialize_io_error( ) -> Result { serialize_generic_error(error, serializer) } -/// Serialize [goblin::error::Error] -pub fn serialize_goblin_error( - error: &goblin::error::Error, - serializer: S, -) -> Result { - serialize_generic_error(error, serializer) -} -/// Serialize [nix::Error] -pub fn serialize_nix_error( - error: &nix::Error, - serializer: S, -) -> Result { - serialize_generic_error(error, serializer) -} -/// Serialize [procfs_core::ProcError] -pub fn serialize_proc_error( - error: &procfs_core::ProcError, - serializer: S, -) -> Result { - serialize_generic_error(error, serializer) -} -/// Serialize [std::string::FromUtf8Error] -pub fn serialize_from_utf8_error( - error: &std::string::FromUtf8Error, - serializer: S, -) -> Result { - serialize_generic_error(error, serializer) -} -/// Serialize [std::time::SystemTimeError] -pub fn serialize_system_time_error( - error: &std::time::SystemTimeError, - serializer: S, -) -> Result { - serialize_generic_error(error, serializer) -} /// Serialize [scroll::Error] pub fn serialize_scroll_error( error: &scroll::Error, diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 68926400..2b5499ef 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -110,3 +110,22 @@ where serde_json::from_str(contents).expect("expected json") } + +#[allow(unused)] +pub fn assert_soft_errors_in_minidump<'a, 'b, T, I>( + dump: &minidump::Minidump<'a, T>, + expected_errors: I, +) where + T: std::ops::Deref + 'a, + I: IntoIterator, +{ + let actual_json = read_minidump_soft_errors_or_panic(dump); + let actual_errors = actual_json.as_array().unwrap(); + + // Ensure that every error we expect is in the actual list somewhere + for expected_error in expected_errors { + assert!(actual_errors + .iter() + .any(|actual_error| actual_error == expected_error)); + } +} diff --git a/tests/linux_minidump_writer.rs b/tests/linux_minidump_writer.rs index f889140f..d62f56ff 100644 --- a/tests/linux_minidump_writer.rs +++ b/tests/linux_minidump_writer.rs @@ -1,30 +1,32 @@ #![cfg(any(target_os = "linux", target_os = "android"))] #![allow(unused_imports, unused_variables)] -use minidump::*; -use minidump_common::format::{GUID, MINIDUMP_STREAM_TYPE::*}; -use minidump_writer::{ - app_memory::AppMemory, - crash_context::CrashContext, - errors::*, - maps_reader::{MappingEntry, MappingInfo, SystemMappingInfo}, - minidump_writer::MinidumpWriter, - module_reader::{BuildId, ReadFromModule}, - ptrace_dumper::PtraceDumper, - Pid, -}; -use nix::{errno::Errno, sys::signal::Signal}; -use procfs_core::process::MMPermissions; -use std::collections::HashSet; - -use std::{ - io::{BufRead, BufReader}, - os::unix::process::ExitStatusExt, - process::{Command, Stdio}, +use { + common::*, + minidump::*, + minidump_common::format::{GUID, MINIDUMP_STREAM_TYPE::*}, + minidump_writer::{ + app_memory::AppMemory, + crash_context::CrashContext, + errors::*, + maps_reader::{MappingEntry, MappingInfo, SystemMappingInfo}, + minidump_writer::MinidumpWriter, + module_reader::{BuildId, ReadFromModule}, + ptrace_dumper::PtraceDumper, + Pid, + }, + nix::{errno::Errno, sys::signal::Signal}, + procfs_core::process::MMPermissions, + serde_json::json, + std::{ + collections::HashSet, + io::{BufRead, BufReader}, + os::unix::process::ExitStatusExt, + process::{Command, Stdio}, + }, }; mod common; -use common::*; #[derive(Debug, PartialEq)] enum Context { @@ -299,6 +301,10 @@ contextual_test! { contextual_test! { fn skip_if_requested(context: Context) { + let expected_errors = vec![ + json!("PrincipalMappingNotReferenced"), + ]; + let num_of_threads = 1; let mut child = start_child_and_wait_for_threads(num_of_threads); let pid = child.id() as i32; @@ -331,13 +337,9 @@ contextual_test! { assert_eq!(waitres.code(), None); assert_eq!(status, Signal::SIGKILL as i32); - // Ensure the minidump has a MozSoftErrors present + // Ensure the MozSoftErrors stream contains the expected errors let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); - let soft_error_json = read_minidump_soft_errors_or_panic(&dump); - - assert_eq!(soft_error_json, serde_json::json! { - ["PrincipalMappingNotReferenced"] - }); + assert_soft_errors_in_minidump(&dump, &expected_errors); } } @@ -796,6 +798,7 @@ fn memory_info_list_stream() { .dump(&mut tmpfile) .expect("cound not write minidump"); child.kill().expect("Failed to kill process"); + child.wait().expect("Failed to wait on killed process"); // Ensure the minidump has a MemoryInfoListStream present and has at least one entry. let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); diff --git a/tests/linux_minidump_writer_soft_error.rs b/tests/linux_minidump_writer_soft_error.rs index 62944128..5c5df212 100644 --- a/tests/linux_minidump_writer_soft_error.rs +++ b/tests/linux_minidump_writer_soft_error.rs @@ -27,6 +27,7 @@ fn soft_error_stream() { .dump(&mut tmpfile) .expect("cound not write minidump"); child.kill().expect("Failed to kill process"); + child.wait().expect("Failed to wait on killed process"); // Ensure the minidump has a MozSoftErrors present let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); @@ -35,8 +36,8 @@ fn soft_error_stream() { #[test] fn soft_error_stream_content() { - let expected_json = json!([ - {"InitErrors": [ + let expected_errors = vec![ + json!({"InitErrors": [ {"StopProcessFailed": {"Stop": "EPERM"}}, {"FillMissingAuxvInfoErrors": ["InvalidFormat"]}, {"EnumerateThreadsErrors": [ @@ -47,17 +48,17 @@ fn soft_error_stream_content() { }" } ]} - ]}, - {"SuspendThreadsErrors": [{"PtraceAttachError": [1234, "EPERM"]}]}, - {"WriteSystemInfoErrors": [ + ]}), + json!({"SuspendThreadsErrors": [{"PtraceAttachError": [1234, "EPERM"]}]}), + json!({"WriteSystemInfoErrors": [ {"WriteCpuInformationFailed": {"IOError": "\ Custom {\n \ kind: Other,\n \ error: \"test requested cpuinfo file failure\",\n\ }" }} - ]} - ]); + ]}), + ]; let mut child = start_child_and_wait_for_threads(1); let pid = child.id() as i32; @@ -83,24 +84,9 @@ fn soft_error_stream_content() { .dump(&mut tmpfile) .expect("cound not write minidump"); child.kill().expect("Failed to kill process"); + child.wait().expect("Failed to wait on killed process"); - // Ensure the MozSoftErrors stream matches the expected JSON + // Ensure the MozSoftErrors stream contains the expected errors let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); - let actual_json = read_minidump_soft_errors_or_panic(&dump); - - if actual_json != expected_json { - panic!( - "\ - JSON mismatch:\n\ - =====Expected=====\n\ - \n\ - {expected_json:#}\n\ - \n\ - =====Actual=====\n\ - \n\ - {actual_json:#}\n\ - \n\ - " - ); - } + assert_soft_errors_in_minidump(&dump, &expected_errors); } From f470076f5d903b26dc6a0919fad148d00b67af5c Mon Sep 17 00:00:00 2001 From: Chris Martin Date: Mon, 13 Jan 2025 12:00:24 -0500 Subject: [PATCH 6/6] Appease clippy, eliminate dupe dependency --- Cargo.lock | 16 +++------------- Cargo.toml | 2 +- src/bin/test.rs | 2 +- 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8ca44d11..04757f8f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1362,7 +1362,7 @@ dependencies = [ "memmap2", "minidump-common", "num-traits", - "procfs-core 0.17.0", + "procfs-core", "range-map", "scroll 0.12.0", "thiserror 1.0.69", @@ -1429,7 +1429,7 @@ dependencies = [ "minidump-common", "minidump-unwind", "nix", - "procfs-core 0.16.0", + "procfs-core", "scroll 0.12.0", "serde", "serde_json", @@ -1729,17 +1729,6 @@ dependencies = [ "unicode-ident", ] -[[package]] -name = "procfs-core" -version = "0.16.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2d3554923a69f4ce04c4a754260c338f505ce22642d3830e049a399fc2059a29" -dependencies = [ - "bitflags 2.6.0", - "hex", - "serde", -] - [[package]] name = "procfs-core" version = "0.17.0" @@ -1748,6 +1737,7 @@ checksum = "239df02d8349b06fc07398a3a1697b06418223b1c7725085e801e7c0fc6a12ec" dependencies = [ "bitflags 2.6.0", "hex", + "serde", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index a71d89d0..e87d1caa 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,7 +40,7 @@ nix = { version = "0.29", default-features = false, features = [ ] } # Used for parsing procfs info. # default-features is disabled since it pulls in chrono -procfs-core = { version = "0.16", default-features = false, features = ["serde1"] } +procfs-core = { version = "0.17", default-features = false, features = ["serde1"] } [target.'cfg(target_os = "windows")'.dependencies] bitflags = "2.4" diff --git a/src/bin/test.rs b/src/bin/test.rs index a2a43e34..9ab42f13 100644 --- a/src/bin/test.rs +++ b/src/bin/test.rs @@ -228,7 +228,7 @@ mod linux { if map .name .as_ref() - .map_or(false, |name| name.to_string_lossy().starts_with(&path)) + .is_some_and(|name| name.to_string_lossy().starts_with(&path)) { mapping_count += 1; // This mapping should encompass the entire original mapped