From 2b2a9ebfde4508d69269ed2d1ea9effaa311a9f2 Mon Sep 17 00:00:00 2001 From: Gabriele Svelto Date: Fri, 23 Sep 2022 14:08:37 +0200 Subject: [PATCH 1/3] Remove unnecessary unsafe blocks --- src/windows/minidump_writer.rs | 2 +- tests/windows_minidump_writer.rs | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/windows/minidump_writer.rs b/src/windows/minidump_writer.rs index 8bbcd79f..e4a462c4 100644 --- a/src/windows/minidump_writer.rs +++ b/src/windows/minidump_writer.rs @@ -145,7 +145,7 @@ impl MinidumpWriter { /// If [`crash_context::CrashContext::exception_pointers`] is specified, it /// is the responsibility of the caller to ensure that the pointer is valid /// for the duration of this function call. - pub unsafe fn dump_crash_context( + pub fn dump_crash_context( crash_context: crash_context::CrashContext, destination: &mut std::fs::File, ) -> Result<(), Error> { diff --git a/tests/windows_minidump_writer.rs b/tests/windows_minidump_writer.rs index 4ee2306b..c0b32168 100644 --- a/tests/windows_minidump_writer.rs +++ b/tests/windows_minidump_writer.rs @@ -152,10 +152,8 @@ fn dump_external_process() { // SAFETY: We keep the process we are dumping alive until the minidump is written // and the test process keep the pointers it sent us alive until it is killed - unsafe { - MinidumpWriter::dump_crash_context(crash_context, tmpfile.as_file_mut()) - .expect("failed to write minidump"); - } + MinidumpWriter::dump_crash_context(crash_context, tmpfile.as_file_mut()) + .expect("failed to write minidump"); child.kill().expect("failed to kill child"); From 0d545542ba9feedeb40ea9a88219a3be7dcb8934 Mon Sep 17 00:00:00 2001 From: Gabriele Svelto Date: Fri, 23 Sep 2022 15:38:47 +0200 Subject: [PATCH 2/3] Remove needless lazy evaluation --- src/windows/minidump_writer.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/windows/minidump_writer.rs b/src/windows/minidump_writer.rs index e4a462c4..f82c55fa 100644 --- a/src/windows/minidump_writer.rs +++ b/src/windows/minidump_writer.rs @@ -174,7 +174,7 @@ impl MinidumpWriter { let tid = crash_context.thread_id; let exception_code = crash_context.exception_code; - let exc_info = (!crash_context.exception_pointers.is_null()).then(|| + let exc_info = (!crash_context.exception_pointers.is_null()).then_some( // https://docs.microsoft.com/en-us/windows/win32/api/minidumpapiset/ns-minidumpapiset-minidump_exception_information md::MINIDUMP_EXCEPTION_INFORMATION { ThreadId: crash_context.thread_id, @@ -189,7 +189,8 @@ impl MinidumpWriter { /// it can use eg `ReadProcessMemory` to get the contextual information from /// the crash, rather than from the current process ClientPointers: if is_external_process { 1 } else { 0 }, - }); + }, + ); let mdw = Self { exc_info, From dc6e8f6fab2909f376e3126350b16626bebef947 Mon Sep 17 00:00:00 2001 From: Gabriele Svelto Date: Fri, 23 Sep 2022 14:17:27 +0200 Subject: [PATCH 3/3] Remove custom HANDLE stream --- Cargo.toml | 2 - src/windows/minidump_writer.rs | 115 ++------------------------------- 2 files changed, 5 insertions(+), 112 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 18376c23..37b4e085 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,8 +40,6 @@ features = [ "Win32_System_Diagnostics_Debug", "Win32_System_Kernel", "Win32_System_Memory", - # VerifierEnumerateResource and friends - "Win32_System_ApplicationVerifier", # GetCurrentThreadId & OpenProcess "Win32_System_Threading", ] diff --git a/src/windows/minidump_writer.rs b/src/windows/minidump_writer.rs index f82c55fa..68938f2e 100644 --- a/src/windows/minidump_writer.rs +++ b/src/windows/minidump_writer.rs @@ -1,13 +1,11 @@ use crate::windows::errors::Error; use minidump_common::format::{BreakpadInfoValid, MINIDUMP_BREAKPAD_INFO, MINIDUMP_STREAM_TYPE}; use scroll::Pwrite; -use std::{ffi::c_void, os::windows::io::AsRawHandle}; +use std::os::windows::io::AsRawHandle; pub use windows_sys::Win32::Foundation::HANDLE; use windows_sys::Win32::{ - Foundation::{ - CloseHandle, ERROR_SUCCESS, STATUS_INVALID_HANDLE, STATUS_NONCONTINUABLE_EXCEPTION, - }, - System::{ApplicationVerifier as av, Diagnostics::Debug as md, Threading as threading}, + Foundation::{CloseHandle, STATUS_NONCONTINUABLE_EXCEPTION}, + System::{Diagnostics::Debug as md, Threading as threading}, }; pub struct MinidumpWriter { @@ -20,6 +18,7 @@ pub struct MinidumpWriter { /// The id of the 'crashing' thread tid: u32, /// The exception code for the dump + #[allow(dead_code)] exception_code: i32, /// Whether we are dumping the current process or not is_external_process: bool, @@ -208,7 +207,7 @@ impl MinidumpWriter { fn dump(mut self, destination: &mut std::fs::File) -> Result<(), Error> { let exc_info = self.exc_info.take(); - let mut user_streams = Vec::with_capacity(2); + let mut user_streams = Vec::with_capacity(1); let mut breakpad_info = self.fill_breakpad_stream(); @@ -221,26 +220,6 @@ impl MinidumpWriter { }); } - let mut handle_stream_buffer = if self.exception_code == STATUS_INVALID_HANDLE { - self.fill_handle_stream() - } else { - None - }; - - // Note that we do this by ref, as the buffer inside the option needs - // to stay alive for as long as we're writing the minidump since - // the user stream has a pointer to it - if let Some(buf) = &mut handle_stream_buffer { - let handle_stream = md::MINIDUMP_USER_STREAM { - Type: MINIDUMP_STREAM_TYPE::HandleOperationListStream as u32, - BufferSize: buf.len() as u32, - // Still not getting over the mut pointers here - Buffer: buf.as_mut_ptr().cast(), - }; - - user_streams.push(handle_stream); - } - let user_stream_infos = md::MINIDUMP_USER_STREAM_INFORMATION { UserStreamCount: user_streams.len() as u32, UserStreamArray: user_streams.as_mut_ptr(), @@ -306,90 +285,6 @@ impl MinidumpWriter { Some(breakpad_info) } - - /// In the case of a `STATUS_INVALID_HANDLE` exception, this function - /// enumerates all of the handle operations that occurred within the crashing - /// process and fills out a minidump user stream with the ops pertaining to - /// the last invalid handle that is enumerated, which is, presumably - /// (hopefully?), the one that led to the exception - fn fill_handle_stream(&self) -> Option> { - // State object we pass to the enumeration - struct HandleState { - ops: Vec, - last_invalid: u64, - } - - unsafe extern "system" fn enum_callback( - resource_description: *mut c_void, - enumeration_context: *mut c_void, - enumeration_level: *mut u32, - ) -> u32 { - let description = &*resource_description.cast::(); - let mut hs = &mut *enumeration_context.cast::(); - - // Remember the last invalid handle operation. - if description.OperationType == av::OperationDbBADREF as u32 { - hs.last_invalid = description.Handle; - } - - // Record all handle operations. - hs.ops.push(*description); - *enumeration_level = av::HeapEnumerationEverything as u32; - ERROR_SUCCESS - } - - let mut hs = HandleState { - ops: Vec::new(), - last_invalid: 0, - }; - - // https://docs.microsoft.com/en-us/windows/win32/api/avrfsdk/nf-avrfsdk-verifierenumerateresource - // SAFETY: syscall - if unsafe { - av::VerifierEnumerateResource( - self.crashing_process, // process to enumerate the handles for - 0, // flags - av::AvrfResourceHandleTrace, // resource typea, we want to trace handles - Some(enum_callback), // enumeration callback - (&mut hs as *mut HandleState).cast(), // enumeration context - ) - } == ERROR_SUCCESS - { - let mut stream_buf = Vec::new(); - - // https://docs.microsoft.com/en-us/windows/win32/api/minidumpapiset/ns-minidumpapiset-minidump_handle_operation_list - let mut md_list = md::MINIDUMP_HANDLE_OPERATION_LIST { - SizeOfHeader: std::mem::size_of::() as u32, - SizeOfEntry: std::mem::size_of::() as u32, - NumberOfEntries: 0, - Reserved: 0, - }; - - stream_buf.resize(md_list.SizeOfHeader as usize, 0); - - #[inline] - fn to_bytes(v: &T) -> &[u8] { - // SAFETY: both AVRF_HANDLE_OPERATION and MINIDUMP_HANDLE_OPERATION_LIST - // are POD types - unsafe { - std::slice::from_raw_parts((v as *const T).cast(), std::mem::size_of::()) - } - } - - for op in hs.ops.into_iter().filter(|op| op.Handle == hs.last_invalid) { - stream_buf.extend_from_slice(to_bytes(&op)); - md_list.NumberOfEntries += 1; - } - - stream_buf[..md_list.SizeOfHeader as usize].copy_from_slice(to_bytes(&md_list)); - - Some(stream_buf) - } else { - // We don't _particularly_ care if this fails, it's better if we had - // the info, but not critical - None - } - } } impl Drop for MinidumpWriter {