From 04bfdb74241ef61d618147886f81784d923bcc47 Mon Sep 17 00:00:00 2001 From: Gabriele Svelto Date: Tue, 2 May 2023 10:35:27 +0200 Subject: [PATCH] Write stacks correctly during stack overflows When encountering a stack overflow we often crash accessing the guard page. The logic assumed that wherever the stack pointer was so was the stack, but this lead the writer to dump the guard page in these cases. This patch changes the logic to inspect the properties of the mapping that appears to correspond to the stack and - if it looks like a guard page - look for the actual stack instead. This change also removes the double limitation we had when retrieving stacks on Linux: previously the logic would only grab the first 32 KiB of each stack before checking for user-specified limits. Now only the user-specified limits are enforced and - if not present - the full stack is stored in the minidump. This brings the behavior in line with minidumps generated on Windows by windbg.dll. This fixes #24 --- Cargo.toml | 1 + src/linux/android.rs | 2 +- src/linux/errors.rs | 3 + src/linux/maps_reader.rs | 38 +++++++----- src/linux/minidump_writer.rs | 7 ++- src/linux/ptrace_dumper.rs | 77 +++++++++++++++++------- src/linux/sections/thread_list_stream.rs | 28 ++++----- tests/linux_minidump_writer.rs | 11 ++-- tests/ptrace_dumper.rs | 5 +- 9 files changed, 107 insertions(+), 65 deletions(-) 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::()];