Repository: luser/rust-minidump
Pull request #482 (2022-03-04 17:36:26)

ARM64: Emulate breakpad for 0 frame pointers
This aligns frame pointer unwinding on ARM64 more closely with the way breakpad works, to wit: if the previous frame pointer was 0, rust-minidump currently short-circuits because it can't get the values of `fp` and `lr` from that address. Breakpad, on the other hand, soldiers on and simply sets those registers to 0 as well. As a consequence, the frame pointer unwind succeeds in breakpad, while rust-minidump falls back to stack scanning. I honestly don't know if that makes sense—the reason I implemented this PR is because this difference in behavior is behind #481. Thoughts, @Gankra @jan-auer?

luser: > I wasn't able to find comments or a discussion of why it runs this way

I still have the emails from the code review in a mail folder, but also they're available on the google-breakpad-dev group This change appears to have been made for better handling of crashes from iOS.

https://groups.google.com/g/google-breakpad-dev/c/h4gC7pyc6aI/m/t3W_X4sTe-EJ
https://groups.google.com/g/google-breakpad-dev/c/PvZi587dHM4/m/nquU9omKnlYJ


Patch:
diff --git a/minidump-processor/src/stackwalker/arm.rs b/minidump-processor/src/stackwalker/arm.rs
index 3f70f36..d2e9a2d 100644
--- a/minidump-processor/src/stackwalker/arm.rs
+++ b/minidump-processor/src/stackwalker/arm.rs
@@ -135,16 +135,18 @@ where
         // drowning the rest of the code in checked_add.
         return None;
     }
-    let caller_fp = stack_memory.get_memory_at_address(last_fp as u64)?;
-    let caller_lr = stack_memory.get_memory_at_address(last_fp as u64 + POINTER_WIDTH as u64)?;
-    let caller_pc = last_lr;
-
-    // TODO: why does breakpad do this? how can fp be null by here?
-    let caller_sp = if last_fp == 0 {
-        last_sp
+    let (caller_fp, caller_lr, caller_sp) = if last_fp == 0 {
+        // In this case we want unwinding to stop. One of the termination conditions in get_caller_frame
+        // is that caller_sp <= last_sp. Therefore we can force termination by setting caller_sp = last_sp.
+        (0, 0, last_sp)
     } else {
-        last_fp + POINTER_WIDTH * 2
+        (
+            stack_memory.get_memory_at_address(last_fp as u64)?,
+            stack_memory.get_memory_at_address(last_fp as u64 + POINTER_WIDTH as u64)?,
+            last_fp + POINTER_WIDTH * 2,
+        )
     };
+    let caller_pc = last_lr;
 
     // Don't do any more validation, just assume it worked.
 
diff --git a/minidump-processor/src/stackwalker/arm64.rs b/minidump-processor/src/stackwalker/arm64.rs
index 1e40eb4..3aa98bc 100644
--- a/minidump-processor/src/stackwalker/arm64.rs
+++ b/minidump-processor/src/stackwalker/arm64.rs
@@ -149,17 +149,20 @@ where
         // drowning the rest of the code in checked_add.
         return None;
     }
-    let caller_fp = stack_memory.get_memory_at_address(last_fp as u64)?;
-    let caller_lr = stack_memory.get_memory_at_address(last_fp + POINTER_WIDTH as u64)?;
-    let caller_lr = ptr_auth_strip(modules, caller_lr);
-    let caller_pc = last_lr;
 
-    // TODO: why does breakpad do this? How could we get this far with a null fp?
-    let caller_sp = if last_fp == 0 {
-        last_sp
+    let (caller_fp, caller_lr, caller_sp) = if last_fp == 0 {
+        // In this case we want unwinding to stop. One of the termination conditions in get_caller_frame
+        // is that caller_sp <= last_sp. Therefore we can force termination by setting caller_sp = last_sp.
+        (0, 0, last_sp)
     } else {
-        last_fp + POINTER_WIDTH * 2
+        (
+            stack_memory.get_memory_at_address(last_fp as u64)?,
+            stack_memory.get_memory_at_address(last_fp + POINTER_WIDTH as u64)?,
+            last_fp + POINTER_WIDTH * 2,
+        )
     };
+    let caller_lr = ptr_auth_strip(modules, caller_lr);
+    let caller_pc = last_lr;
 
     // TODO: restore all the other callee-save registers that weren't touched.
     // unclear: does this mean we need to be aware of ".undef" entries at this point?


Test patch:
diff --git a/minidump-processor/src/stackwalker/arm_unittest.rs b/minidump-processor/src/stackwalker/arm_unittest.rs
index 78c3b50..6f265cd 100644
--- a/minidump-processor/src/stackwalker/arm_unittest.rs
+++ b/minidump-processor/src/stackwalker/arm_unittest.rs
@@ -106,6 +106,9 @@ async fn test_scan_without_symbols() {
         .append_repeated(0, 32); // end of stack
 
     f.raw.set_register("pc", 0x40005510);
+    // set an invalid non-zero value for the frame pointer
+    // to force stack scanning
+    f.raw.set_register("fp", 0x00000001);
     f.raw
         .set_register("sp", stack.start().value().unwrap() as u32);
 
@@ -195,6 +198,9 @@ async fn test_scan_first_frame() {
         .append_repeated(0, 64); // end of stack
 
     f.raw.set_register("pc", 0x40005510);
+    // set an invalid non-zero value for the frame pointer
+    // to force stack scanning
+    f.raw.set_register("fp", 0x00000001);
     f.raw
         .set_register("sp", stack.start().value().unwrap() as u32);