Skip to content

Commit

Permalink
Reduce panics in dbghelp (#608)
Browse files Browse the repository at this point in the history 
According to the docs for `resolve`:

> The closure may not be called if resolution could not be performed
> This function strives to never panic.

So we should ideally not panic and instead simply return without calling
the closure. Therefore, I've changed the panicky FFI calls to instead
simply return.

I also replaced our custom UTF-16 to UTF-8 converter with a call to
`WideCharToMultiByte`. This function already has the semantics we want
and doesn't include panicking code.

These changes should have a small but notable effect on binary size.
  • Loading branch information
@ChrisDenton
ChrisDenton committed May 7, 2024
1 parent f63b581 commit 7d26334
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 46 deletions.
2 changes: 2 additions & 0 deletions Cargo.toml
View file
Expand Up @@ -93,6 +93,8 @@ verify-winapi = [
'winapi/tlhelp32',
'winapi/winbase',
'winapi/winnt',
'winapi/winnls',
'winapi/stringapiset',
]

[[example]]
Expand Down
27 changes: 15 additions & 12 deletions src/dbghelp.rs
View file
Expand Up @@ -376,16 +376,21 @@ pub fn init() -> Result<Init, ()> {
DBGHELP.ensure_open()?;

static mut INITIALIZED: bool = false;
if INITIALIZED {
return Ok(ret);
if !INITIALIZED {
set_optional_options();
INITIALIZED = true;
}

let orig = DBGHELP.SymGetOptions().unwrap()();
Ok(ret)
}
}
fn set_optional_options() -> Option<()> {
unsafe {
let orig = DBGHELP.SymGetOptions()?();

// Ensure that the `SYMOPT_DEFERRED_LOADS` flag is set, because
// according to MSVC's own docs about this: "This is the fastest, most
// efficient way to use the symbol handler.", so let's do that!
DBGHELP.SymSetOptions().unwrap()(orig | SYMOPT_DEFERRED_LOADS);
DBGHELP.SymSetOptions()?(orig | SYMOPT_DEFERRED_LOADS);

// Actually initialize symbols with MSVC. Note that this can fail, but we
// ignore it. There's not a ton of prior art for this per se, but LLVM
Expand All @@ -399,7 +404,7 @@ pub fn init() -> Result<Init, ()> {
// the time, but now that it's using this crate it means that someone will
// get to initialization first and the other will pick up that
// initialization.
DBGHELP.SymInitializeW().unwrap()(GetCurrentProcess(), ptr::null_mut(), TRUE);
DBGHELP.SymInitializeW()?(GetCurrentProcess(), ptr::null_mut(), TRUE);

// The default search path for dbghelp will only look in the current working
// directory and (possibly) `_NT_SYMBOL_PATH` and `_NT_ALT_SYMBOL_PATH`.
Expand All @@ -413,7 +418,7 @@ pub fn init() -> Result<Init, ()> {
search_path_buf.resize(1024, 0);

// Prefill the buffer with the current search path.
if DBGHELP.SymGetSearchPathW().unwrap()(
if DBGHELP.SymGetSearchPathW()?(
GetCurrentProcess(),
search_path_buf.as_mut_ptr(),
search_path_buf.len() as _,
Expand All @@ -433,7 +438,7 @@ pub fn init() -> Result<Init, ()> {
let mut search_path = SearchPath::new(search_path_buf);

// Update the search path to include the directory of the executable and each DLL.
DBGHELP.EnumerateLoadedModulesW64().unwrap()(
DBGHELP.EnumerateLoadedModulesW64()?(
GetCurrentProcess(),
Some(enum_loaded_modules_callback),
((&mut search_path) as *mut SearchPath) as *mut c_void,
Expand All @@ -442,11 +447,9 @@ pub fn init() -> Result<Init, ()> {
let new_search_path = search_path.finalize();

// Set the new search path.
DBGHELP.SymSetSearchPathW().unwrap()(GetCurrentProcess(), new_search_path.as_ptr());

INITIALIZED = true;
Ok(ret)
DBGHELP.SymSetSearchPathW()?(GetCurrentProcess(), new_search_path.as_ptr());
}
Some(())
}

struct SearchPath {
Expand Down
66 changes: 32 additions & 34 deletions src/symbolize/dbghelp.rs
View file
Expand Up @@ -19,7 +19,6 @@

use super::super::{dbghelp, windows::*};
use super::{BytesOrWideString, ResolveWhat, SymbolName};
use core::char;
use core::ffi::c_void;
use core::marker;
use core::mem;
Expand Down Expand Up @@ -91,7 +90,7 @@ pub unsafe fn resolve(what: ResolveWhat<'_>, cb: &mut dyn FnMut(&super::Symbol))
ResolveWhat::Frame(frame) => {
resolve_with_inline(&dbghelp, frame.ip(), frame.inner.inline_context(), cb)
}
}
};
}

#[cfg(target_vendor = "win7")]
Expand All @@ -116,7 +115,7 @@ pub unsafe fn resolve(what: ResolveWhat<'_>, cb: &mut dyn FnMut(&super::Symbol))
ResolveWhat::Frame(frame) => {
resolve_inner(&dbghelp, frame.ip(), frame.inner.inline_context(), cb)
}
}
};
}

/// Resolve the address using the legacy dbghelp API.
Expand Down Expand Up @@ -147,22 +146,28 @@ unsafe fn resolve_with_inline(
addr: *mut c_void,
inline_context: Option<DWORD>,
cb: &mut dyn FnMut(&super::Symbol),
) {
) -> Option<()> {
let current_process = GetCurrentProcess();
// Ensure we have the functions we need. Return if any aren't found.
let SymFromInlineContextW = (*dbghelp.dbghelp()).SymFromInlineContextW()?;
let SymGetLineFromInlineContextW = (*dbghelp.dbghelp()).SymGetLineFromInlineContextW()?;

let addr = super::adjust_ip(addr) as DWORD64;

let (inlined_frame_count, inline_context) = if let Some(ic) = inline_context {
(0, ic)
} else {
let mut inlined_frame_count = dbghelp.SymAddrIncludeInlineTrace()(current_process, addr);
let SymAddrIncludeInlineTrace = (*dbghelp.dbghelp()).SymAddrIncludeInlineTrace()?;
let SymQueryInlineTrace = (*dbghelp.dbghelp()).SymQueryInlineTrace()?;

let mut inlined_frame_count = SymAddrIncludeInlineTrace(current_process, addr);

let mut inline_context = 0;

// If there is are inlined frames but we can't load them for some reason OR if there are no
// inlined frames, then we disregard inlined_frame_count and inline_context.
if (inlined_frame_count > 0
&& dbghelp.SymQueryInlineTrace()(
&& SymQueryInlineTrace(
current_process,
addr,
0,
Expand All @@ -184,22 +189,14 @@ unsafe fn resolve_with_inline(

for inline_context in inline_context..last_inline_context {
do_resolve(
|info| {
dbghelp.SymFromInlineContextW()(current_process, addr, inline_context, &mut 0, info)
},
|info| SymFromInlineContextW(current_process, addr, inline_context, &mut 0, info),
|line| {
dbghelp.SymGetLineFromInlineContextW()(
current_process,
addr,
inline_context,
0,
&mut 0,
line,
)
SymGetLineFromInlineContextW(current_process, addr, inline_context, 0, &mut 0, line)
},
cb,
);
}
Some(())
}

unsafe fn do_resolve(
Expand All @@ -225,26 +222,27 @@ unsafe fn do_resolve(
// the real value.
let name_len = ::core::cmp::min(info.NameLen as usize, info.MaxNameLen as usize - 1);
let name_ptr = info.Name.as_ptr().cast::<u16>();
let name = slice::from_raw_parts(name_ptr, name_len);

// Reencode the utf-16 symbol to utf-8 so we can use `SymbolName::new` like
// all other platforms
let mut name_len = 0;
let mut name_buffer = [0; 256];
{
let mut remaining = &mut name_buffer[..];
for c in char::decode_utf16(name.iter().cloned()) {
let c = c.unwrap_or(char::REPLACEMENT_CHARACTER);
let len = c.len_utf8();
if len < remaining.len() {
c.encode_utf8(remaining);
let tmp = remaining;
remaining = &mut tmp[len..];
name_len += len;
} else {
break;
}
}
let mut name_buffer = [0_u8; 256];
let mut name_len = WideCharToMultiByte(
CP_UTF8,
0,
name_ptr,
name_len as i32,
name_buffer.as_mut_ptr().cast::<i8>(),
name_buffer.len() as i32,
core::ptr::null_mut(),
core::ptr::null_mut(),
) as usize;
if name_len == 0 {
// If the returned length is zero that means the buffer wasn't big enough.
// However, the buffer will be filled with as much as will fit.
name_len = name_buffer.len();
} else if name_len > name_buffer.len() {
// This can't happen.
return;
}
let name = ptr::addr_of!(name_buffer[..name_len]);

Expand Down
13 changes: 13 additions & 0 deletions src/windows.rs
View file
Expand Up @@ -38,6 +38,8 @@ cfg_if::cfg_if! {
pub use winapi::um::tlhelp32::*;
pub use winapi::um::winbase::*;
pub use winapi::um::winnt::*;
pub use winapi::um::winnls::*;
pub use winapi::um::stringapiset::*;

// Work around winapi not having this function on aarch64.
#[cfg(target_arch = "aarch64")]
Expand Down Expand Up @@ -379,6 +381,7 @@ ffi! {
pub const INVALID_HANDLE_VALUE: HANDLE = -1isize as HANDLE;
pub const MAX_MODULE_NAME32: usize = 255;
pub const MAX_PATH: usize = 260;
pub const CP_UTF8: u32 = 65001;

pub type DWORD = u32;
pub type PDWORD = *mut u32;
Expand Down Expand Up @@ -456,6 +459,16 @@ ffi! {
lpme: LPMODULEENTRY32W,
) -> BOOL;
pub fn lstrlenW(lpstring: PCWSTR) -> i32;
pub fn WideCharToMultiByte(
codepage: u32,
dwflags: u32,
lpwidecharstr: PCWSTR,
cchwidechar: i32,
lpmultibytestr: *mut i8,
cbmultibyte: i32,
lpdefaultchar: *const i8,
lpuseddefaultchar: *mut BOOL
) -> i32;
}
}

Expand Down

0 comments on commit 7d26334

Please sign in to comment.