diff --git a/Cargo.toml b/Cargo.toml index 373a9ec3..832f3e1a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,7 @@ edition = "2021" license = "MIT" [dependencies] +bitflags = "2.0" byteorder = "1.3.2" cfg-if = "1.0" crash-context = "0.6" diff --git a/src/linux/android.rs b/src/linux/android.rs index 465e93c9..1fa28a04 100644 --- a/src/linux/android.rs +++ b/src/linux/android.rs @@ -118,7 +118,7 @@ pub fn late_process_mappings(pid: Pid, mappings: &mut [MappingInfo]) -> Result<( // where the ELF header indicates a mapped shared library. for mut map in mappings .iter_mut() - .filter(|m| m.executable && m.name_is_path()) + .filter(|m| m.is_executable() && m.name_is_path()) { let ehdr_opt = PtraceDumper::copy_from_process( pid, diff --git a/src/linux/errors.rs b/src/linux/errors.rs index a2e70957..f5183ee9 100644 --- a/src/linux/errors.rs +++ b/src/linux/errors.rs @@ -3,6 +3,7 @@ use crate::maps_reader::MappingInfo; use crate::mem_writer::MemoryWriterError; use crate::thread_info::Pid; use goblin; +use nix::errno::Errno; use std::ffi::OsString; use thiserror::Error; @@ -16,6 +17,8 @@ pub enum InitError { PrincipalMappingNotReferenced, #[error("Failed Android specific late init")] AndroidLateInitError(#[from] AndroidError), + #[error("Failed to read the page size")] + PageSizeError(#[from] Errno), } #[derive(Error, Debug)] diff --git a/src/linux/maps_reader.rs b/src/linux/maps_reader.rs index f6bfddec..ade63d45 100644 --- a/src/linux/maps_reader.rs +++ b/src/linux/maps_reader.rs @@ -36,8 +36,8 @@ pub struct MappingInfo { // address range. The following structure holds the original mapping // address range as reported by the operating system. pub system_mapping_info: SystemMappingInfo, - pub offset: usize, // offset into the backed file. - pub executable: bool, // true if the mapping has the execute bit set. + pub offset: usize, // offset into the backed file. + pub permissions: MMPermissions, // read, write and execute permissions. pub name: Option, // pub elf_obj: Option, } @@ -78,8 +78,6 @@ impl MappingInfo { let end_address: usize = mm.address.1.try_into()?; let mut offset: usize = mm.offset.try_into()?; - let executable = mm.perms.contains(MMPermissions::EXECUTE); - let mut pathname: Option = match mm.pathname { MMapPath::Path(p) => Some(p.into()), MMapPath::Heap => Some("[heap]".into()), @@ -110,7 +108,7 @@ impl MappingInfo { { module.system_mapping_info.end_address = end_address; module.size = end_address - module.start_address; - module.executable |= executable; + module.permissions |= mm.perms & MMPermissions::EXECUTE; continue; } } else { @@ -120,7 +118,7 @@ impl MappingInfo { // and which directly follow an executable mapping. let module_end_address = module.start_address + module.size; if (start_address == module_end_address) - && module.executable + && module.is_executable() && is_mapping_a_path(module.name.as_deref()) && (offset == 0 || offset == module_end_address) && mm.perms == MMPermissions::PRIVATE @@ -139,7 +137,7 @@ impl MappingInfo { end_address, }, offset, - executable, + permissions: mm.perms, name: pathname, }); } @@ -295,7 +293,7 @@ impl MappingInfo { return Ok((file_path, file_name)); }; - if self.executable && self.offset != 0 { + if self.is_executable() && self.offset != 0 { // If an executable is mapped from a non-zero offset, this is likely because // the executable was loaded directly from inside an archive file (e.g., an // apk on Android). @@ -330,7 +328,7 @@ impl MappingInfo { self.name.is_some() && // Only want to include one mapping per shared lib. // Avoid filtering executable mappings. - (self.offset == 0 || self.executable) && + (self.offset == 0 || self.is_executable()) && // big enough to get a signature for. self.size >= 4096 } @@ -339,6 +337,18 @@ impl MappingInfo { self.system_mapping_info.start_address <= address && address < self.system_mapping_info.end_address } + + pub fn is_executable(&self) -> bool { + self.permissions.contains(MMPermissions::EXECUTE) + } + + pub fn is_readable(&self) -> bool { + self.permissions.contains(MMPermissions::READ) + } + + pub fn is_writable(&self) -> bool { + self.permissions.contains(MMPermissions::WRITE) + } } #[cfg(test)] @@ -423,7 +433,7 @@ ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsysca end_address: 0x559748406000, }, offset: 0, - executable: true, + permissions: MMPermissions::READ | MMPermissions::EXECUTE | MMPermissions::PRIVATE, name: Some("/usr/bin/cat".into()), }; @@ -437,7 +447,7 @@ ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsysca end_address: 0x559749b2f000, }, offset: 0, - executable: false, + permissions: MMPermissions::READ | MMPermissions::WRITE | MMPermissions::PRIVATE, name: Some("[heap]".into()), }; @@ -451,7 +461,7 @@ ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsysca end_address: 0x7efd968f5000, }, offset: 0, - executable: false, + permissions: MMPermissions::READ | MMPermissions::WRITE | MMPermissions::PRIVATE, name: None, }; @@ -470,7 +480,7 @@ ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsysca end_address: 0x7ffc6e0f9000, }, offset: 0, - executable: true, + permissions: MMPermissions::READ | MMPermissions::EXECUTE | MMPermissions::PRIVATE, name: Some("linux-gate.so".into()), }; @@ -530,7 +540,7 @@ ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsysca end_address: 0x7efd96d8c000, // ..but this is not visible here }, offset: 0, - executable: true, + permissions: MMPermissions::READ | MMPermissions::EXECUTE | MMPermissions::PRIVATE, name: Some("/lib64/libc-2.32.so".into()), }; diff --git a/src/linux/minidump_writer.rs b/src/linux/minidump_writer.rs index f864269c..72e030ba 100644 --- a/src/linux/minidump_writer.rs +++ b/src/linux/minidump_writer.rs @@ -156,15 +156,16 @@ impl MinidumpWriter { return true; } - let (stack_ptr, stack_len) = match dumper.get_stack_info(stack_pointer) { + let (valid_stack_pointer, stack_len) = match dumper.get_stack_info(stack_pointer) { Ok(x) => x, Err(_) => { return false; } }; + let stack_copy = match PtraceDumper::copy_from_process( self.blamed_thread, - stack_ptr as *mut libc::c_void, + valid_stack_pointer as *mut libc::c_void, stack_len, ) { Ok(x) => x, @@ -173,7 +174,7 @@ impl MinidumpWriter { } }; - let sp_offset = stack_pointer - stack_ptr; + let sp_offset = stack_pointer.saturating_sub(valid_stack_pointer); self.principal_mapping .as_ref() .unwrap() diff --git a/src/linux/ptrace_dumper.rs b/src/linux/ptrace_dumper.rs index 3cf030e0..d4bb1466 100644 --- a/src/linux/ptrace_dumper.rs +++ b/src/linux/ptrace_dumper.rs @@ -15,6 +15,7 @@ use nix::{ errno::Errno, sys::{ptrace, wait}, }; +use procfs_core::process::MMPermissions; use std::{collections::HashMap, ffi::c_void, io::BufReader, path, result::Result}; #[derive(Debug, Clone)] @@ -30,6 +31,7 @@ pub struct PtraceDumper { pub threads: Vec, pub auxv: HashMap, pub mappings: Vec, + pub page_size: usize, } #[cfg(target_pointer_width = "32")] @@ -70,6 +72,7 @@ impl PtraceDumper { threads: Vec::new(), auxv: HashMap::new(), mappings: Vec::new(), + page_size: 0, }; dumper.init()?; Ok(dumper) @@ -80,6 +83,10 @@ impl PtraceDumper { self.read_auxv()?; 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(()) } @@ -314,28 +321,52 @@ impl PtraceDumper { ThreadInfo::create(self.pid, self.threads[index].tid) } - // Get information about the stack, given the stack pointer. We don't try to - // walk the stack since we might not have all the information needed to do - // unwind. So we just grab, up to, 32k of stack. + // Returns a valid stack pointer and the mapping that contains the stack. + // The stack pointer will usually point within this mapping, but it might + // not in case of stack overflows, hence the returned pointer might be + // different from the one that was passed in. pub fn get_stack_info(&self, int_stack_pointer: usize) -> Result<(usize, usize), DumperError> { - // Move the stack pointer to the bottom of the page that it's in. - // NOTE: original code uses getpagesize(), which a) isn't there in Rust and - // b) shouldn't be used, as its not portable (see man getpagesize) - let page_size = nix::unistd::sysconf(nix::unistd::SysconfVar::PAGE_SIZE)? - .expect("page size apparently unlimited: doesn't make sense."); - let stack_pointer = int_stack_pointer & !(page_size as usize - 1); - - // The number of bytes of stack which we try to capture. - let stack_to_capture = 32 * 1024; - - let mapping = self - .find_mapping(stack_pointer) - .ok_or(DumperError::NoStackPointerMapping)?; - let offset = stack_pointer - mapping.start_address; - let distance_to_end = mapping.size - offset; - let stack_len = std::cmp::min(distance_to_end, stack_to_capture); - - Ok((stack_pointer, stack_len)) + // Round the stack pointer to the nearest page, this will cause us to + // capture data below the stack pointer which might still be relevant. + let mut stack_pointer = int_stack_pointer & !(self.page_size - 1); + let mut mapping = self.find_mapping(stack_pointer); + + // The guard page has been 1 MiB in size since kernel 4.12, older + // kernels used a 4 KiB one instead. + let guard_page_max_addr = stack_pointer + (1024 * 1024); + + // If we found no mapping, or the mapping we found has no permissions + // then we might have hit a guard page, try looking for a mapping in + // addresses past the stack pointer. Stack grows towards lower addresses + // on the platforms we care about so the stack should appear after the + // guard page. + while !Self::may_be_stack(mapping) && (stack_pointer <= guard_page_max_addr) { + stack_pointer += self.page_size; + mapping = self.find_mapping(stack_pointer); + } + + mapping + .map(|mapping| { + let valid_stack_pointer = if mapping.contains_address(stack_pointer) { + stack_pointer + } else { + mapping.start_address + }; + + let stack_len = mapping.size - (valid_stack_pointer - mapping.start_address); + (valid_stack_pointer, stack_len) + }) + .ok_or(DumperError::NoStackPointerMapping) + } + + fn may_be_stack(mapping: Option<&MappingInfo>) -> bool { + if let Some(mapping) = mapping { + return mapping + .permissions + .intersects(MMPermissions::READ | MMPermissions::WRITE); + } + + false } pub fn sanitize_stack_copy( @@ -384,7 +415,7 @@ impl PtraceDumper { // bit, modulo the bitfield size, is not set then there does not // exist a mapping in mappings that would contain that pointer. for mapping in &self.mappings { - if !mapping.executable { + if !mapping.is_executable() { continue; } // For each mapping, work out the (unmodulo'ed) range of bits to @@ -431,7 +462,7 @@ impl PtraceDumper { let test = addr >> shift; if could_hit_mapping[(test >> 3) & array_mask] & (1 << (test & 7)) != 0 { if let Some(hit_mapping) = self.find_mapping_no_bias(addr) { - if hit_mapping.executable { + if hit_mapping.is_executable() { last_hit_mapping = Some(hit_mapping); continue; } diff --git a/src/linux/sections/thread_list_stream.rs b/src/linux/sections/thread_list_stream.rs index 11e625d0..648aef98 100644 --- a/src/linux/sections/thread_list_stream.rs +++ b/src/linux/sections/thread_list_stream.rs @@ -1,3 +1,5 @@ +use std::cmp::min; + use super::*; use crate::{minidump_cpu::RawContextCPU, minidump_writer::CrashingThreadContext}; @@ -185,27 +187,19 @@ fn fill_thread_stack( thread.stack.memory.data_size = 0; thread.stack.memory.rva = buffer.position() as u32; - if let Ok((mut stack, mut stack_len)) = dumper.get_stack_info(stack_ptr) { - if let MaxStackLen::Len(max_stack_len) = max_stack_len { - if stack_len > max_stack_len { - stack_len = max_stack_len; - - // Skip empty chunks of length max_stack_len. - // Meaning != 0 - if stack_len > 0 { - while stack + stack_len < stack_ptr { - stack += stack_len; - } - } - } - } + if let Ok((valid_stack_ptr, stack_len)) = dumper.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 mut stack_bytes = PtraceDumper::copy_from_process( thread.thread_id.try_into()?, - stack as *mut libc::c_void, + valid_stack_ptr as *mut libc::c_void, stack_len, )?; - let stack_pointer_offset = stack_ptr - stack; + let stack_pointer_offset = stack_ptr.saturating_sub(valid_stack_ptr); if config.skip_stacks_if_mapping_unreferenced { if let Some(principal_mapping) = &config.principal_mapping { let low_addr = principal_mapping.system_mapping_info.start_address; @@ -230,7 +224,7 @@ fn fill_thread_stack( rva: buffer.position() as u32, }; buffer.write_all(&stack_bytes); - thread.stack.start_of_memory_range = stack as u64; + thread.stack.start_of_memory_range = valid_stack_ptr as u64; thread.stack.memory = stack_location; config.memory_blocks.push(thread.stack); } diff --git a/tests/linux_minidump_writer.rs b/tests/linux_minidump_writer.rs index e7a230e3..3493dd2f 100644 --- a/tests/linux_minidump_writer.rs +++ b/tests/linux_minidump_writer.rs @@ -13,6 +13,7 @@ use minidump_writer::{ thread_info::Pid, }; use nix::{errno::Errno, sys::signal::Signal}; +use procfs_core::process::MMPermissions; use std::collections::HashSet; use std::{ @@ -151,7 +152,7 @@ contextual_tests! { start_address: mmap_addr, size: memory_size, offset: 0, - executable: false, + permissions: MMPermissions::READ | MMPermissions::WRITE, name: Some("a fake mapping".into()), system_mapping_info: SystemMappingInfo { start_address: mmap_addr, @@ -532,14 +533,14 @@ fn test_minidump_size_limit() { // large enough value -- the limit-checking code in minidump_writer.rs // does just a rough estimate. // TODO: Fix this properly - // There are occasionally CI failures where the sizes are off by 1 due - // some minor difference in (probably) a string somewhere in the dump - // since the state capture is not going to be 100% the same //assert_eq!(meta.len(), normal_file_size); let min = std::cmp::min(meta.len(), normal_file_size); let max = std::cmp::max(meta.len(), normal_file_size); - assert!(max - min < 10); + // Setting a stack limit limits the size of non-main stacks even before + // the limit is reached. This will cause slight variations in size + // between a limited and an unlimited minidump. + assert!(max - min < 1024, "max = {max:} min = {min:}"); } // Third, write a minidump with a size limit small enough to be triggered. diff --git a/tests/ptrace_dumper.rs b/tests/ptrace_dumper.rs index 02ba836a..0db0396f 100644 --- a/tests/ptrace_dumper.rs +++ b/tests/ptrace_dumper.rs @@ -39,9 +39,10 @@ fn test_thread_list_from_parent() { let info = dumper .get_thread_info_by_index(idx) .expect("Could not get thread info by index"); - let (_stack_ptr, stack_len) = dumper + let (_valid_stack_ptr, stack_len) = dumper .get_stack_info(info.stack_pointer) .expect("Could not get stack_pointer"); + assert!(stack_len > 0); // TODO: I currently know of no way to write the thread_id into the registers using Rust, @@ -259,7 +260,7 @@ fn test_sanitize_stack_copy() { let mapping_info = dumper .find_mapping_no_bias(instr_ptr) .expect("Failed to find mapping info"); - assert!(mapping_info.executable); + assert!(mapping_info.is_executable()); // Pointers to code shouldn't be sanitized. simulated_stack = vec![0u8; 2 * size_of::()];