Repository: luser/rust-minidump (rust)

Issue:
ARM(32) Walker should not use FP unwinding (should not rely on valid LR)
Context:

We have seen some minidumps in the wild which have an obvious addr to something on the stack stored in the LR.

In `get_caller_by_frame_pointer`, the ARM stackwalker just blindly uses the LR of the callee without validation:
https://github.com/luser/rust-minidump/blob/7f5adf743560705fa3d46a784c874655e6681432/minidump-processor/src/stackwalker/arm.rs#L140

Though the LR is not guarenteed to be valid. At least the ARMv6 and ARMv7 architecture manuals say that LR can be used as a general purpose register:

![grafik](https://user-images.githubusercontent.com/580492/154690152-15f77c85-ad2b-4f64-9380-94f5c1ebbe50.png)

(thats from the ARMv7 manual)

Breakpad special cases this in a weird way:

https://github.com/getsentry/breakpad/blob/63c90f00bf64a1b4a8883e50bbe8217048ed2a85/src/processor/stackwalker.cc#L300-L302

This is a bit of a misnomer. Essentially making the `fp_register` invalid completely disabled FP unwinding on ARM on anything other than iOS, probably because the iOS calling conventions are stricter than the ARM conventions.


**TLDR**:

We should either not do FP unwinding on (non-iOS) ARM at all, or more strictly validate the LR value (it should pass the "address looks valid" check).

Pull request description:
fix: Restrict ARM32 LR unwinding to iOS 
The LR register on ARM can be used as general purpose register, so we
should not expect it to hold a valid return addr. Except on iOS, which
has stricter calling conventions.

Based on top of #487, fixes #460.

Patch:
diff --git a/minidump-processor/src/stackwalker/arm.rs b/minidump-processor/src/stackwalker/arm.rs
index a3d28c8..f1c0a30 100644
--- a/minidump-processor/src/stackwalker/arm.rs
+++ b/minidump-processor/src/stackwalker/arm.rs
@@ -9,6 +9,7 @@ use crate::stackwalker::unwind::Unwind;
 use crate::stackwalker::CfiStackWalker;
 use crate::{SymbolProvider, SystemInfo};
 use log::trace;
+use minidump::system_info::Os;
 use minidump::{
     CpuContext, MinidumpContext, MinidumpContextValidity, MinidumpMemory, MinidumpModuleList,
     MinidumpRawContext,
@@ -99,11 +100,22 @@ fn get_caller_by_frame_pointer<P>(
     callee: &StackFrame,
     stack_memory: &MinidumpMemory<'_>,
     _modules: &MinidumpModuleList,
+    system_info: &SystemInfo,
     _symbol_provider: &P,
 ) -> Option<StackFrame>
 where
     P: SymbolProvider + Sync,
 {
+    // The ARM manual states that:
+    // > LR can be used for other purposes when it is not required to support
+    // > a return from a subroutine.
+    // In other words, we need to be conservative and treat it as a general
+    // purpose register. Except on iOS, which has stricter conventions around
+    // register use, and does guarantee that LR contains a valid return addr.
+    if system_info.os != Os::Ios {
+        return None;
+    }
+
     trace!("unwind: trying frame pointer");
     // Assume that the standard %fp-using ARM calling convention is in use.
     // The main quirk of this ABI is that the return address doesn't need to
@@ -305,7 +317,7 @@ impl Unwind for ArmContext {
         grand_callee: Option<&StackFrame>,
         stack_memory: Option<&MinidumpMemory<'_>>,
         modules: &MinidumpModuleList,
-        _system_info: &SystemInfo,
+        system_info: &SystemInfo,
         syms: &P,
     ) -> Option<StackFrame>
     where
@@ -319,7 +331,7 @@ impl Unwind for ArmContext {
             frame = get_caller_by_cfi(self, callee, grand_callee, stack, modules, syms).await;
         }
         if frame.is_none() {
-            frame = get_caller_by_frame_pointer(self, callee, stack, modules, syms);
+            frame = get_caller_by_frame_pointer(self, callee, stack, modules, system_info, syms);
         }
         if frame.is_none() {
             frame = get_caller_by_scan(self, callee, stack, modules, syms).await;


Test patch:
diff --git a/minidump-processor/src/stackwalker/arm_unittest.rs b/minidump-processor/src/stackwalker/arm_unittest.rs
index 2744431..9452ca5 100644
--- a/minidump-processor/src/stackwalker/arm_unittest.rs
+++ b/minidump-processor/src/stackwalker/arm_unittest.rs
@@ -13,6 +13,7 @@ use test_assembler::*;
 struct TestFixture {
     pub raw: CONTEXT_ARM,
     pub modules: MinidumpModuleList,
+    pub system_info: SystemInfo,
     pub symbols: HashMap<String, String>,
 }
 
@@ -26,6 +27,15 @@ impl TestFixture {
                 MinidumpModule::new(0x40000000, 0x10000, "module1"),
                 MinidumpModule::new(0x50000000, 0x10000, "module2"),
             ]),
+            system_info: SystemInfo {
+                os: Os::Ios,
+                os_version: None,
+                os_build: None,
+                cpu: Cpu::Arm,
+                cpu_info: None,
+                cpu_microcode_version: None,
+                cpu_count: 1,
+            },
             symbols: HashMap::new(),
         }
     }
@@ -44,21 +54,12 @@ impl TestFixture {
             size,
             bytes: &stack,
         };
-        let system_info = SystemInfo {
-            os: Os::Windows,
-            os_version: None,
-            os_build: None,
-            cpu: Cpu::Arm,
-            cpu_info: None,
-            cpu_microcode_version: None,
-            cpu_count: 1,
-        };
         let symbolizer = Symbolizer::new(string_symbol_supplier(self.symbols.clone()));
         walk_stack(
             &Some(&context),
             Some(&stack_memory),
             &self.modules,
-            &system_info,
+            &self.system_info,
             &symbolizer,
         )
         .await
@@ -248,6 +249,102 @@ async fn test_scan_first_frame() {
     }
 }
 
