diff --git a/src/lib.rs b/src/lib.rs index d20f1ed3..9337b4cb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9,6 +9,9 @@ cfg_if::cfg_if! { ThreadName, SuspendThreads, CpuInfoFileOpen, + StackPointerMapping, + ThreadStackCopy, + CrashingThreadIpCopy, } } diff --git a/src/linux/minidump_writer/errors.rs b/src/linux/minidump_writer/errors.rs index 722703ea..0b8f49bc 100644 --- a/src/linux/minidump_writer/errors.rs +++ b/src/linux/minidump_writer/errors.rs @@ -69,6 +69,8 @@ pub enum WriterError { ResumeThreadsErrors(#[source] ErrorList), #[error("Errors occurred while writing system info")] WriteSystemInfoErrors(#[source] ErrorList), + #[error("Errors occurred while writing the thread list")] + WriteThreadListErrors(#[source] ErrorList), #[error("Failed writing cpuinfo")] WriteCpuInfoFailed(#[source] MemoryWriterError), #[error("Failed writing thread proc status")] diff --git a/src/linux/minidump_writer/mod.rs b/src/linux/minidump_writer/mod.rs index e66ede2a..56a5a2bd 100644 --- a/src/linux/minidump_writer/mod.rs +++ b/src/linux/minidump_writer/mod.rs @@ -345,7 +345,10 @@ impl MinidumpWriter { // we should have a mostly-intact dump dir_section.write_to_file(buffer, None)?; - let dirent = self.write_thread_list_stream(buffer)?; + let dirent = self.write_thread_list_stream( + buffer, + soft_errors.subwriter(WriterError::WriteThreadListErrors), + )?; dir_section.write_to_file(buffer, Some(dirent))?; let dirent = self.write_mappings(buffer)?; diff --git a/src/linux/minidump_writer/thread_list_stream.rs b/src/linux/minidump_writer/thread_list_stream.rs index f0160af9..e764e3a0 100644 --- a/src/linux/minidump_writer/thread_list_stream.rs +++ b/src/linux/minidump_writer/thread_list_stream.rs @@ -1,4 +1,7 @@ -use {super::*, crate::minidump_cpu::RawContextCPU, std::cmp::min}; +use { + super::*, crate::minidump_cpu::RawContextCPU, error_graph::WriteErrorList, failspot::failspot, + std::cmp::min, +}; // The following kLimit* constants are for when minidump_size_limit_ is set // and the minidump size might exceed it. @@ -34,6 +37,8 @@ pub enum SectionThreadListError { ), #[error("Failed to copy memory from process")] CopyFromProcessError(#[source] CopyFromProcessError), + #[error("Failed to locate the stack pointer's memory mapping")] + GetStackInfoFailed(#[source] Box), #[error("Failed to get thread info")] ThreadInfoError(#[from] ThreadInfoError), #[error("Failed to write to memory buffer")] @@ -50,6 +55,7 @@ impl MinidumpWriter { pub fn write_thread_list_stream( &mut self, buffer: &mut DumpBuf, + mut soft_errors: impl WriteErrorList, ) -> Result { let num_threads = self.threads.len(); // Memory looks like this: @@ -107,6 +113,7 @@ impl MinidumpWriter { instruction_ptr, stack_pointer, MaxStackLen::None, + &mut soft_errors, )?; // Copy 256 bytes around crashing instruction pointer to minidump. let ip_memory_size: usize = 256; @@ -114,6 +121,12 @@ impl MinidumpWriter { // it's contained within. If it's not in mapped memory, // don't bother trying to write it. for mapping in &self.mappings { + let instruction_ptr = failspot!(if CrashingThreadIpCopy { + // Just use the first mapping since we'll error out anyway + mapping.start_address + mapping.size / 2 + } else { + instruction_ptr + }); if instruction_ptr < mapping.start_address || instruction_ptr >= mapping.start_address + mapping.size { @@ -136,12 +149,22 @@ impl MinidumpWriter { ip_memory_d.memory.data_size = (end_of_range - ip_memory_d.start_of_memory_range) as u32; - let memory_copy = MinidumpWriter::copy_from_process( - &self.process_inspector, - ip_memory_d.start_of_memory_range as _, - ip_memory_d.memory.data_size as usize, - ) - .map_err(SectionThreadListError::CopyFromProcessError)?; + let ip_copy = failspot!(if CrashingThreadIpCopy { + Err(CopyFromProcessError::InvalidArgument) + } else { + MinidumpWriter::copy_from_process( + &self.process_inspector, + ip_memory_d.start_of_memory_range as _, + ip_memory_d.memory.data_size as usize, + ) + }); + let memory_copy = match ip_copy { + Ok(x) => x, + Err(e) => { + soft_errors.push(SectionThreadListError::CopyFromProcessError(e)); + break; + } + }; let mem_section = MemoryArrayWriter::alloc_from_array(buffer, &memory_copy)?; ip_memory_d.memory = mem_section.location(); @@ -172,6 +195,7 @@ impl MinidumpWriter { instruction_ptr, info.stack_pointer, max_stack_len, + &mut soft_errors, )?; let mut cpu = RawContextCPU::default(); @@ -199,54 +223,72 @@ impl MinidumpWriter { instruction_ptr: usize, stack_ptr: usize, max_stack_len: MaxStackLen, + mut soft_errors: impl WriteErrorList, ) -> Result<(), SectionThreadListError> { thread.stack.start_of_memory_range = stack_ptr.try_into()?; thread.stack.memory.data_size = 0; thread.stack.memory.rva = buffer.position() as u32; - if let Ok((valid_stack_ptr, stack_len)) = self.get_stack_info(stack_ptr) { - let stack_len = if let MaxStackLen::Len(max_stack_len) = max_stack_len { - min(stack_len, max_stack_len) - } else { - stack_len - }; + let stack_info = failspot!(if StackPointerMapping { + Err(WriterError::NoStackPointerMapping) + } else { + self.get_stack_info(stack_ptr) + }); + let (valid_stack_ptr, stack_len) = match stack_info { + Ok(x) => x, + Err(e) => { + soft_errors.push(SectionThreadListError::GetStackInfoFailed(Box::new(e))); + return Ok(()); + } + }; - let mut stack_bytes = MinidumpWriter::copy_from_process( - &self.process_inspector, - valid_stack_ptr, - stack_len, - ) - .map_err(SectionThreadListError::CopyFromProcessError)?; - let stack_pointer_offset = stack_ptr.saturating_sub(valid_stack_ptr); - if self.skip_stacks_if_mapping_unreferenced { - if let Some(principal_mapping) = &self.principal_mapping { - let low_addr = principal_mapping.system_mapping_info.start_address; - let high_addr = principal_mapping.system_mapping_info.end_address; - if (instruction_ptr < low_addr || instruction_ptr > high_addr) - && !principal_mapping - .stack_has_pointer_to_mapping(&stack_bytes, stack_pointer_offset) - { - return Ok(()); - } - } else { + let stack_len = if let MaxStackLen::Len(max_stack_len) = max_stack_len { + min(stack_len, max_stack_len) + } else { + stack_len + }; + + let stack_copy = failspot!(if ThreadStackCopy { + Err(CopyFromProcessError::InvalidArgument) + } else { + MinidumpWriter::copy_from_process(&self.process_inspector, valid_stack_ptr, stack_len) + }); + let mut stack_bytes = match stack_copy { + Ok(x) => x, + Err(e) => { + soft_errors.push(SectionThreadListError::CopyFromProcessError(e)); + return Ok(()); + } + }; + let stack_pointer_offset = stack_ptr.saturating_sub(valid_stack_ptr); + if self.skip_stacks_if_mapping_unreferenced { + if let Some(principal_mapping) = &self.principal_mapping { + let low_addr = principal_mapping.system_mapping_info.start_address; + let high_addr = principal_mapping.system_mapping_info.end_address; + if (instruction_ptr < low_addr || instruction_ptr > high_addr) + && !principal_mapping + .stack_has_pointer_to_mapping(&stack_bytes, stack_pointer_offset) + { return Ok(()); } + } else { + return Ok(()); } + } - if self.sanitize_stack { - self.sanitize_stack_copy(&mut stack_bytes, stack_ptr, stack_pointer_offset) - .map_err(|e| SectionThreadListError::SanitizeStackCopyFailed(Box::new(e)))?; - } - - let stack_location = MDLocationDescriptor { - data_size: stack_bytes.len() as u32, - rva: buffer.position() as u32, - }; - buffer.write_all(&stack_bytes); - thread.stack.start_of_memory_range = valid_stack_ptr as u64; - thread.stack.memory = stack_location; - self.memory_blocks.push(thread.stack); + if self.sanitize_stack { + self.sanitize_stack_copy(&mut stack_bytes, stack_ptr, stack_pointer_offset) + .map_err(|e| SectionThreadListError::SanitizeStackCopyFailed(Box::new(e)))?; } + + let stack_location = MDLocationDescriptor { + data_size: stack_bytes.len() as u32, + rva: buffer.position() as u32, + }; + buffer.write_all(&stack_bytes); + thread.stack.start_of_memory_range = valid_stack_ptr as u64; + thread.stack.memory = stack_location; + self.memory_blocks.push(thread.stack); Ok(()) } } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index bf6368b1..1bfa4cd0 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -144,6 +144,34 @@ pub fn assert_soft_errors_in_minidump<'a, 'b, T, I>( } } +/// Asserts that the soft-error stream contains an error named `variant` anywhere +/// in its (possibly nested) structure. Data-carrying variants appear as object +/// keys and unit variants as strings, so both forms are matched. +#[allow(unused)] +pub fn assert_minidump_contains_soft_error<'a, T>(dump: &minidump::Minidump<'a, T>, variant: &str) +where + T: std::ops::Deref + 'a, +{ + let errors = read_minidump_soft_errors_or_panic(dump); + assert!( + soft_errors_contain(&errors, variant), + "soft error list missing expected error `{variant}`\nError_list: {errors:#?}" + ); +} + +fn soft_errors_contain(value: &serde_json::Value, variant: &str) -> bool { + match value { + serde_json::Value::String(s) => s == variant, + serde_json::Value::Array(items) => { + items.iter().any(|item| soft_errors_contain(item, variant)) + } + serde_json::Value::Object(map) => { + map.contains_key(variant) || map.values().any(|v| soft_errors_contain(v, variant)) + } + _ => false, + } +} + #[cfg(any(target_os = "linux", target_os = "android"))] #[allow(unused)] pub use linux::*; @@ -152,7 +180,10 @@ pub use linux::*; #[allow(unused)] mod linux { use { - minidump_writer::module_reader::{self, ModuleMemoryReadError, ReadModuleMemory}, + minidump_writer::{ + CrashContextExt, Pid, + module_reader::{self, ModuleMemoryReadError, ReadModuleMemory}, + }, std::borrow::Cow, }; pub struct SliceModuleMemoryReader<'a>(pub &'a [u8]); @@ -192,4 +223,21 @@ mod linux { false } } + + pub fn get_dummy_crash_context(tid: Pid) -> CrashContextExt { + let siginfo: libc::signalfd_siginfo = unsafe { std::mem::zeroed() }; + let context = unsafe { std::mem::zeroed() }; + #[cfg(not(target_arch = "arm"))] + let float_state = unsafe { std::mem::zeroed() }; + CrashContextExt { + inner: crash_context::CrashContext { + siginfo, + pid: std::process::id() as _, + tid, + context, + #[cfg(not(target_arch = "arm"))] + float_state, + }, + } + } } diff --git a/tests/linux_minidump_writer.rs b/tests/linux_minidump_writer.rs index 576c3a42..4ce892b5 100644 --- a/tests/linux_minidump_writer.rs +++ b/tests/linux_minidump_writer.rs @@ -34,41 +34,13 @@ impl Context { pub fn minidump_writer(&self, pid: Pid) -> MinidumpWriterConfig { let mut mw = MinidumpWriterConfig::new(pid, pid); if self == &Context::With { - let crash_context = get_crash_context(pid); + let crash_context = get_dummy_crash_context(pid); mw.set_crash_context(crash_context); } mw } } -fn get_ucontext() -> Result { - let mut context = std::mem::MaybeUninit::uninit(); - unsafe { - let res = crash_context::crash_context_getcontext(context.as_mut_ptr()); - if res == -1 { - Err(std::io::Error::last_os_error())?; - } - Ok(context.assume_init()) - } -} - -fn get_crash_context(tid: Pid) -> CrashContextExt { - let siginfo: libc::signalfd_siginfo = unsafe { std::mem::zeroed() }; - let context = get_ucontext().expect("Failed to get ucontext"); - #[cfg(not(target_arch = "arm"))] - let float_state = unsafe { std::mem::zeroed() }; - CrashContextExt { - inner: crash_context::CrashContext { - siginfo, - pid: std::process::id() as _, - tid, - context, - #[cfg(not(target_arch = "arm"))] - float_state, - }, - } -} - macro_rules! contextual_test { ( $(#[$attr:meta])? fn $name:ident ($ctx:ident : Context) $body:block ) => { mod $name { diff --git a/tests/linux_minidump_writer_soft_error.rs b/tests/linux_minidump_writer_soft_error.rs index 0b2bd1a0..a0881726 100644 --- a/tests/linux_minidump_writer_soft_error.rs +++ b/tests/linux_minidump_writer_soft_error.rs @@ -101,3 +101,74 @@ fn soft_error_stream_content() { let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); assert_soft_errors_in_minidump(&dump, &expected_errors); } + +#[test] +fn thread_stack_pointer_unmapped_soft_error() { + let mut child = start_child_and_wait_for_threads(1); + let pid = child.id() as i32; + + let mut tmpfile = tempfile::Builder::new() + .prefix("stack_pointer_unmapped") + .tempfile() + .unwrap(); + + let mut fail_client = FailSpotName::testing_client(); + fail_client.set_enabled(FailSpotName::StackPointerMapping, true); + + MinidumpWriterConfig::new(pid, pid) + .write(&mut tmpfile) + .expect("could not write minidump"); + child.kill().expect("Failed to kill process"); + child.wait().expect("Failed to wait on killed process"); + + let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); + assert_minidump_contains_soft_error(&dump, "GetStackInfoFailed"); +} + +#[test] +fn thread_stack_unreadable_soft_error() { + let mut child = start_child_and_wait_for_threads(1); + let pid = child.id() as i32; + + let mut tmpfile = tempfile::Builder::new() + .prefix("stack_unreadable") + .tempfile() + .unwrap(); + + let mut fail_client = FailSpotName::testing_client(); + fail_client.set_enabled(FailSpotName::ThreadStackCopy, true); + + MinidumpWriterConfig::new(pid, pid) + .write(&mut tmpfile) + .expect("could not write minidump"); + child.kill().expect("Failed to kill process"); + child.wait().expect("Failed to wait on killed process"); + + let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); + assert_minidump_contains_soft_error(&dump, "CopyFromProcessError"); +} + +#[test] +fn crashing_thread_ip_memory_unreadable_soft_error() { + let mut child = start_child_and_wait_for_threads(1); + let pid = child.id() as i32; + + let mut tmpfile = tempfile::Builder::new() + .prefix("ip_memory_unreadable") + .tempfile() + .unwrap(); + + let mut fail_client = FailSpotName::testing_client(); + fail_client.set_enabled(FailSpotName::CrashingThreadIpCopy, true); + + let mut config = MinidumpWriterConfig::new(pid, pid); + config.set_crash_context(get_dummy_crash_context(pid)); + config + .write(&mut tmpfile) + .expect("could not write minidump"); + child.kill().expect("Failed to kill process"); + child.wait().expect("Failed to wait on killed process"); + + let dump = Minidump::read_path(tmpfile.path()).expect("failed to read minidump"); + assert_minidump_contains_soft_error(&dump, "CopyFromProcessError"); +}