From ac878bb79012649d75df34fb7e86e269d44d4973 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 20 Jul 2022 12:28:09 +0200 Subject: [PATCH 1/6] Update minidump, use patch for crash-context --- Cargo.toml | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 60a63d2d..aa4ad724 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,7 +13,7 @@ byteorder = "1.3.2" cfg-if = "1.0" crash-context = "0.3" memoffset = "0.6" -minidump-common = "0.11" +minidump-common = "0.12" scroll = "0.11" tempfile = "3.1.0" thiserror = "1.0.21" @@ -24,7 +24,12 @@ goblin = "0.5" memmap2 = "0.5" [target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies] -nix = { version = "0.24", default-features = false, features = ["mman", "process", "ptrace", "user"] } +nix = { version = "0.24", default-features = false, features = [ + "mman", + "process", + "ptrace", + "user", +] } [target.'cfg(target_os = "windows")'.dependencies.windows-sys] version = "0.36" @@ -48,15 +53,16 @@ mach2 = "0.4" [dev-dependencies] # Minidump-processor is async so we need an executor futures = { version = "0.3", features = ["executor"] } -minidump = "0.11" +minidump = "0.12" memmap2 = "0.5" [target.'cfg(target_os = "macos")'.dev-dependencies] # We dump symbols for the `test` executable so that we can validate that minidumps # created by this crate can be processed by minidump-processor dump_syms = { version = "1.0.1", default-features = false } -minidump-processor = { version = "0.11", default-features = false, features = [ - "breakpad-syms", -] } +minidump-processor = { version = "0.12", default-features = false } similar-asserts = "1.2" uuid = "1.0" + +[patch.crates-io] +crash-context = { git = "https://github.com/EmbarkStudios/crash-handling", branch = "macos-exc-resource" } From 1122534b00980cc05cba9a60172e804c8da0e9c5 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Thu, 21 Jul 2022 10:12:38 +0200 Subject: [PATCH 2/6] Improve support for Mach exceptions * `EXC_CRASH` exceptions are now unwrapped (if valid) and the wrapped exception is the one written to the minidump * `EXC_RESOURCE` and `EXC_GUARD` are now properly handled to report the upper 32-bits as `exception_flags` * The `exception_address` now properly takes `EXC_BAD_ACCESS` into account rather than using the subcode for the address regardless of the exception type * The exception type (kind), full 64-bit code, and full 64-bit subcode (if it exists) are now written to `exception_information` so that no exception data is lost and a minidump parser can extract those full details --- Cargo.toml | 2 +- src/mac/streams/exception.rs | 126 ++++++++++++++++++++++++++++++++--- 2 files changed, 118 insertions(+), 10 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index aa4ad724..1962143a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -65,4 +65,4 @@ similar-asserts = "1.2" uuid = "1.0" [patch.crates-io] -crash-context = { git = "https://github.com/EmbarkStudios/crash-handling", branch = "macos-exc-resource" } +crash-context = { git = "https://github.com/EmbarkStudios/crash-handling", branch = "macos/exc-guard" } diff --git a/src/mac/streams/exception.rs b/src/mac/streams/exception.rs index 6d7c91af..e594dd8d 100644 --- a/src/mac/streams/exception.rs +++ b/src/mac/streams/exception.rs @@ -1,5 +1,7 @@ use super::*; +use mach2::exception_types as et; + impl MinidumpWriter { /// Writes the [`minidump_common::format::MINIDUMP_EXCEPTION_STREAM`] stream. /// @@ -33,21 +35,85 @@ impl MinidumpWriter { .exception .as_ref() .map(|exc| { - let exception_address = if let Some(subcode) = exc.subcode { - subcode as u64 - } else if let Some(ts) = thread_state { - ts.pc() + let code = exc.code as u64; + + // `EXC_CRASH` exceptions wrap other exceptions, so we want to + // retrieve the _actual_ exception + let wrapped_exc = if exc.kind as u32 == et::EXC_CRASH { + recover_exc_crash_wrapped_exception(code) + } else { + None + }; + + // For EXC_RESOURCE and EXC_GUARD crashes Crashpad records the + // uppermost 32 bits of the exception code in the exception flags, + // as they are the most interesting for those exceptions. Neither + // of these exceptions can be wrapped by an `EXC_CRASH` + // + // EXC_GUARD + // code: + // +-------------------+----------------+--------------+ + // |[63:61] guard type | [60:32] flavor | [31:0] target| + // +-------------------+----------------+--------------+ + // + // EXC_RESOURCE + // code: + // +--------------------------------------------------------+ + // |[63:61] resource type | [60:58] flavor | [57:32] unused | + // +--------------------------------------------------------+ + let exception_code = + if exc.kind as u32 == et::EXC_RESOURCE || exc.kind as u32 == et::EXC_GUARD { + (code >> 32) as u32 + } else if let Some(wrapped) = wrapped_exc { + wrapped.code + } else { + // For all other exceptions types, the value in the code + // _should_ never exceed 32 bits, crashpad does an actual + // range check here, but since we don't really log anything + // else at the moment I'll punt that for now + // TODO: log/do something if exc.code > u32::MAX + code as u32 + }; + + let exception_kind = if let Some(wrapped) = wrapped_exc { + wrapped.kind } else { - 0 + exc.kind }; + let exception_address = + if exception_kind == et::EXC_BAD_ACCESS && exc.subcode.is_some() { + exc.subcode.unwrap_or_default() + } else if let Some(ts) = thread_state { + ts.pc() + } else { + 0 + }; + // The naming is confusing here, but it is how it is - MDException { - exception_code: exc.kind as u32, - exception_flags: exc.code as u32, + let mut md_exc = MDException { + exception_code: exception_kind, + exception_flags: exception_code, exception_address, ..Default::default() - } + }; + + // Now append the (mostly) original information to the "ancillary" + // exception_information at the end. This allows a minidump parser + // to recover the full exception information for the crash, rather + // than only using the (potentially) truncated information we + // just set in `exception_code` and `exception_flags` + md_exc.exception_information[0] = exception_kind as u64; + md_exc.exception_information[1] = code; + + md_exc.number_parameters = if let Some(subcode) = exc.subcode { + md_exc.exception_information[2] = subcode; + 3 + } else { + 2 + }; + + md_exc }) .unwrap_or_default(); @@ -66,3 +132,45 @@ impl MinidumpWriter { }) } } + +/// [`et::EXC_CRASH`] is a wrapper exception around another exception, but not +/// all exceptions can be wrapped by it, so this function validates that the +/// `EXC_CRASH` is actually valid +#[inline] +fn is_valid_exc_crash(exc_code: u64) -> bool { + let wrapped = ((exc_code >> 20) & 0xf) as u32; + + !( + wrapped == et::EXC_CRASH // EXC_CRASH can't wrap another one + || wrapped == et::EXC_RESOURCE // EXC_RESOURCE would lose information + || wrapped == et::EXC_GUARD // EXC_GUARD would lose information + || wrapped == et::EXC_CORPSE_NOTIFY + // cannot be wrapped + ) +} + +/// The details for an exception wrapped by an `EXC_CRASH` +#[derive(Copy, Clone)] +struct WrappedException { + /// The `EXC_*` that was wrapped + kind: u32, + /// The code of the wrapped exception, for all exceptions other than + /// `EXC_RESOURCE` and `EXC_GUARD` this _should_ never exceed 32 bits, and + /// is one of the reasons that `EXC_CRASH` cannot wrap those 2 exceptions + code: u32, + /// The Unix signal number that the original exception was converted into + _signal: u8, +} + +/// Unwraps an `EXC_CRASH` exception code to the inner exception it wraps. +/// +/// Will return `None` if the specified code is wrapping an exception that +/// should not be possible to be wrapped in an `EXC_CRASH` +#[inline] +fn recover_exc_crash_wrapped_exception(code: u64) -> Option { + is_valid_exc_crash(code).then(|| WrappedException { + kind: ((code >> 20) & 0xf) as u32, + code: (code & 0xfffff) as u32, + _signal: ((code >> 24) & 0xff) as u8, + }) +} From 07867cbe0e1977323e41a8d225c9d1e2edf46075 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Thu, 21 Jul 2022 10:50:40 +0200 Subject: [PATCH 3/6] Minor cleanup --- src/linux/sections/exception_stream.rs | 19 +++++-------------- src/linux/sections/thread_list_stream.rs | 2 +- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/src/linux/sections/exception_stream.rs b/src/linux/sections/exception_stream.rs index 665b2b47..832d6734 100644 --- a/src/linux/sections/exception_stream.rs +++ b/src/linux/sections/exception_stream.rs @@ -46,30 +46,21 @@ pub fn write( buffer: &mut DumpBuf, ) -> Result { let exception = if let Some(context) = &config.crash_context { - let sig_addr = context.inner.siginfo.ssi_addr as u64; - MDException { exception_code: context.inner.siginfo.ssi_signo as u32, exception_flags: context.inner.siginfo.ssi_code as u32, - exception_record: 0, - exception_address: sig_addr, - number_parameters: 0, - __align: 0, - exception_information: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], + exception_address: context.inner.siginfo.ssi_addr as u64, + ..Default::default() } } else { - let addr = match config.crashing_thread_context { - CrashingThreadContext::CrashContextPlusAddress((_, addr)) => addr, + let addr = match &config.crashing_thread_context { + CrashingThreadContext::CrashContextPlusAddress((_, addr)) => *addr, _ => 0, }; MDException { exception_code: MDExceptionCodeLinux::MD_EXCEPTION_CODE_LIN_DUMP_REQUESTED as u32, - exception_flags: 0, - exception_record: 0, exception_address: addr as u64, - number_parameters: 0, - __align: 0, - exception_information: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], + ..Default::default() } }; diff --git a/src/linux/sections/thread_list_stream.rs b/src/linux/sections/thread_list_stream.rs index e4607784..0e952e87 100644 --- a/src/linux/sections/thread_list_stream.rs +++ b/src/linux/sections/thread_list_stream.rs @@ -163,7 +163,7 @@ pub fn write( // while the instruction pointer is already here. config.crashing_thread_context = CrashingThreadContext::CrashContextPlusAddress(( cpu_section.location(), - info.get_instruction_pointer(), + instruction_ptr, )); } } From ff8882398ecc484c00a9457020029a7adb1f98b1 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Thu, 21 Jul 2022 15:26:28 +0200 Subject: [PATCH 4/6] Update crash-context --- Cargo.toml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 1962143a..a4010a54 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,7 +11,7 @@ license = "MIT" [dependencies] byteorder = "1.3.2" cfg-if = "1.0" -crash-context = "0.3" +crash-context = "0.4" memoffset = "0.6" minidump-common = "0.12" scroll = "0.11" @@ -63,6 +63,3 @@ dump_syms = { version = "1.0.1", default-features = false } minidump-processor = { version = "0.12", default-features = false } similar-asserts = "1.2" uuid = "1.0" - -[patch.crates-io] -crash-context = { git = "https://github.com/EmbarkStudios/crash-handling", branch = "macos/exc-guard" } From c1501c39730bab23b7db450389bd32cfa378cc99 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Thu, 21 Jul 2022 15:38:17 +0200 Subject: [PATCH 5/6] Fix tests --- src/bin/test.rs | 2 +- src/mac/streams/module_list.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bin/test.rs b/src/bin/test.rs index 6f84c799..c5ad8e0c 100644 --- a/src/bin/test.rs +++ b/src/bin/test.rs @@ -363,7 +363,7 @@ mod mac { thread: mach2::mach_init::mach_thread_self(), handler_thread: mach2::port::MACH_PORT_NULL, exception: Some(crash_context::ExceptionInfo { - kind: exception as i32, + kind: exception, code: 0, subcode: None, }), diff --git a/src/mac/streams/module_list.rs b/src/mac/streams/module_list.rs index 5bdafcc9..2b4d13ea 100644 --- a/src/mac/streams/module_list.rs +++ b/src/mac/streams/module_list.rs @@ -370,7 +370,7 @@ mod test { }; let actual_img_details = mdw - .read_image(actual_img.clone(), &td) + .read_image(*actual_img, &td) .expect("failed to get image details"); let expected_image_name = From 78c36f1f16bd99483f7faf77d885da9dff9b1e2b Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Thu, 21 Jul 2022 15:45:21 +0200 Subject: [PATCH 6/6] Why not 2? --- tests/linux_minidump_writer.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/linux_minidump_writer.rs b/tests/linux_minidump_writer.rs index 72f19062..7c280a94 100644 --- a/tests/linux_minidump_writer.rs +++ b/tests/linux_minidump_writer.rs @@ -335,9 +335,9 @@ fn test_minidump_size_limit() { // Second, write a minidump with a size limit big enough to not trigger // anything. { - // Set size limit arbitrarily 1MB larger than the normal file size -- such + // Set size limit arbitrarily 2MiB larger than the normal file size -- such // that the limiting code will not kick in. - let minidump_size_limit = normal_file_size + 1024 * 1024; + let minidump_size_limit = normal_file_size + 2 * 1024 * 1024; let mut tmpfile = tempfile::Builder::new() .prefix("write_dump_pseudolimited")