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 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.as_ref().map_or(false, |n| n.starts_with("/")))
+        .filter(|m| m.is_executable() && m.name.as_ref().map_or(false, |n| n.starts_with("/")))
     {
         let ehdr_opt = PtraceDumper::copy_from_process(
             pid,

src/linux/maps_reader.rs

@@ -18,6 +18,16 @@ pub struct SystemMappingInfo {
     pub end_address: usize,
 }
 
+bitflags::bitflags! {
+    #[derive(Copy, Clone, Debug, Default, Eq, PartialEq)]
+    #[repr(transparent)]
+    pub struct MappingPermission: u8 {
+        const READABLE = 0b0001;
+        const WRITABLE = 0b0010;
+        const EXECUTABLE = 0b0100;
+    }
+}
+
 // One of these is produced for each mapping in the process (i.e. line in
 // /proc/$x/maps).
 #[derive(Debug, PartialEq, Eq, Clone)]
@@ -34,8 +44,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: MappingPermission, // read, write and execute permissions.
     pub name: Option<String>,
     // pub elf_obj: Option<elf::Elf>,
 }
@@ -120,7 +130,11 @@ impl MappingInfo {
         let start_address = usize::from_str_radix(addresses.next().unwrap(), 16)?;
         let end_address = usize::from_str_radix(addresses.next().unwrap(), 16)?;
 
-        let executable = perms.contains('x');
+        let mut permissions = MappingPermission::default();
+        permissions.set(MappingPermission::EXECUTABLE, perms.contains('x'));
+        permissions.set(MappingPermission::READABLE, perms.contains('r'));
+        permissions.set(MappingPermission::WRITABLE, perms.contains('w'));
+        let permissions = permissions;
 
         // Only copy name if the name is a valid path name, or if
         // it's the VDSO image.
@@ -140,7 +154,7 @@ impl MappingInfo {
                 {
                     module.system_mapping_info.end_address = end_address;
                     module.size = end_address - module.start_address;
-                    module.executable |= executable;
+                    module.permissions |= permissions;
                     return Ok(MappingInfoParsingResult::SkipLine);
                 }
             }
@@ -151,7 +165,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)
                     && perms == RESERVED_FLAGS
@@ -173,7 +187,7 @@ impl MappingInfo {
                 end_address,
             },
             offset,
-            executable,
+            permissions,
             name,
             // elf_obj,
         };
@@ -315,7 +329,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).
@@ -356,7 +370,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
     }
@@ -365,6 +379,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(MappingPermission::EXECUTABLE)
+    }
+
+    pub fn is_readable(&self) -> bool {
+        self.permissions.contains(MappingPermission::READABLE)
+    }
+
+    pub fn is_writable(&self) -> bool {
+        self.permissions.contains(MappingPermission::WRITABLE)
+    }
 }
 
 #[cfg(test)]
@@ -453,7 +479,9 @@ mod tests {
                 end_address: 0x559748406000,
             },
             offset: 0,
-            executable: true,
+            permissions: MappingPermission::READABLE
+                | MappingPermission::WRITABLE
+                | MappingPermission::EXECUTABLE,
             name: Some("/usr/bin/cat".to_string()),
         };
 
@@ -467,7 +495,7 @@ mod tests {
                 end_address: 0x559749b2f000,
             },
             offset: 0,
-            executable: false,
+            permissions: MappingPermission::READABLE | MappingPermission::WRITABLE,
             name: Some("[heap]".to_string()),
         };
 
@@ -481,7 +509,7 @@ mod tests {
                 end_address: 0x7efd968f5000,
             },
             offset: 0,
-            executable: false,
+            permissions: MappingPermission::READABLE | MappingPermission::WRITABLE,
             name: None,
         };
 
@@ -500,7 +528,8 @@ mod tests {
                 end_address: 0x7ffc6e0f9000,
             },
             offset: 0,
-            executable: true,
+            permissions: MappingPermission::READABLE | MappingPermission::EXECUTABLE,
+
             name: Some("linux-gate.so".to_string()),
         };
 
@@ -560,7 +589,9 @@ mod tests {
                 end_address: 0x7efd96d8c000, // ..but this is not visible here
             },
             offset: 0,
-            executable: true,
+            permissions: MappingPermission::READABLE
+                | MappingPermission::WRITABLE
+                | MappingPermission::EXECUTABLE,
             name: Some("/lib64/libc-2.32.so".to_string()),
         };
 
@@ -621,7 +652,7 @@ mod tests {
                 end_address: 0x7f0b97b73000,
             },
             offset: 0,
-            executable: true,
+            permissions: MappingPermission::READABLE | MappingPermission::EXECUTABLE,
             name: Some("libmozgtk.so".to_string()),
         };
 

