Write stacks correctly during stack overflows (rust-minidump#78)

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 rust-minidump#24

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"

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,

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

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<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()),
         };
 

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

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

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