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` diff --git a/Cargo.toml b/Cargo.toml index 94234d3e..c33d20f0 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,15 @@ 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 +[patch.crates-io] +# PR: https://github.com/mozilla/dump_syms/pull/376 +dump_syms = { git = "https://github.com/EmbarkStudios/dump_syms", rev = "6531018" } diff --git a/src/mac/mach.rs b/src/mac/mach.rs index 9212c0ec..7c9c9bfd 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 @@ -568,4 +577,16 @@ 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. + /// + /// 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: u32, + flavor: u32, + thread_info: *mut i32, + info_size: *mut u32, + ) -> kern_return_t; } diff --git a/src/mac/minidump_writer.rs b/src/mac/minidump_writer.rs index 6f68a3f2..aed3dcbe 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 @@ -85,4 +86,51 @@ impl MinidumpWriter { Ok(buffer.into()) } + + /// 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(&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 len(&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]); + } + } + + None + } } 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_list.rs b/src/mac/streams/thread_list.rs index fdc40cc3..180bb2f6 100644 --- a/src/mac/streams/thread_list.rs +++ b/src/mac/streams/thread_list.rs @@ -9,32 +9,20 @@ 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, 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; - let handler_thread = self.crash_context.handler_thread; - for (i, tid) in threads - .iter() - .filter(|tid| **tid != handler_thread) - .enumerate() - { - let thread = self.write_thread(*tid, buffer, dumper)?; + 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 new file mode 100644 index 00000000..42242a63 --- /dev/null +++ b/src/mac/streams/thread_names.rs @@ -0,0 +1,79 @@ +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 = self.threads(dumper); + + 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.len())?; + dirent.location.data_size += names.location().data_size; + + 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) { + Ok(loc) => loc, + Err(_err) => { + // TODO: log error + 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, + dumper: &TaskDumper, + tid: u32, + ) -> Result { + // 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 + } + + let thread_info: libc::proc_threadinfo = dumper.thread_info(tid)?; + + 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 + let tname = match name.find('\0') { + Some(i) => &name[..i], + None => name, + }; + + 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. diff --git a/tests/mac_minidump_writer.rs b/tests/mac_minidump_writer.rs index ab5871f2..45ad4494 100644 --- a/tests/mac_minidump_writer.rs +++ b/tests/mac_minidump_writer.rs @@ -138,71 +138,73 @@ 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: if cfg!(target_arch = "aarch64") { + "arm64" + } 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, + 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" + ); +}