src/linux/minidump_writer.rs

@@ -103,6 +103,7 @@ impl MinidumpWriter {
     /// Generates a minidump and writes to the destination provided. Returns the in-memory
     /// version of the minidump as well.
     pub fn dump(&mut self, destination: &mut (impl Write + Seek)) -> Result<Vec<u8>> {
+        eprintln!("**** yes 1");
         let mut dumper = PtraceDumper::new(self.process_id)?;
         dumper.suspend_threads()?;
         dumper.late_init()?;
@@ -156,15 +157,23 @@ impl MinidumpWriter {
             return true;
         }
 
-        let (stack_ptr, stack_len) = match dumper.get_stack_info(stack_pointer) {
+        let stack_mapping = match dumper.get_stack(stack_pointer) {
             Ok(x) => x,
             Err(_) => {
                 return false;
             }
         };
+
+        let valid_stack_ptr = if stack_mapping.contains_address(stack_pointer) {
+            stack_pointer
+        } else {
+            stack_mapping.start_address
+        };
+        let stack_len = (stack_mapping.start_address + stack_mapping.size) - valid_stack_ptr;
+
         let stack_copy = match PtraceDumper::copy_from_process(
             self.blamed_thread,
-            stack_ptr as *mut libc::c_void,
+            valid_stack_ptr as *mut libc::c_void,
             stack_len,
         ) {
             Ok(x) => x,
@@ -173,11 +182,10 @@ impl MinidumpWriter {
             }
         };
 
-        let sp_offset = stack_pointer - stack_ptr;
         self.principal_mapping
             .as_ref()
             .unwrap()
-            .stack_has_pointer_to_mapping(&stack_copy, sp_offset)
+            .stack_has_pointer_to_mapping(&stack_copy, 0)
     }
 
     fn generate_dump(

src/linux/ptrace_dumper.rs

@@ -324,28 +324,41 @@ 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.
-    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.
+    // Return the mapping that contains the stack. The stack pointer will
+    // usually point within this mapping, but it might not in case of stack
+    // overflows.
+    pub fn get_stack(&self, mut stack_pointer: usize) -> Result<&MappingInfo, DumperError> {
         // 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);
+            .expect("page size apparently unlimited: doesn't make sense.")
+            as usize;
+
+        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 += page_size;
+            mapping = self.find_mapping(stack_pointer);
+        }
 
-        // The number of bytes of stack which we try to capture.
-        let stack_to_capture = 32 * 1024;
+        mapping.ok_or(DumperError::NoStackPointerMapping)
+    }
 
-        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);
+    fn may_be_stack(mapping: Option<&MappingInfo>) -> bool {
+        if let Some(mapping) = mapping {
+            return !mapping.permissions.is_empty();
+        }
 
-        Ok((stack_pointer, stack_len))
+        return false;
     }
 
     pub fn sanitize_stack_copy(
@@ -394,7 +407,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
@@ -441,7 +454,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,34 +187,31 @@ 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(stack_mapping) = dumper.get_stack(stack_ptr) {
+        let valid_stack_ptr = if stack_mapping.contains_address(stack_ptr) {
+            stack_ptr
+        } else {
+            stack_mapping.start_address
+        };
+
+        let stack_len = (stack_mapping.start_address + stack_mapping.size) - valid_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;
         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;
                 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)
+                    && !principal_mapping.stack_has_pointer_to_mapping(&stack_bytes, 0)
                 {
                     return Ok(());
                 }
@@ -222,15 +221,15 @@ fn fill_thread_stack(
         }
 
         if config.sanitize_stack {
-            dumper.sanitize_stack_copy(&mut stack_bytes, stack_ptr, stack_pointer_offset)?;
+            dumper.sanitize_stack_copy(&mut stack_bytes, stack_ptr, 0)?;
         }
 
         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 = 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);
     }

tests/linux_minidump_writer.rs

@@ -7,7 +7,7 @@ use minidump_writer::{
     app_memory::AppMemory,
     crash_context::CrashContext,
     errors::*,
-    maps_reader::{MappingEntry, MappingInfo, SystemMappingInfo},
+    maps_reader::{MappingEntry, MappingInfo, MappingPermission, SystemMappingInfo},
     minidump_writer::MinidumpWriter,
     ptrace_dumper::PtraceDumper,
     thread_info::Pid,
@@ -132,7 +132,7 @@ fn test_write_and_read_dump_from_parent_helper(context: Context) {
         start_address: mmap_addr,
         size: memory_size,
         offset: 0,
-        executable: false,
+        permissions: MappingPermission::READABLE | MappingPermission::WRITABLE,
         name: Some("a fake mapping".to_string()),
         system_mapping_info: SystemMappingInfo {
             start_address: mmap_addr,