rust-minidump · Jake-Shadle · May 26, 2023 · May 2, 2023 · Jake-Shadle · May 26, 2023
@@ -9,6 +9,7 @@ edition = "2021"
 license = "MIT"
 
 [dependencies]
+bitflags = "2.0"
 byteorder = "1.3.2"
 cfg-if = "1.0"
 crash-context = "0.6"

@@ -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,

@@ -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)]

@@ -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<OsString>,
     // pub elf_obj: Option<elf::Elf>,
 }
@@ -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<OsString> = 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()),
         };
 

@@ -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()

@@ -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<Thread>,
     pub auxv: HashMap<AuxvType, AuxvType>,
     pub mappings: Vec<MappingInfo>,
+    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;
                     }

@@ -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);
     }