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 #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"
@@ -31,9 +32,6 @@ nix = { version = "0.26", default-features = false, features = [
     "user",
 ] }
 
-[target.'cfg(target_os = "windows")'.dependencies]
-bitflags = "2.0"
-
 [target.'cfg(target_os = "macos")'.dependencies]
 # Binds some additional mac specifics not in libc
 mach2 = "0.4"

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/ptrace_dumper.rs

@@ -332,15 +332,31 @@ impl PtraceDumper {
         // 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 stack_pointer = int_stack_pointer & !(page_size - 1);
 
         // The number of bytes of stack which we try to capture.
         let stack_to_capture = 32 * 1024;
 
-        let mapping = self
+        let mut mapping = self
             .find_mapping(stack_pointer)
             .ok_or(DumperError::NoStackPointerMapping)?;
+
+        // If the mapping we found has no permissions then it's likely the
+        // guard page, try using the mapping right before it. The stack grows
+        // towards lower addresses on the platforms we care about so the stack
+        // should appear after the guard page.
+        if mapping.permissions.is_empty() {
+            // This has been true since Linux 4.12
+            let max_guard_page_size = 1024 * 1024;
+
+            let next_mapping_address = mapping.start_address + mapping.size;
+            if (next_mapping_address - stack_pointer) < max_guard_page_size {
+                mapping = self.find_mapping(next_mapping_address).unwrap_or(mapping);
+            }
+        }
+
         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);
@@ -394,7 +410,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 +457,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;
                     }

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,

tests/ptrace_dumper.rs

@@ -259,7 +259,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::<usize>()];