Add initial MacOS implementation #21

Jake-Shadle · 2022-04-20T06:35:41Z

This essentially copies the Breakpad minidump creation, with a couple of minor changes.

Breakpad uses getrusage to determine the process start time, however...this was wrong. getrusage can only retrieve the process information for the current task (process) or its children, this implementation instead uses proc_pidinfo to get the process info for the actual crashed process.
Breakpad/Crashpad both retrieve OS/kernel version information via an deprecated and undocumented (that's not saying much though, Apple hates documentation it seems) method, however there is a perfectly fine way to retrieve this information via sysctlbyname, kern.osproductversion has been available since 10.13.4 which is 2 versions older than the oldest supported version, and kern.osversion has been available for...I actually don't know how long, again, Apple is really terrible at documentation.

Note that this implementation only supports MacOS (not iOS), and only x86_64 and aarch64 as those are the only officially supported architectures on MacOS now.

Also note, most of my local testing has been done on an aarch64 machine, I'll add building of aarch64 from an x86_64 to CI to cover that case, but we won't be able to run actual tests (not that CI actually runs tests atm...) until Github Actions adds M1 runners, which who knows when that will happen.

Support for VMs on Apple M1 actions/runner-images#2187

Another option would be for Embark to supply a temporary M1 runner for this project similarly to how we already supply one for https://github.com/bytecodealliance/wasmtime as seen in https://github.com/EmbarkStudios/wasmtime-aarch64-apple-darwin.

Jake-Shadle · 2022-04-26T15:48:00Z

Converting this to a draft, I've just found out that I completely misunderstood mach_task_self which always returns the same value, I had assumed it was a unique identifier like a PID, but that's completely wrong and apparently I need to refactor the code I was using to test this change (in addition to the tests in this PR).

The value returned by task_self_trap() is not a unique identifier like a Unix process ID. In fact, its value will be the same for all tasks, even on different machines, provided the machines are running identical kernels.

This starts a refactor to pull out all of the system calls to a separate TaskDumper similar to PTraceDumper to more easily test and maintain the code

Jake-Shadle · 2022-04-28T19:47:41Z

I've now got this working on x86_64 and aarch64, I've commented out the test that I used for confirmation since its dependency chain leads to openssl and the build was failing and I don't want to deal with it at the moment, I'll fix it properly tomorrow by opening a PR to breakpad-symbols to use rustls instead of openssl.

Gankra · 2022-04-28T20:31:10Z

Is this something that the vendored openssl hack fixes? https://github.com/rust-minidump/rust-minidump/blob/main/minidump-stackwalk/Cargo.toml#L27

Jake-Shadle · 2022-04-28T20:51:38Z

Possibly, but the dependency was on minidump-processor which doesn't have that feature. Exposing the rustls feature on reqwest or making reqwest/http symbols optional altogether would definitely work though.

Gankra · 2022-04-28T21:32:58Z

yeah in theory all the crates that touch breakpad-symbols should have the hack but I just tossed on the one I needed to ship

I encountered an issue, only on x86_64, but could affect aarch64 as well, where a stack overflow could fail to be dumped due the stack address being borked causing the vm_read to fail with `InvalidAddress`. This changes it so that in that case a sentinel value is used in place of the actual stack, as there was already a graceful handling of the case where the stack was reported to be of 0 size that was ported from Breakpad

Jake-Shadle · 2022-04-29T14:56:42Z

src/dir_section.rs

@@ -0,0 +1,103 @@
+use crate::{


This file is just a move of the DirSection and related code so that it can be shared between Linux and Mac.

Jake-Shadle · 2022-04-29T14:57:32Z

src/lib.rs

+// Non-windows platforms need additional code since they are essentially
+// replicating functionality we get for free on Windows
+cfg_if::cfg_if! {
+    if #[cfg(not(target_os = "windows"))] {
+        pub(crate) mod mem_writer;
+        pub(crate) mod dir_section;
+    }
+}


Like the comment says, these modules are shared code that non-windows can use to help write minidump streams.

Jake-Shadle · 2022-04-29T14:59:30Z

src/mac.rs

+#[cfg(target_pointer_width = "32")]
+compile_error!("Various MacOS FFI bindings assume we are on a 64-bit architechture");
+


Since Apple only officially supports x86_64 and aarch64 for MacOS it was easier/cleaner to just use the 64-bit structs in some of the handcoded bindings that weren't available in mach2, this just guards against someone trying to target a non-64-bit architecture and getting a nasty surprise at runtime.

Jake-Shadle · 2022-04-29T15:00:30Z

src/mac/mach.rs

+// Just exports all of the mach functions we use into a flat list
+pub use mach2::{
+    kern_return::{kern_return_t, KERN_SUCCESS},
+    port::mach_port_name_t,
+    task::{self, task_threads},
+    task_info,
+    thread_act::thread_get_state,
+    traps::mach_task_self,
+    vm::{mach_vm_deallocate, mach_vm_read, mach_vm_region_recurse},
+    vm_region::vm_region_submap_info_64,
+};
+


The mach2 crate has a really annoying layout that I suppose they inherited from Apple.

Jake-Shadle · 2022-04-29T15:02:02Z

src/mac/mach.rs

+/// A Mach kernel error.
+///
+/// See <usr/include/mach/kern_return.h>.
+#[derive(thiserror::Error, Debug)]
+pub enum KernelError {
+    #[error("specified address is not currently valid")]


This just puts kernel errors into an enum so the debug/error display for them is not an opaque integer that ones must lookup. Also adds the actual comments in the kernel source for each one (at least the relevant parts) to at least give a user some idea what went wrong.

Jake-Shadle · 2022-04-29T15:13:29Z

src/mac/streams/system_info.rs

+        let mut it = vers.split('.');
+
+        let major: u32 = it.next()?.parse().ok()?;
+        let minor: u32 = it.next()?.parse().ok()?;
+        let patch: u32 = it.next().and_then(|p| p.parse().ok()).unwrap_or_default();
+


The version is usually a standard semver <major>.<minor>.<patch> but it can be just <major>.<minor>

Jake-Shadle · 2022-04-29T15:19:20Z

src/mac/streams/thread_list.rs

+                // This is kind of a lie as we don't actually include the full float state..?
+                out.context_flags = format::ContextFlagsArm64Old::CONTEXT_ARM64_OLD_FULL.bits() as u64;
+


The MacOS threadstate doesn't include floating point registers on aarch64, but Breakpad was still setting MD_CONTEXT_ARM64_FULL_OLD which includes MD_CONTEXT_ARM64_FLOATING_POINT_OLD. From what I have seen this is fine and stackwalker/sentry have no issues with the thread state in such a dump.

Jake-Shadle · 2022-04-29T15:19:53Z

src/mem_writer.rs

+use crate::minidump_format::{MDLocationDescriptor, MDRVA};
+use scroll::ctx::{SizeWith, TryIntoCtx};
+


The contents of this file were moved to not be Linux specific.

Jake-Shadle · 2022-04-29T15:21:25Z

src/windows/minidump_writer.rs

    crash_context: crash_context::CrashContext,
    /// Handle to the crashing process, which could be ourselves
    crashing_process: HANDLE,
-    /// The pid of the crashing process.
-    crashing_pid: u32,
    /// The `EXCEPTION_POINTERS` contained in crash context is a pointer into the


This was a slight refactoring from the bump to crash-context 0.2 since the pid is now part of the crash-context itself on Windows to match linux and mac, so this allowed us to just cleanup the interface a bit since it can internally open the process in the correct way without the user needing to specify it.

tests/mac_minidump_writer.rs

There are a couple of advisories that are "OK" for now (though I will PR them to fix them), but the cargo audit GHA doesn't have a way to ignore advisories, cargo-deny does

Jake-Shadle · 2022-05-03T05:56:10Z

FYI I've filed mdsteele/rust-cab#13 to get rid of the 2 security advisories introduced by this change.

This PR makes changes so that this crate it can be used as a library (eg rust-minidump/minidump-writer#21 (comment)). - Replaced `failure` with `anyhow`. `failure` has been [unmaintained](https://rustsec.org/advisories/RUSTSEC-2020-0036.html) for several years, `anyhow` is one of the maintained alternatives. This change is intended as temporary, as IMO the errors in this crate should use `thiserror`, or just manually defined errors, as it makes library usage easier by not using string errors for everything, but made sense for a binary application. - Places `clap` and `simplelog` behind a `cli` feature flag, these dependencies are only used when used as a binary, crates that depend on this as a library don't need them and thus should not compile them. To preserve backwards compatibility for `cargo install` this feature is made default, so library users will need to explicitly opt-out. - Places HTTP symbol retrieval behind the `http` feature. This removes several heavy dependencies only used for retrieving symbols from remote symbol stores, in the use case that motivated this PR linked above, this functionality is not needed and only adds compile time for no benefit. Again, this feature is enabled by default for backwards compatibility. - Replaces `reqwest`'s use of native-tls with `rustls`, simplifying the build when `http` is enabled. I frankly despise OpenSSL and all of the aggravating problems that come with it, `rustls` is a drop-in replacement that completely avoids all of those aggravations. This change is obviously due to my personal bias, I can back it out if requested. Resolves: #253

Jake-Shadle force-pushed the macos branch 2 times, most recently from 84612ba to 20f1337 Compare April 20, 2022 07:04

Jake-Shadle mentioned this pull request Apr 20, 2022

Embark's pull requests EmbarkStudios/rust-ecosystem#20

Open

26 tasks

msirringhaus requested a review from Gankra April 20, 2022 07:59

Jake-Shadle force-pushed the macos branch 7 times, most recently from 9e575a3 to 4dc2e8a Compare April 20, 2022 10:11

This was referenced Apr 20, 2022

Add more derive impls rust-minidump/rust-minidump#534

Merged

Add inital MacOS implementation EmbarkStudios/crash-handling#17

Merged

Jake-Shadle force-pushed the macos branch 6 times, most recently from be5d699 to 0a16b84 Compare April 26, 2022 05:57

Jake-Shadle marked this pull request as draft April 26, 2022 15:44

Jake-Shadle added 9 commits April 27, 2022 08:34

Move memory write to be a shared module

b4c50cb

Begin fleshing out MacOS implementation

d777ecc

Further fleshing out

977ebb2

Add remaining streams

cbf84f3

Begin refactor

b3c8c10

This starts a refactor to pull out all of the system calls to a separate TaskDumper similar to PTraceDumper to more easily test and maintain the code

Update branch name for path

b439d66

Make DirSection shared

9c0fe44

Finish up refactoring and actually get it compiling

c73f1f4

Add macos dump test

cc84ce9

Jake-Shadle added 2 commits April 28, 2022 08:23

Using as a path dep for now

4ec6a9a

Use crash-context to properly send process info

4af3274

Jake-Shadle marked this pull request as ready for review April 28, 2022 19:42

Remove minidump-processor temporarily due to openssl

73c53c8

Jake-Shadle mentioned this pull request Apr 28, 2022

MacOS follow up #23

Closed

Jake-Shadle added 3 commits April 28, 2022 21:55

Fix windows

ccada3b

Fix it better

fe4e0d9

oops

b67946e

Jake-Shadle added 5 commits April 29, 2022 07:33

Get rid of needless functions now that pid is part of context

a8e0783

Shame

b18c572

Fix windows test compilation

a563388

Remove crash-context patch

9189c8d

Jake-Shadle commented Apr 29, 2022

View reviewed changes

Add CHANGELOG.md

46e3999

This was referenced May 2, 2022

Libify mozilla/dump_syms#356

Merged

Make breakpad-symbols HTTP retrieval optional rust-minidump/rust-minidump#548

Merged

Jake-Shadle added 4 commits May 2, 2022 13:44

Reenable test with patches

2082ff7

Fix build

41c1768

Fix it for real this time

0b62a95

Replace cargo-audit with cargo-deny

699f248

There are a couple of advisories that are "OK" for now (though I will PR them to fix them), but the cargo audit GHA doesn't have a way to ignore advisories, cargo-deny does

Jake-Shadle added 2 commits May 19, 2022 21:43

Remove minidump* patches now that 0.11 was released

1018093

Ensure aarch64 compiles as well

4e44566

Jake-Shadle merged commit df24400 into rust-minidump:main May 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial MacOS implementation #21

Add initial MacOS implementation #21

Jake-Shadle commented Apr 20, 2022 •

edited

Loading

Jake-Shadle commented Apr 26, 2022 •

edited

Loading

Jake-Shadle commented Apr 28, 2022

Gankra commented Apr 28, 2022

Jake-Shadle commented Apr 28, 2022

Gankra commented Apr 28, 2022 •

edited

Loading

Jake-Shadle Apr 29, 2022

Jake-Shadle Apr 29, 2022

Jake-Shadle Apr 29, 2022

Jake-Shadle Apr 29, 2022 •

edited

Loading

Jake-Shadle Apr 29, 2022

Jake-Shadle Apr 29, 2022

Jake-Shadle Apr 29, 2022

Jake-Shadle Apr 29, 2022

Jake-Shadle Apr 29, 2022

Jake-Shadle commented May 3, 2022

		#[cfg(target_pointer_width = "32")]
		compile_error!("Various MacOS FFI bindings assume we are on a 64-bit architechture");

		// This is kind of a lie as we don't actually include the full float state..?
		out.context_flags = format::ContextFlagsArm64Old::CONTEXT_ARM64_OLD_FULL.bits() as u64;

		use crate::minidump_format::{MDLocationDescriptor, MDRVA};
		use scroll::ctx::{SizeWith, TryIntoCtx};

Add initial MacOS implementation #21

Add initial MacOS implementation #21

Conversation

Jake-Shadle commented Apr 20, 2022 • edited Loading

Jake-Shadle commented Apr 26, 2022 • edited Loading

Jake-Shadle commented Apr 28, 2022

Gankra commented Apr 28, 2022

Jake-Shadle commented Apr 28, 2022

Gankra commented Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jake-Shadle Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jake-Shadle commented May 3, 2022

Jake-Shadle commented Apr 20, 2022 •

edited

Loading

Jake-Shadle commented Apr 26, 2022 •

edited

Loading

Gankra commented Apr 28, 2022 •

edited

Loading

Jake-Shadle Apr 29, 2022 •

edited

Loading