+#[tokio::test]
+async fn test_invalid_lr() {
+    let mut f = TestFixture::new();
+    f.system_info.os = Os::Linux;
+
+    let mut stack = Section::new();
+    stack.start().set_const(0x80000000);
+
+    let lr = Label::new();
+    let return_address1 = 0x50000100u32;
+    let return_address2 = 0x50000900u32;
+    let frame1_sp = Label::new();
+    let frame2_sp = Label::new();
+    let frame1_fp = Label::new();
+    let frame2_fp = Label::new();
+
+    stack = stack
+        // frame 0
+        .append_repeated(0, 32) // space
+        .mark(&lr) // the LR points to something on the stack
+        .D32(0x0000000D) // junk that's not
+        .D32(0xF0000000) // a return address
+        .mark(&frame1_fp) // next fp will point to the next value
+        .D32(&frame2_fp) // save current frame pointer
+        .D32(return_address1) // save current link register
+        .mark(&frame1_sp)
+        // frame 1
+        .append_repeated(0, 32) // space
+        .D32(0x0000000D) // junk that's not
+        .D32(0xF0000000) // a return address
+        .mark(&frame2_fp)
+        .D32(0)
+        .D32(return_address2)
+        .mark(&frame2_sp);
+
+    f.raw.set_register("pc", 0x40005510);
+    f.raw.set_register("lr", lr.value().unwrap() as u32);
+    f.raw.set_register("fp", frame1_fp.value().unwrap() as u32);
+    f.raw
+        .set_register("sp", stack.start().value().unwrap() as u32);
+
+    let s = f.walk_stack(stack).await;
+    assert_eq!(s.frames.len(), 3);
+
+    {
+        // Frame 0
+        let frame = &s.frames[0];
+        assert_eq!(frame.trust, FrameTrust::Context);
+        assert_eq!(frame.context.valid, MinidumpContextValidity::All);
+    }
+
+    {
+        // Frame 1
+        let frame = &s.frames[1];
+        let valid = &frame.context.valid;
+        assert_eq!(frame.trust, FrameTrust::Scan);
+        if let MinidumpContextValidity::Some(ref which) = valid {
+            assert_eq!(which.len(), 2);
+        } else {
+            unreachable!();
+        }
+
+        if let MinidumpRawContext::Arm(ctx) = &frame.context.raw {
+            assert_eq!(ctx.get_register("pc", valid).unwrap(), return_address1);
+            assert_eq!(
+                ctx.get_register("sp", valid).unwrap(),
+                frame1_sp.value().unwrap() as u32
+            );
+        } else {
+            unreachable!();
+        }
+    }
+
+    {
+        // Frame 2
+        let frame = &s.frames[2];
+        let valid = &frame.context.valid;
+        assert_eq!(frame.trust, FrameTrust::Scan);
+        if let MinidumpContextValidity::Some(ref which) = valid {
+            assert_eq!(which.len(), 2);
+        } else {
+            unreachable!();
+        }
+
+        if let MinidumpRawContext::Arm(ctx) = &frame.context.raw {
+            assert_eq!(ctx.get_register("pc", valid).unwrap(), return_address2);
+            assert_eq!(
+                ctx.get_register("sp", valid).unwrap(),
+                frame2_sp.value().unwrap() as u32
+            );
+        } else {
+            unreachable!();
+        }
+    }
+}
+
 #[tokio::test]
 async fn test_frame_pointer() {
     // Frame-pointer-based unwinding