From df9102b6cffdab16527a90a2c74d88e59948ca58 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 09:17:43 +0200 Subject: [PATCH 01/21] Add back test now that dump_syms is published --- Cargo.toml | 18 ++--- tests/mac_minidump_writer.rs | 132 +++++++++++++++++------------------ 2 files changed, 71 insertions(+), 79 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 94234d3e..f16c14de 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.2" +crash-context = "0.3" memoffset = "0.6" minidump-common = "0.11" scroll = "0.11" @@ -52,15 +52,11 @@ minidump = "0.11" 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 = "0.0.7", default-features = false } -# minidump-processor = { version = "0.11", default-features = false, features = [ -# "breakpad-syms", -# ] } +# 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 = "0.0.7", default-features = false } +minidump-processor = { version = "0.11", default-features = false, features = [ + "breakpad-syms", +] } similar-asserts = "1.2" uuid = "1.0" - -# [patch.crates-io] -# # PR https://github.com/mozilla/dump_syms/pull/356, merged, but unreleased -# dump_syms = { git = "https://github.com/mozilla/dump_syms", rev = "c2743d5" } # branch = master diff --git a/tests/mac_minidump_writer.rs b/tests/mac_minidump_writer.rs index ab5871f2..e6dc80b4 100644 --- a/tests/mac_minidump_writer.rs +++ b/tests/mac_minidump_writer.rs @@ -138,71 +138,67 @@ fn dump_external_process() { } } -// /// Validates we can actually walk the stack for each thread in the minidump, -// /// this is using minidump-processor, which (currently) depends on breakpad -// /// symbols, however https://github.com/mozilla/dump_syms is not available as -// /// a library https://github.com/mozilla/dump_syms/issues/253, so we just require -// /// that it already be installed, hence the ignore -// #[test] -// fn stackwalks() { -// println!("generating minidump..."); -// let md = capture_minidump("stackwalks", mach2::exception_types::EXC_BREAKPOINT); - -// // Generate the breakpad symbols -// println!("generating symbols..."); -// dump_syms::dumper::single_file( -// &dump_syms::dumper::Config { -// output: dump_syms::dumper::Output::Store(".test-symbols".into()), -// symbol_server: None, -// debug_id: None, -// code_id: None, -// arch: "", -// file_type: dump_syms::common::FileType::Macho, -// num_jobs: 2, // default this -// check_cfi: false, -// mapping_var: None, -// mapping_src: None, -// mapping_dest: None, -// mapping_file: None, -// }, -// "target/debug/test", -// ) -// .expect("failed to dump symbols"); - -// let provider = -// minidump_processor::Symbolizer::new(minidump_processor::simple_symbol_supplier(vec![ -// ".test-symbols".into(), -// ])); - -// let state = futures::executor::block_on(async { -// minidump_processor::process_minidump(&md.minidump, &provider).await -// }) -// .unwrap(); - -// //state.print(&mut std::io::stdout()).map_err(|_| ()).unwrap(); - -// // We expect at least 2 threads, one of which is the fake crashing thread -// let fake_crash_thread = state -// .threads -// .iter() -// .find(|cs| cs.thread_id == md.thread) -// .expect("failed to find crash thread"); - -// // The thread is named, however we currently don't retrieve that information -// // currently, indeed, it appears that you need to retrieve the pthread that -// // corresponds the mach port for a thread, however that API seems to be -// // task specific... -// // assert_eq!( -// // fake_crash_thread.thread_name.as_deref(), -// // Some("test-thread") -// // ); - -// assert!( -// fake_crash_thread.frames.iter().any(|sf| { -// sf.function_name -// .as_ref() -// .map_or(false, |fname| fname.ends_with("wait_until_killed")) -// }), -// "unable to locate expected function" -// ); -// } +/// Validates we can actually walk the stack for each thread in the minidump, +/// this is using minidump-processor, which (currently) depends on breakpad +/// symbols, however https://github.com/mozilla/dump_syms is not available as +/// a library https://github.com/mozilla/dump_syms/issues/253, so we just require +/// that it already be installed, hence the ignore +#[test] +fn stackwalks() { + println!("generating minidump..."); + let md = capture_minidump("stackwalks", mach2::exception_types::EXC_BREAKPOINT); + + // Generate the breakpad symbols + println!("generating symbols..."); + dump_syms::dumper::single_file( + &dump_syms::dumper::Config { + output: dump_syms::dumper::Output::Store(".test-symbols".into()), + symbol_server: None, + debug_id: None, + code_id: None, + arch: "", + file_type: dump_syms::common::FileType::Macho, + num_jobs: 2, // default this + check_cfi: false, + mapping_var: None, + mapping_src: None, + mapping_dest: None, + mapping_file: None, + }, + "target/debug/test", + ) + .expect("failed to dump symbols"); + + let provider = + minidump_processor::Symbolizer::new(minidump_processor::simple_symbol_supplier(vec![ + ".test-symbols".into(), + ])); + + let state = futures::executor::block_on(async { + minidump_processor::process_minidump(&md.minidump, &provider).await + }) + .unwrap(); + + //state.print(&mut std::io::stdout()).map_err(|_| ()).unwrap(); + + // We expect at least 2 threads, one of which is the fake crashing thread + let fake_crash_thread = state + .threads + .iter() + .find(|cs| cs.thread_id == md.thread) + .expect("failed to find crash thread"); + + assert_eq!( + fake_crash_thread.thread_name.as_deref(), + Some("test-thread") + ); + + assert!( + fake_crash_thread.frames.iter().any(|sf| { + sf.function_name + .as_ref() + .map_or(false, |fname| fname.ends_with("wait_until_killed")) + }), + "unable to locate expected function" + ); +} From bdbcd992f5803ab558a664a56169584b7e78fa3b Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 09:17:57 +0200 Subject: [PATCH 02/21] Add macos thread names stream --- src/mac/minidump_writer.rs | 1 + src/mac/streams.rs | 1 + src/mac/streams/thread_names.rs | 88 +++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+) create mode 100644 src/mac/streams/thread_names.rs diff --git a/src/mac/minidump_writer.rs b/src/mac/minidump_writer.rs index 6f68a3f2..b1523906 100644 --- a/src/mac/minidump_writer.rs +++ b/src/mac/minidump_writer.rs @@ -37,6 +37,7 @@ impl MinidumpWriter { Box::new(|mw, buffer, dumper| mw.write_module_list(buffer, dumper)), Box::new(|mw, buffer, dumper| mw.write_misc_info(buffer, dumper)), Box::new(|mw, buffer, dumper| mw.write_breakpad_info(buffer, dumper)), + Box::new(|mw, buffer, dumper| mw.write_thread_names(buffer, dumper)), ]; // Exception stream needs to be the last entry in this array as it may diff --git a/src/mac/streams.rs b/src/mac/streams.rs index e1e6d6a4..21f869b3 100644 --- a/src/mac/streams.rs +++ b/src/mac/streams.rs @@ -5,6 +5,7 @@ mod misc_info; mod module_list; mod system_info; mod thread_list; +mod thread_names; use super::{ errors::WriterError, diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs new file mode 100644 index 00000000..d32ab976 --- /dev/null +++ b/src/mac/streams/thread_names.rs @@ -0,0 +1,88 @@ +use super::*; + +impl MinidumpWriter { + /// Writes the [`MDStreamType::ThreadNamesStream`] which is an array of + /// [`miniduimp_common::format::MINIDUMP_THREAD`] + pub(crate) fn write_thread_names( + &mut self, + buffer: &mut DumpBuf, + dumper: &TaskDumper, + ) -> Result { + let threads = dumper.read_threads()?; + + // Ignore the thread that handled the exception + let thread_count = if self.crash_context.handler_thread != mach2::port::MACH_PORT_NULL { + threads.len() - 1 + } else { + threads.len() + }; + + let list_header = MemoryWriter::::alloc_with_val(buffer, thread_count as u32)?; + + let mut dirent = MDRawDirectory { + stream_type: MDStreamType::ThreadNamesStream as u32, + location: list_header.location(), + }; + + let mut names = MemoryArrayWriter::::alloc_array(buffer, thread_count)?; + dirent.location.data_size += names.location().data_size; + + let handler_thread = self.crash_context.handler_thread; + for (i, tid) in threads + .iter() + .filter(|tid| **tid != handler_thread) + .enumerate() + { + // It's unfortunate if we can't grab a thread name, but it's also + // not a critical failure + let name_loc = match Self::write_thread_name(buffer, *tid) { + Some(loc) => loc, + None => write_string_to_location(buffer, "")?, + }; + + let thread = MDRawThreadName { + thread_id: *tid, + thread_name_rva: name_loc.rva.into(), + }; + + names.set_value_at(buffer, thread, i)?; + } + + Ok(dirent) + } + + /// Attempts to retrieve and write the threadname, returning the threa names + /// location if successful + fn write_thread_name(buffer: &mut Buffer, tid: u32) -> Option { + // SAFETY: syscalls + unsafe { + let mut thread_info = std::mem::MaybeUninit::::uninit(); + let size = std::mem::size_of::() as i32; + if libc::proc_pidinfo( + tid as _, + libc::PROC_PIDTHREADINFO, + 0, + thread_info.as_mut_ptr().cast(), + size, + ) == size + { + let thread_info = thread_info.assume_init(); + let name = std::str::from_utf8(std::slice::from_raw_parts( + thread_info.pth_name.as_ptr().cast(), + thread_info.pth_name.len(), + )) + .ok()?; + + // Ignore the null terminator + let tname = match name.find('\0') { + Some(i) => &name[..i], + None => name, + }; + + write_string_to_location(buffer, tname).ok() + } else { + None + } + } + } +} From b6b6525b64b044ca08508286d17ddb66382c3f53 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 10:05:15 +0200 Subject: [PATCH 03/21] Patch dump_syms to fix issue --- Cargo.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index f16c14de..af838105 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,3 +60,6 @@ minidump-processor = { version = "0.11", default-features = false, features = [ ] } similar-asserts = "1.2" uuid = "1.0" + +[patch.crates-io] +dump_syms = { git = "https://github.com/EmbarkStudios/dump_syms", rev = "6531018" } From 5f762efb5589cdc72a67b2c2bc275934d4b8c8ce Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 10:12:20 +0200 Subject: [PATCH 04/21] Specify target arch --- tests/mac_minidump_writer.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/mac_minidump_writer.rs b/tests/mac_minidump_writer.rs index e6dc80b4..53a73a0e 100644 --- a/tests/mac_minidump_writer.rs +++ b/tests/mac_minidump_writer.rs @@ -156,7 +156,13 @@ fn stackwalks() { symbol_server: None, debug_id: None, code_id: None, - arch: "", + arch: if cfg!(target_arch = "aarch64") { + "aarch64" + } else if cfg!(target_arch = "x86_64") { + "x86_64" + } else { + panic!("invalid MacOS target architecture") + }, file_type: dump_syms::common::FileType::Macho, num_jobs: 2, // default this check_cfi: false, From 285e05cdf98daaa970530c140459f780d49663e5 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 10:43:14 +0200 Subject: [PATCH 05/21] Use 'correct' arch name --- tests/mac_minidump_writer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/mac_minidump_writer.rs b/tests/mac_minidump_writer.rs index 53a73a0e..45ad4494 100644 --- a/tests/mac_minidump_writer.rs +++ b/tests/mac_minidump_writer.rs @@ -157,7 +157,7 @@ fn stackwalks() { debug_id: None, code_id: None, arch: if cfg!(target_arch = "aarch64") { - "aarch64" + "arm64" } else if cfg!(target_arch = "x86_64") { "x86_64" } else { From 8494f4e69c84d48b5cb1237859fbb769b2ed1fcd Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 10:57:07 +0200 Subject: [PATCH 06/21] Debug prints --- src/mac/streams/thread_names.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index d32ab976..3aad3656 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -58,19 +58,19 @@ impl MinidumpWriter { unsafe { let mut thread_info = std::mem::MaybeUninit::::uninit(); let size = std::mem::size_of::() as i32; - if libc::proc_pidinfo( - tid as _, + if dbg!(libc::proc_pidinfo( + dbg!(tid as _), libc::PROC_PIDTHREADINFO, 0, thread_info.as_mut_ptr().cast(), size, - ) == size + )) == size { let thread_info = thread_info.assume_init(); - let name = std::str::from_utf8(std::slice::from_raw_parts( + let name = dbg!(std::str::from_utf8(std::slice::from_raw_parts( thread_info.pth_name.as_ptr().cast(), thread_info.pth_name.len(), - )) + ))) .ok()?; // Ignore the null terminator From 2b8f44199efe8476a7df05e89f55f687e1b7d7f4 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 11:20:30 +0200 Subject: [PATCH 07/21] use mach thread_info instead --- src/mac/mach.rs | 9 ++++++ src/mac/streams/thread_names.rs | 57 ++++++++++++++++++--------------- 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/src/mac/mach.rs b/src/mac/mach.rs index 9212c0ec..4569089b 100644 --- a/src/mac/mach.rs +++ b/src/mac/mach.rs @@ -568,4 +568,13 @@ extern "C" { /// Apple, there is no mention of a replacement function or when/if it might /// eventually disappear. pub fn pid_for_task(task: mach_port_name_t, pid: *mut i32) -> kern_return_t; + + /// Fomr , this retrieves thread info for the + /// for the specified thread. + pub fn thread_info( + thread: mach_port_t, + flavor: i32, + thread_info: *mut i32, + info_size: *mut u32, + ) -> kern_return_t; } diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index 3aad3656..ac8fd647 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -53,36 +53,41 @@ impl MinidumpWriter { /// Attempts to retrieve and write the threadname, returning the threa names /// location if successful - fn write_thread_name(buffer: &mut Buffer, tid: u32) -> Option { + fn write_thread_name( + buffer: &mut Buffer, + tid: u32, + ) -> Result { + const THREAD_INFO_COUNT: u32 = + (std::mem::size_of::() / std::mem::size_of::()) as u32; + // SAFETY: syscalls unsafe { let mut thread_info = std::mem::MaybeUninit::::uninit(); - let size = std::mem::size_of::() as i32; - if dbg!(libc::proc_pidinfo( - dbg!(tid as _), - libc::PROC_PIDTHREADINFO, - 0, + let mut count = THREAD_INFO_COUNT; + + // As noted in usr/include/mach/thread_info.h, the THREAD_EXTENDED_INFO + // return is exactly the same as proc_pidinfo(..., proc_threadinfo) + + mach_call!(mach::thread_info( + tid, + 5, // THREAD_EXTENDED_INFO thread_info.as_mut_ptr().cast(), - size, - )) == size - { - let thread_info = thread_info.assume_init(); - let name = dbg!(std::str::from_utf8(std::slice::from_raw_parts( - thread_info.pth_name.as_ptr().cast(), - thread_info.pth_name.len(), - ))) - .ok()?; - - // Ignore the null terminator - let tname = match name.find('\0') { - Some(i) => &name[..i], - None => name, - }; - - write_string_to_location(buffer, tname).ok() - } else { - None - } + &mut size, + ))?; + + let thread_info = thread_info.assume_init(); + let name = dbg!(std::str::from_utf8(std::slice::from_raw_parts( + thread_info.pth_name.as_ptr().cast(), + thread_info.pth_name.len(), + )))?; + + // Ignore the null terminator + let tname = match name.find('\0') { + Some(i) => &name[..i], + None => name, + }; + + Ok(write_string_to_location(buffer, tname)?) } } } From 184200fb22da426b43665fd0342bea3352127af1 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 11:34:59 +0200 Subject: [PATCH 08/21] Oops --- src/mac/streams/thread_names.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index ac8fd647..d36bf52d 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -36,8 +36,11 @@ impl MinidumpWriter { // It's unfortunate if we can't grab a thread name, but it's also // not a critical failure let name_loc = match Self::write_thread_name(buffer, *tid) { - Some(loc) => loc, - None => write_string_to_location(buffer, "")?, + Ok(loc) => loc, + Err(_err) => { + // TODO: log error + write_string_to_location(buffer, "")? + } }; let thread = MDRawThreadName { @@ -56,7 +59,7 @@ impl MinidumpWriter { fn write_thread_name( buffer: &mut Buffer, tid: u32, - ) -> Result { + ) -> Result { const THREAD_INFO_COUNT: u32 = (std::mem::size_of::() / std::mem::size_of::()) as u32; From cab424b54226642f277c04a7ea35380b2eb79d4c Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 11:50:01 +0200 Subject: [PATCH 09/21] Cleanup --- src/mac/mach.rs | 14 +++++++++- src/mac/streams/thread_names.rs | 48 +++++++++++++-------------------- src/mac/task_dumper.rs | 22 +++++++++++++++ 3 files changed, 54 insertions(+), 30 deletions(-) diff --git a/src/mac/mach.rs b/src/mac/mach.rs index 4569089b..cf1c2077 100644 --- a/src/mac/mach.rs +++ b/src/mac/mach.rs @@ -290,6 +290,15 @@ pub trait TaskInfo { const FLAVOR: u32; } +/// Minimal trait that just pairs a structure that can be filled out by +/// [`thread_info`] with the "flavor" that tells it the info we +/// actually want to retrieve +pub trait ThreadInfo { + /// One of the `THREAD_*` integers. I assume it's very bad if you implement + /// this trait and provide the wrong flavor for the struct + const FLAVOR: u32; +} + /// , the file type for the main executable image pub const MH_EXECUTE: u32 = 0x2; // usr/include/mach-o/loader.h, magic number for MachHeader @@ -571,8 +580,11 @@ extern "C" { /// Fomr , this retrieves thread info for the /// for the specified thread. + /// + /// Note that the info_size parameter is actually the size of the thread_info / 4 + /// as it is the number of words in the thread info pub fn thread_info( - thread: mach_port_t, + thread: u32, flavor: i32, thread_info: *mut i32, info_size: *mut u32, diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index d36bf52d..d097e94b 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -35,7 +35,7 @@ impl MinidumpWriter { { // It's unfortunate if we can't grab a thread name, but it's also // not a critical failure - let name_loc = match Self::write_thread_name(buffer, *tid) { + let name_loc = match Self::write_thread_name(buffer, dumper, *tid) { Ok(loc) => loc, Err(_err) => { // TODO: log error @@ -58,39 +58,29 @@ impl MinidumpWriter { /// location if successful fn write_thread_name( buffer: &mut Buffer, + dumper: &TaskDumper, tid: u32, ) -> Result { - const THREAD_INFO_COUNT: u32 = - (std::mem::size_of::() / std::mem::size_of::()) as u32; - - // SAFETY: syscalls - unsafe { - let mut thread_info = std::mem::MaybeUninit::::uninit(); - let mut count = THREAD_INFO_COUNT; - - // As noted in usr/include/mach/thread_info.h, the THREAD_EXTENDED_INFO - // return is exactly the same as proc_pidinfo(..., proc_threadinfo) + // As noted in usr/include/mach/thread_info.h, the THREAD_EXTENDED_INFO + // return is exactly the same as proc_pidinfo(..., proc_threadinfo) + impl mach::ThreadInfo for libc::proc_threadinfo { + const FLAVOR: u32 = 5; // THREAD_EXTENDED_INFO + } - mach_call!(mach::thread_info( - tid, - 5, // THREAD_EXTENDED_INFO - thread_info.as_mut_ptr().cast(), - &mut size, - ))?; + let thread_info: libc::proc_threadinfo = dumper.thread_info(tid)?; - let thread_info = thread_info.assume_init(); - let name = dbg!(std::str::from_utf8(std::slice::from_raw_parts( - thread_info.pth_name.as_ptr().cast(), - thread_info.pth_name.len(), - )))?; + let name = dbg!(std::str::from_utf8(std::slice::from_raw_parts( + thread_info.pth_name.as_ptr().cast(), + thread_info.pth_name.len(), + ))) + .map_err(|err| TaskDumpError::from(err))?; - // Ignore the null terminator - let tname = match name.find('\0') { - Some(i) => &name[..i], - None => name, - }; + // Ignore the null terminator + let tname = match name.find('\0') { + Some(i) => &name[..i], + None => name, + }; - Ok(write_string_to_location(buffer, tname)?) - } + Ok(write_string_to_location(buffer, tname)?) } } diff --git a/src/mac/task_dumper.rs b/src/mac/task_dumper.rs index 417aaf4d..6d9e9609 100644 --- a/src/mac/task_dumper.rs +++ b/src/mac/task_dumper.rs @@ -287,6 +287,28 @@ impl TaskDumper { unsafe { Ok(info.assume_init()) } } + /// Reads the specified task information. + /// + /// # Errors + /// + /// The syscall to receive the task information failed for some reason, eg. + /// the specified type and the flavor are mismatched and considered invalid, + /// or the thread no longer exists + pub fn thread_info(&self, tid: u32) -> Result { + let mut thread_info = std::mem::MaybeUninit::::uninit(); + let mut count = (std::mem::size_of::() / std::mem::size_of::()) as u32; + + mach_call!(mach::thread_info( + tid, + T::FLAVOR, + thread_info.as_mut_ptr().cast(), + &mut count, + ))?; + + // SAFETY: this will be initialized if the call succeeded + unsafe { Ok(thread_info.assume_init()) } + } + /// Retrieves all of the images loaded in the task. /// /// Note that there may be multiple images with the same load address. From 28b30f29d8ef88935194a7b863fe684cbc2bfefb Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 11:57:51 +0200 Subject: [PATCH 10/21] Get compiling --- src/mac/mach.rs | 2 +- src/mac/streams/thread_names.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mac/mach.rs b/src/mac/mach.rs index cf1c2077..7c9c9bfd 100644 --- a/src/mac/mach.rs +++ b/src/mac/mach.rs @@ -585,7 +585,7 @@ extern "C" { /// as it is the number of words in the thread info pub fn thread_info( thread: u32, - flavor: i32, + flavor: u32, thread_info: *mut i32, info_size: *mut u32, ) -> kern_return_t; diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index d097e94b..1275e602 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -73,7 +73,7 @@ impl MinidumpWriter { thread_info.pth_name.as_ptr().cast(), thread_info.pth_name.len(), ))) - .map_err(|err| TaskDumpError::from(err))?; + .unwrap_or_default(); // Ignore the null terminator let tname = match name.find('\0') { From b24a30b4fec84369d7adf01ff47b80ad648ed02c Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 11:59:30 +0200 Subject: [PATCH 11/21] Oops, add back unsafe --- src/mac/streams/thread_names.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index 1275e602..4e902466 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -69,10 +69,15 @@ impl MinidumpWriter { let thread_info: libc::proc_threadinfo = dumper.thread_info(tid)?; - let name = dbg!(std::str::from_utf8(std::slice::from_raw_parts( - thread_info.pth_name.as_ptr().cast(), - thread_info.pth_name.len(), - ))) + let name = dbg!(std::str::from_utf8( + // SAFETY: This is an initialized block of static size + unsafe { + std::slice::from_raw_parts( + thread_info.pth_name.as_ptr().cast(), + thread_info.pth_name.len(), + ) + } + )) .unwrap_or_default(); // Ignore the null terminator From 614afb52c5ffe501ee1bcbc54b454273f0244567 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 12:04:24 +0200 Subject: [PATCH 12/21] Ensure test is actually testing what I think it is --- tests/mac_minidump_writer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/mac_minidump_writer.rs b/tests/mac_minidump_writer.rs index 45ad4494..ce8259f9 100644 --- a/tests/mac_minidump_writer.rs +++ b/tests/mac_minidump_writer.rs @@ -196,7 +196,7 @@ fn stackwalks() { assert_eq!( fake_crash_thread.thread_name.as_deref(), - Some("test-thread") + Some("test-thread-just-for-giggles") ); assert!( From db7056286e1a4f40923f15ea45c7899ef9dcaa91 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 12:05:18 +0200 Subject: [PATCH 13/21] nice --- tests/mac_minidump_writer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/mac_minidump_writer.rs b/tests/mac_minidump_writer.rs index ce8259f9..45ad4494 100644 --- a/tests/mac_minidump_writer.rs +++ b/tests/mac_minidump_writer.rs @@ -196,7 +196,7 @@ fn stackwalks() { assert_eq!( fake_crash_thread.thread_name.as_deref(), - Some("test-thread-just-for-giggles") + Some("test-thread") ); assert!( From c32a65f6310b0eaaec7679d375ed3f332b1509f0 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 12:08:34 +0200 Subject: [PATCH 14/21] Remove debug print --- src/mac/streams/thread_names.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index 4e902466..4e38f109 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -69,15 +69,15 @@ impl MinidumpWriter { let thread_info: libc::proc_threadinfo = dumper.thread_info(tid)?; - let name = dbg!(std::str::from_utf8( + let name = std::str::from_utf8( // SAFETY: This is an initialized block of static size unsafe { std::slice::from_raw_parts( thread_info.pth_name.as_ptr().cast(), thread_info.pth_name.len(), ) - } - )) + }, + ) .unwrap_or_default(); // Ignore the null terminator From 87c9396d602527a57016397e7cfad94185f64cb3 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 12:21:06 +0200 Subject: [PATCH 15/21] Note PR --- Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.toml b/Cargo.toml index af838105..c33d20f0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -62,4 +62,5 @@ similar-asserts = "1.2" uuid = "1.0" [patch.crates-io] +# PR: https://github.com/mozilla/dump_syms/pull/376 dump_syms = { git = "https://github.com/EmbarkStudios/dump_syms", rev = "6531018" } From b7aed96bf1372481ff211a1af9502f8cde4074c8 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 12:21:13 +0200 Subject: [PATCH 16/21] Update CHANGELOG --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f42ea3fe..004c1887 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - ReleaseDate +### Added +-[PR#32](https://github.com/rust-minidump/minidump-writer/pull/32) resolved [#23](https://github.com/rust-minidump/minidump-writer/issues/23) by adding support for the thread names stream on MacOS. + ## [0.2.0] - 2022-05-23 ### Added - [PR#21](https://github.com/rust-minidump/minidump-writer/pull/21) added an initial implementation for `x86_64-apple-darwin` and `aarch64-apple-darwin` From 4ce010e35b3a465d5c844ebee96bd4a12348927d Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 13:33:24 +0200 Subject: [PATCH 17/21] Address feedback --- src/mac/minidump_writer.rs | 26 ++++++++++++++++++++++++++ src/mac/streams/thread_list.rs | 18 +++--------------- src/mac/streams/thread_names.rs | 18 +++--------------- 3 files changed, 32 insertions(+), 30 deletions(-) diff --git a/src/mac/minidump_writer.rs b/src/mac/minidump_writer.rs index b1523906..e72a8513 100644 --- a/src/mac/minidump_writer.rs +++ b/src/mac/minidump_writer.rs @@ -14,6 +14,8 @@ pub struct MinidumpWriter { /// List of raw blocks of memory we've written into the stream. These are /// referenced by other streams (eg thread list) pub(crate) memory_blocks: Vec, + /// List of threads, minus the handler thread + threads: Vec, } impl MinidumpWriter { @@ -86,4 +88,28 @@ impl MinidumpWriter { Ok(buffer.into()) } + + /// Retrieves the list of active threads in the target process, but removes + /// the handler thread if it is known to simplify dump analysis + #[inline] + fn threads(&mut self, dumper: &TaskDumper) -> &[u32] { + if self.threads.is_empty() { + if let Ok(threads) = dumper.read_threads() { + self.threads = threads.into(); + // Ignore the thread that handled the exception + if self.crash_context.handler_thread != mach2::port::MACH_PORT_NULL { + if let Some(ind) = self + .threads + .iter() + .position(|tid| *tid == self.crash_context.handler_thread) + { + // Preserves order, but not sure if that is actually relevant in most cases + self.threads.remove(ind); + } + } + } + } + + &self.threads + } } diff --git a/src/mac/streams/thread_list.rs b/src/mac/streams/thread_list.rs index fdc40cc3..dcb98c3f 100644 --- a/src/mac/streams/thread_list.rs +++ b/src/mac/streams/thread_list.rs @@ -9,16 +9,9 @@ impl MinidumpWriter { buffer: &mut DumpBuf, dumper: &TaskDumper, ) -> Result { - let threads = dumper.read_threads()?; + let threads = self.threads(dumper); - // Ignore the thread that handled the exception - let thread_count = if self.crash_context.handler_thread != mach2::port::MACH_PORT_NULL { - threads.len() - 1 - } else { - threads.len() - }; - - let list_header = MemoryWriter::::alloc_with_val(buffer, thread_count as u32)?; + let list_header = MemoryWriter::::alloc_with_val(buffer, threads.len() as u32)?; let mut dirent = MDRawDirectory { stream_type: MDStreamType::ThreadListStream as u32, @@ -28,12 +21,7 @@ impl MinidumpWriter { let mut thread_list = MemoryArrayWriter::::alloc_array(buffer, thread_count)?; dirent.location.data_size += thread_list.location().data_size; - let handler_thread = self.crash_context.handler_thread; - for (i, tid) in threads - .iter() - .filter(|tid| **tid != handler_thread) - .enumerate() - { + for (i, tid) in threads.iter().enumerate() { let thread = self.write_thread(*tid, buffer, dumper)?; thread_list.set_value_at(buffer, thread, i)?; } diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index 4e38f109..e9c8ba35 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -8,16 +8,9 @@ impl MinidumpWriter { buffer: &mut DumpBuf, dumper: &TaskDumper, ) -> Result { - let threads = dumper.read_threads()?; + let threads = self.threads(dumper); - // Ignore the thread that handled the exception - let thread_count = if self.crash_context.handler_thread != mach2::port::MACH_PORT_NULL { - threads.len() - 1 - } else { - threads.len() - }; - - let list_header = MemoryWriter::::alloc_with_val(buffer, thread_count as u32)?; + let list_header = MemoryWriter::::alloc_with_val(buffer, threads.len() as u32)?; let mut dirent = MDRawDirectory { stream_type: MDStreamType::ThreadNamesStream as u32, @@ -27,12 +20,7 @@ impl MinidumpWriter { let mut names = MemoryArrayWriter::::alloc_array(buffer, thread_count)?; dirent.location.data_size += names.location().data_size; - let handler_thread = self.crash_context.handler_thread; - for (i, tid) in threads - .iter() - .filter(|tid| **tid != handler_thread) - .enumerate() - { + for (i, tid) in threads.iter().enumerate() { // It's unfortunate if we can't grab a thread name, but it's also // not a critical failure let name_loc = match Self::write_thread_name(buffer, dumper, *tid) { From ab99e10939daea1c664ddf0f71edd11f851b8088 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 13:35:00 +0200 Subject: [PATCH 18/21] Compile --- src/mac/minidump_writer.rs | 3 ++- src/mac/streams/thread_list.rs | 2 +- src/mac/streams/thread_names.rs | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/mac/minidump_writer.rs b/src/mac/minidump_writer.rs index e72a8513..8ced9123 100644 --- a/src/mac/minidump_writer.rs +++ b/src/mac/minidump_writer.rs @@ -24,6 +24,7 @@ impl MinidumpWriter { Self { crash_context, memory_blocks: Vec::new(), + threads: Vec::new(), } } @@ -92,7 +93,7 @@ impl MinidumpWriter { /// Retrieves the list of active threads in the target process, but removes /// the handler thread if it is known to simplify dump analysis #[inline] - fn threads(&mut self, dumper: &TaskDumper) -> &[u32] { + pub(crate) fn threads(&mut self, dumper: &TaskDumper) -> &[u32] { if self.threads.is_empty() { if let Ok(threads) = dumper.read_threads() { self.threads = threads.into(); diff --git a/src/mac/streams/thread_list.rs b/src/mac/streams/thread_list.rs index dcb98c3f..dc897e6f 100644 --- a/src/mac/streams/thread_list.rs +++ b/src/mac/streams/thread_list.rs @@ -18,7 +18,7 @@ impl MinidumpWriter { location: list_header.location(), }; - let mut thread_list = MemoryArrayWriter::::alloc_array(buffer, thread_count)?; + let mut thread_list = MemoryArrayWriter::::alloc_array(buffer, threads.len())?; dirent.location.data_size += thread_list.location().data_size; for (i, tid) in threads.iter().enumerate() { diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index e9c8ba35..0e03776c 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -17,7 +17,7 @@ impl MinidumpWriter { location: list_header.location(), }; - let mut names = MemoryArrayWriter::::alloc_array(buffer, thread_count)?; + let mut names = MemoryArrayWriter::::alloc_array(buffer, threads.len())?; dirent.location.data_size += names.location().data_size; for (i, tid) in threads.iter().enumerate() { From 036a868b6f4e37a8fadb5c49d4228a1f788a4948 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 13:54:27 +0200 Subject: [PATCH 19/21] Fixup --- src/mac/minidump_writer.rs | 62 ++++++++++++++++++++++----------- src/mac/streams/thread_list.rs | 7 ++-- src/mac/streams/thread_names.rs | 6 ++-- 3 files changed, 48 insertions(+), 27 deletions(-) diff --git a/src/mac/minidump_writer.rs b/src/mac/minidump_writer.rs index 8ced9123..8d31cc7c 100644 --- a/src/mac/minidump_writer.rs +++ b/src/mac/minidump_writer.rs @@ -14,8 +14,6 @@ pub struct MinidumpWriter { /// List of raw blocks of memory we've written into the stream. These are /// referenced by other streams (eg thread list) pub(crate) memory_blocks: Vec, - /// List of threads, minus the handler thread - threads: Vec, } impl MinidumpWriter { @@ -24,7 +22,6 @@ impl MinidumpWriter { Self { crash_context, memory_blocks: Vec::new(), - threads: Vec::new(), } } @@ -90,27 +87,50 @@ impl MinidumpWriter { Ok(buffer.into()) } - /// Retrieves the list of active threads in the target process, but removes - /// the handler thread if it is known to simplify dump analysis + /// Retrieves the list of active threads in the target process, except + /// the handler thread if it is known, to simplify dump analysis #[inline] - pub(crate) fn threads(&mut self, dumper: &TaskDumper) -> &[u32] { - if self.threads.is_empty() { - if let Ok(threads) = dumper.read_threads() { - self.threads = threads.into(); - // Ignore the thread that handled the exception - if self.crash_context.handler_thread != mach2::port::MACH_PORT_NULL { - if let Some(ind) = self - .threads - .iter() - .position(|tid| *tid == self.crash_context.handler_thread) - { - // Preserves order, but not sure if that is actually relevant in most cases - self.threads.remove(ind); - } - } + pub(crate) fn threads(&self, dumper: &TaskDumper) -> ActiveThreads { + ActiveThreads { + threads: dumper.read_threads().unwrap_or_default(), + handler_thread: self.crash_context.handler_thread, + i: 0, + } + } +} + +pub(crate) struct ActiveThreads { + threads: &'static [u32], + handler_thread: u32, + i: usize, +} + +impl ActiveThreads { + #[inline] + pub(crate) fn count(&self) -> usize { + let mut len = self.threads.len(); + + if self.handler_thread != mach2::port::MACH_PORT_NULL { + len -= 1; + } + + len + } +} + +impl Iterator for ActiveThreads { + type Item = u32; + + fn next(&mut self) -> Option { + while self.i < self.threads.len() { + let i = self.i; + self.i += 1; + + if self.threads[i] != self.handler_thread { + return Some(self.threads[i]); } } - &self.threads + None } } diff --git a/src/mac/streams/thread_list.rs b/src/mac/streams/thread_list.rs index dc897e6f..3404f0ae 100644 --- a/src/mac/streams/thread_list.rs +++ b/src/mac/streams/thread_list.rs @@ -11,17 +11,18 @@ impl MinidumpWriter { ) -> Result { let threads = self.threads(dumper); - let list_header = MemoryWriter::::alloc_with_val(buffer, threads.len() as u32)?; + let list_header = MemoryWriter::::alloc_with_val(buffer, threads.count() as u32)?; let mut dirent = MDRawDirectory { stream_type: MDStreamType::ThreadListStream as u32, location: list_header.location(), }; - let mut thread_list = MemoryArrayWriter::::alloc_array(buffer, threads.len())?; + let mut thread_list = + MemoryArrayWriter::::alloc_array(buffer, threads.count())?; dirent.location.data_size += thread_list.location().data_size; - for (i, tid) in threads.iter().enumerate() { + for (i, tid) in threads.enumerate() { let thread = self.write_thread(*tid, buffer, dumper)?; thread_list.set_value_at(buffer, thread, i)?; } diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index 0e03776c..8fe89add 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -10,17 +10,17 @@ impl MinidumpWriter { ) -> Result { let threads = self.threads(dumper); - let list_header = MemoryWriter::::alloc_with_val(buffer, threads.len() as u32)?; + let list_header = MemoryWriter::::alloc_with_val(buffer, threads.count() as u32)?; let mut dirent = MDRawDirectory { stream_type: MDStreamType::ThreadNamesStream as u32, location: list_header.location(), }; - let mut names = MemoryArrayWriter::::alloc_array(buffer, threads.len())?; + let mut names = MemoryArrayWriter::::alloc_array(buffer, threads.count())?; dirent.location.data_size += names.location().data_size; - for (i, tid) in threads.iter().enumerate() { + for (i, tid) in threads.enumerate() { // It's unfortunate if we can't grab a thread name, but it's also // not a critical failure let name_loc = match Self::write_thread_name(buffer, dumper, *tid) { From a5f78997b77c47ed4380e6ec276b743a08475092 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 13:55:11 +0200 Subject: [PATCH 20/21] again --- src/mac/streams/thread_list.rs | 2 +- src/mac/streams/thread_names.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mac/streams/thread_list.rs b/src/mac/streams/thread_list.rs index 3404f0ae..625ee377 100644 --- a/src/mac/streams/thread_list.rs +++ b/src/mac/streams/thread_list.rs @@ -23,7 +23,7 @@ impl MinidumpWriter { dirent.location.data_size += thread_list.location().data_size; for (i, tid) in threads.enumerate() { - let thread = self.write_thread(*tid, buffer, dumper)?; + let thread = self.write_thread(tid, buffer, dumper)?; thread_list.set_value_at(buffer, thread, i)?; } diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index 8fe89add..54b44cf8 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -23,7 +23,7 @@ impl MinidumpWriter { for (i, tid) in threads.enumerate() { // It's unfortunate if we can't grab a thread name, but it's also // not a critical failure - let name_loc = match Self::write_thread_name(buffer, dumper, *tid) { + let name_loc = match Self::write_thread_name(buffer, dumper, tid) { Ok(loc) => loc, Err(_err) => { // TODO: log error @@ -32,7 +32,7 @@ impl MinidumpWriter { }; let thread = MDRawThreadName { - thread_id: *tid, + thread_id: tid, thread_name_rva: name_loc.rva.into(), }; From 1a9f164895f8efc6c983c95e82a3d256095aa89d Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Wed, 25 May 2022 13:56:14 +0200 Subject: [PATCH 21/21] Confused type inference apparently --- src/mac/minidump_writer.rs | 2 +- src/mac/streams/thread_list.rs | 5 ++--- src/mac/streams/thread_names.rs | 4 ++-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/mac/minidump_writer.rs b/src/mac/minidump_writer.rs index 8d31cc7c..aed3dcbe 100644 --- a/src/mac/minidump_writer.rs +++ b/src/mac/minidump_writer.rs @@ -107,7 +107,7 @@ pub(crate) struct ActiveThreads { impl ActiveThreads { #[inline] - pub(crate) fn count(&self) -> usize { + pub(crate) fn len(&self) -> usize { let mut len = self.threads.len(); if self.handler_thread != mach2::port::MACH_PORT_NULL { diff --git a/src/mac/streams/thread_list.rs b/src/mac/streams/thread_list.rs index 625ee377..180bb2f6 100644 --- a/src/mac/streams/thread_list.rs +++ b/src/mac/streams/thread_list.rs @@ -11,15 +11,14 @@ impl MinidumpWriter { ) -> Result { let threads = self.threads(dumper); - let list_header = MemoryWriter::::alloc_with_val(buffer, threads.count() as u32)?; + let list_header = MemoryWriter::::alloc_with_val(buffer, threads.len() as u32)?; let mut dirent = MDRawDirectory { stream_type: MDStreamType::ThreadListStream as u32, location: list_header.location(), }; - let mut thread_list = - MemoryArrayWriter::::alloc_array(buffer, threads.count())?; + let mut thread_list = MemoryArrayWriter::::alloc_array(buffer, threads.len())?; dirent.location.data_size += thread_list.location().data_size; for (i, tid) in threads.enumerate() { diff --git a/src/mac/streams/thread_names.rs b/src/mac/streams/thread_names.rs index 54b44cf8..42242a63 100644 --- a/src/mac/streams/thread_names.rs +++ b/src/mac/streams/thread_names.rs @@ -10,14 +10,14 @@ impl MinidumpWriter { ) -> Result { let threads = self.threads(dumper); - let list_header = MemoryWriter::::alloc_with_val(buffer, threads.count() as u32)?; + let list_header = MemoryWriter::::alloc_with_val(buffer, threads.len() as u32)?; let mut dirent = MDRawDirectory { stream_type: MDStreamType::ThreadNamesStream as u32, location: list_header.location(), }; - let mut names = MemoryArrayWriter::::alloc_array(buffer, threads.count())?; + let mut names = MemoryArrayWriter::::alloc_array(buffer, threads.len())?; dirent.location.data_size += names.location().data_size; for (i, tid) in threads.enumerate() {