From e42c4e9a5b81343452f10b4bd92f5c8590122624 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 4 May 2021 08:17:56 -0700 Subject: [PATCH 1/3] Remove libbacktrace implementation This commit removes the `backtrace-sys` crate and the libbacktrace implementation of this crate. For quite some time now the `gimli-symbolize` feture has been on-by-default. While `libbacktrace` has been an option all relevant features should have been implemented in `gimli-symbolize` by now. The libbacktrace implementation has always been approached with wariness where it may segfault or cause worse behavior when fed bad debug information. Debug information is possible to change at runtime, so libbacktrace has always been a risk for Rust binaries. Additionally libbacktrace development was very quite for a very long time and our patches upstream were generally met with silence. Development seems to have picked back up upstream but with this crate now switched to gimli I'm not too personally motivated to check to see if all our fixes have landed. In general, though, libbacktrace upstream always worked best with Linux and other platforms were more "best-effort". Additionally gimli now has more features for supporting compressed and split-debuginfo as well. This commit comes about to reduce the maintenance burden on this crate. Recent changes in rust-lang/rust have actually broken libbacktrace testing. This appears fixed with sync'ing to the upstream repository of libbacktrace, but it seems like now's the right time to go ahead and remove the crate. --- .github/workflows/main.yml | 21 +- Cargo.toml | 31 +- crates/backtrace-sys/Cargo.toml | 27 -- crates/backtrace-sys/LICENSE-APACHE | 1 - crates/backtrace-sys/LICENSE-MIT | 1 - crates/backtrace-sys/build.rs | 168 --------- crates/backtrace-sys/src/android-api.c | 4 - crates/backtrace-sys/src/lib.rs | 58 ---- crates/backtrace-sys/src/libbacktrace | 1 - src/symbolize/libbacktrace.rs | 460 ------------------------- src/symbolize/mod.rs | 11 - tests/accuracy/main.rs | 2 - tests/smoke.rs | 92 ++--- 13 files changed, 32 insertions(+), 845 deletions(-) delete mode 100644 crates/backtrace-sys/Cargo.toml delete mode 120000 crates/backtrace-sys/LICENSE-APACHE delete mode 120000 crates/backtrace-sys/LICENSE-MIT delete mode 100644 crates/backtrace-sys/build.rs delete mode 100644 crates/backtrace-sys/src/android-api.c delete mode 100644 crates/backtrace-sys/src/lib.rs delete mode 160000 crates/backtrace-sys/src/libbacktrace delete mode 100644 src/symbolize/libbacktrace.rs diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 15d3c9484..d4cd25120 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -52,39 +52,20 @@ jobs: shell: bash if: matrix.rust == 'stable-i686-msvc' - # Force packed debuginfo on macOS because libbacktrace only works with - # packed debuginfo. We specifically test later that both packed and - # unpacked work. - - run: | - echo CARGO_PROFILE_DEV_SPLIT_DEBUGINFO=packed >> $GITHUB_ENV - echo CARGO_PROFILE_TEST_SPLIT_DEBUGINFO=packed >> $GITHUB_ENV - if: matrix.os == 'macos-latest' - - - run: cargo build --manifest-path crates/backtrace-sys/Cargo.toml - run: cargo build - run: cargo test - - run: cargo test --features "gimli-symbolize" - # run: cargo test --features "libbacktrace" - - run: cargo check --features "libbacktrace gimli-symbolize" - run: cargo test --features "serialize-rustc" - run: cargo test --features "serialize-serde" - run: cargo test --features "verify-winapi" - run: cargo test --features "cpp_demangle" - run: cargo test --no-default-features - - run: cargo test --no-default-features --features "libbacktrace" - - run: cargo test --no-default-features --features "gimli-symbolize" - - run: cargo test --no-default-features --features "gimli-symbolize libbacktrace" - - run: cargo test --no-default-features --features "libbacktrace std" - - run: cargo test --no-default-features --features "gimli-symbolize std" - run: cargo test --no-default-features --features "std" - run: cargo test --manifest-path crates/cpp_smoke_test/Cargo.toml - run: cargo test --manifest-path crates/macos_frames_test/Cargo.toml - - run: cargo test --features libbacktrace --manifest-path crates/without_debuginfo/Cargo.toml - run: cargo test --features gimli-symbolize --manifest-path crates/without_debuginfo/Cargo.toml - - run: cargo test --manifest-path crates/line-tables-only/Cargo.toml --features libbacktrace - run: cargo test --manifest-path crates/line-tables-only/Cargo.toml --features gimli-symbolize - # Test that if debuginfo is compressed gimli still works + # Test debuginfo compression still works - run: cargo test if: contains(matrix.os, 'ubuntu') env: diff --git a/Cargo.toml b/Cargo.toml index b137e64ad..f9a51df8a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,6 @@ exclude = ['crates/without_debuginfo', 'crates/macos_frames_test', 'crates/line- [dependencies] cfg-if = "1.0" rustc-demangle = "0.1.4" -backtrace-sys = { path = "crates/backtrace-sys", version = "0.1.35", optional = true, default_features = false } libc = { version = "0.2.94", default-features = false } # Optionally enable the ability to serialize a `Backtrace`, controlled through @@ -36,11 +35,10 @@ cpp_demangle = { default-features = false, version = "0.3.0", optional = true } # Optional dependencies enabled through the `gimli-symbolize` feature, do not # use these features directly. -addr2line = { version = "0.15.1", optional = true, default-features = false } -miniz_oxide = { version = "0.4.0", optional = true, default-features = false } +addr2line = { version = "0.15.1", default-features = false } +miniz_oxide = { version = "0.4.0", default-features = false } [dependencies.object] version = "0.24" -optional = true default-features = false features = ['read_core', 'elf', 'macho', 'pe', 'unaligned', 'archive'] @@ -58,26 +56,11 @@ libloading = "0.6" [features] # By default libstd support and gimli-symbolize is used to symbolize addresses. -default = ["std", "gimli-symbolize"] +default = ["std"] # Include std support. This enables types like `Backtrace`. std = [] -#======================================= -# Methods of resolving symbols -# -# - gimli-symbolize: use the `gimli-rs/addr2line` crate to symbolicate -# addresses into file, line, and name using DWARF debug information. -# - libbacktrace: this feature activates the `backtrace-sys` dependency, -# building the libbacktrace library found in gcc repos. -# -# Note that MSVC unconditionally uses the dbghelp library to symbolize and won't -# be affected by feature selection here. Also note that it's highly unlikely you -# want to configure this. If you're having trouble getting backtraces it's -# likely best to open an issue. -gimli-symbolize = ["addr2line", "miniz_oxide", "object"] -libbacktrace = ["backtrace-sys/backtrace-sys"] - #======================================= # Methods of serialization # @@ -91,11 +74,13 @@ serialize-serde = ["serde"] # Only here for backwards compatibility purposes or for internal testing # purposes. New code should use none of these features. coresymbolication = [] +dbghelp = [] dladdr = [] +gimli-symbolize = [] kernel32 = [] -unix-backtrace = [] +libbacktrace = [] libunwind = [] -dbghelp = [] +unix-backtrace = [] verify-winapi = [ 'winapi/dbghelp', 'winapi/handleapi', @@ -132,7 +117,7 @@ edition = '2018' [[test]] name = "accuracy" -required-features = ["std", "gimli-symbolize"] +required-features = ["std"] edition = '2018' [[test]] diff --git a/crates/backtrace-sys/Cargo.toml b/crates/backtrace-sys/Cargo.toml deleted file mode 100644 index f337ebd80..000000000 --- a/crates/backtrace-sys/Cargo.toml +++ /dev/null @@ -1,27 +0,0 @@ -[package] -name = "backtrace-sys" -version = "0.1.37" -authors = ["Alex Crichton "] -build = "build.rs" -license = "MIT/Apache-2.0" -repository = "https://github.com/rust-lang/backtrace-rs" -homepage = "https://github.com/rust-lang/backtrace-rs" -documentation = "https://docs.rs/backtrace-sys" -description = """ -Bindings to the libbacktrace gcc library -""" - -[dependencies] -libc = { version = "0.2", default-features = false } -core = { version = '1.0.0', optional = true, package = 'rustc-std-workspace-core' } -compiler_builtins = { version = '0.1.2', optional = true } - -[build-dependencies] -cc = "1.0.37" - -[features] -default = ["backtrace-sys"] - -# Without this feature, this crate does nothing. -backtrace-sys = [] -rustc-dep-of-std = ['core', 'compiler_builtins'] diff --git a/crates/backtrace-sys/LICENSE-APACHE b/crates/backtrace-sys/LICENSE-APACHE deleted file mode 120000 index 1cd601d0a..000000000 --- a/crates/backtrace-sys/LICENSE-APACHE +++ /dev/null @@ -1 +0,0 @@ -../../LICENSE-APACHE \ No newline at end of file diff --git a/crates/backtrace-sys/LICENSE-MIT b/crates/backtrace-sys/LICENSE-MIT deleted file mode 120000 index b2cfbdc7b..000000000 --- a/crates/backtrace-sys/LICENSE-MIT +++ /dev/null @@ -1 +0,0 @@ -../../LICENSE-MIT \ No newline at end of file diff --git a/crates/backtrace-sys/build.rs b/crates/backtrace-sys/build.rs deleted file mode 100644 index e861399d8..000000000 --- a/crates/backtrace-sys/build.rs +++ /dev/null @@ -1,168 +0,0 @@ -extern crate cc; - -use std::env; -use std::fs::File; -use std::path::PathBuf; - -fn main() { - let target = env::var("TARGET").unwrap(); - - if !cfg!(feature = "backtrace-sys") || // without this feature, this crate does nothing - target.contains("msvc") || // libbacktrace isn't used on MSVC windows - target.contains("emscripten") || // no way this will ever compile for emscripten - target.contains("cloudabi") || - target.contains("hermit") || - target.contains("wasm32") || - target.contains("fuchsia") || - target.contains("uclibc") || - target.contains("libnx") - { - println!("cargo:rustc-cfg=empty"); - return; - } - - let out_dir = PathBuf::from(env::var_os("OUT_DIR").unwrap()); - - let mut build = cc::Build::new(); - build - .include("src/libbacktrace") - .include(&out_dir) - .warnings(false) - .file("src/libbacktrace/alloc.c") - .file("src/libbacktrace/dwarf.c") - .file("src/libbacktrace/fileline.c") - .file("src/libbacktrace/posix.c") - .file("src/libbacktrace/sort.c") - .file("src/libbacktrace/state.c"); - - // `mmap` does not exist on Windows, so we use - // the less efficient `read`-based code. - // Using `mmap` on macOS causes weird isseus - see - // https://github.com/rust-lang/rust/pull/45866 - if target.contains("windows") || target.contains("darwin") { - build.file("src/libbacktrace/read.c"); - } else { - build.file("src/libbacktrace/mmapio.c"); - } - - // No need to have any symbols reexported form shared objects - build.flag("-fvisibility=hidden"); - - if target.contains("darwin") { - build.file("src/libbacktrace/macho.c"); - build.define("HAVE_MACH_O_DYLD_H", "1"); - } else if target.contains("windows") { - build.file("src/libbacktrace/pecoff.c"); - } else { - build.file("src/libbacktrace/elf.c"); - - let pointer_width = env::var("CARGO_CFG_TARGET_POINTER_WIDTH").unwrap(); - if pointer_width == "64" { - build.define("BACKTRACE_ELF_SIZE", "64"); - } else { - build.define("BACKTRACE_ELF_SIZE", "32"); - } - } - - File::create(out_dir.join("backtrace-supported.h")).unwrap(); - build.define("BACKTRACE_SUPPORTED", "1"); - build.define("BACKTRACE_USES_MALLOC", "1"); - build.define("BACKTRACE_SUPPORTS_THREADS", "0"); - build.define("BACKTRACE_SUPPORTS_DATA", "0"); - - File::create(out_dir.join("config.h")).unwrap(); - if target.contains("android") { - maybe_enable_dl_iterate_phdr_android(&mut build); - } else if target.contains("dragonfly") || target.contains("freebsd") { - build.define("HAVE_DL_ITERATE_PHDR", "1"); - build.define("HAVE_KERN_PROC", "1"); - } else if target.contains("netbsd") { - build.define("HAVE_DL_ITERATE_PHDR", "1"); - build.define("HAVE_KERN_PROC_ARGS", "1"); - } else if !target.contains("apple-ios") - && !target.contains("solaris") - && !target.contains("redox") - && !target.contains("haiku") - && !target.contains("vxworks") - { - build.define("HAVE_DL_ITERATE_PHDR", "1"); - } - build.define("_GNU_SOURCE", "1"); - build.define("_LARGE_FILES", "1"); - - let syms = [ - "backtrace_full", - "backtrace_dwarf_add", - "backtrace_initialize", - "backtrace_pcinfo", - "backtrace_syminfo", - "backtrace_get_view", - "backtrace_release_view", - "backtrace_alloc", - "backtrace_free", - "backtrace_vector_finish", - "backtrace_vector_grow", - "backtrace_vector_release", - "backtrace_close", - "backtrace_open", - "backtrace_print", - "backtrace_simple", - "backtrace_qsort", - "backtrace_create_state", - "backtrace_uncompress_zdebug", - // These should be `static` in C, but they aren't... - "macho_get_view", - "macho_symbol_type_relevant", - "macho_get_commands", - "macho_try_dsym", - "macho_try_dwarf", - "macho_get_addr_range", - "macho_get_uuid", - "macho_add", - "macho_add_symtab", - "macho_file_to_host_u64", - "macho_file_to_host_u32", - "macho_file_to_host_u16", - ]; - let prefix = if cfg!(feature = "rustc-dep-of-std") { - println!("cargo:rustc-cfg=rdos"); - "__rdos_" - } else { - println!("cargo:rustc-cfg=rbt"); - "__rbt_" - }; - for sym in syms.iter() { - build.define(sym, &format!("{}{}", prefix, sym)[..]); - } - - build.compile("backtrace"); -} - -// The `dl_iterate_phdr` API was added in Android API 21+ (according to #227), -// so if we can dynamically detect an appropriately configured C compiler for -// that then let's enable the `dl_iterate_phdr` API, otherwise Android just -// won't have any information. -fn maybe_enable_dl_iterate_phdr_android(build: &mut cc::Build) { - let expansion = cc::Build::new().file("src/android-api.c").expand(); - let expansion = match std::str::from_utf8(&expansion) { - Ok(s) => s, - Err(_) => return, - }; - println!("expanded android version detection:\n{}", expansion); - let marker = "APIVERSION"; - let i = match expansion.find(marker) { - Some(i) => i, - None => return, - }; - let version = match expansion[i + marker.len() + 1..].split_whitespace().next() { - Some(s) => s, - None => return, - }; - let version = match version.parse::() { - Ok(n) => n, - Err(_) => return, - }; - if version >= 21 { - build.define("HAVE_DL_ITERATE_PHDR", "1"); - } -} diff --git a/crates/backtrace-sys/src/android-api.c b/crates/backtrace-sys/src/android-api.c deleted file mode 100644 index 1bfeadf5b..000000000 --- a/crates/backtrace-sys/src/android-api.c +++ /dev/null @@ -1,4 +0,0 @@ -// Used from the build script to detect the value of the `__ANDROID_API__` -// builtin #define - -APIVERSION __ANDROID_API__ diff --git a/crates/backtrace-sys/src/lib.rs b/crates/backtrace-sys/src/lib.rs deleted file mode 100644 index 98a54c30b..000000000 --- a/crates/backtrace-sys/src/lib.rs +++ /dev/null @@ -1,58 +0,0 @@ -#![allow(bad_style)] -#![no_std] - -extern crate libc; - -#[cfg(not(empty))] -pub use self::bindings::*; -#[cfg(not(empty))] -mod bindings { - use libc::{c_char, c_int, c_void, uintptr_t}; - - pub type backtrace_syminfo_callback = extern "C" fn( - data: *mut c_void, - pc: uintptr_t, - symname: *const c_char, - symval: uintptr_t, - symsize: uintptr_t, - ); - pub type backtrace_full_callback = extern "C" fn( - data: *mut c_void, - pc: uintptr_t, - filename: *const c_char, - lineno: c_int, - function: *const c_char, - ) -> c_int; - pub type backtrace_error_callback = - extern "C" fn(data: *mut c_void, msg: *const c_char, errnum: c_int); - pub enum backtrace_state {} - - extern "C" { - #[cfg_attr(rdos, link_name = "__rdos_backtrace_create_state")] - #[cfg_attr(rbt, link_name = "__rbt_backtrace_create_state")] - pub fn backtrace_create_state( - filename: *const c_char, - threaded: c_int, - error: backtrace_error_callback, - data: *mut c_void, - ) -> *mut backtrace_state; - #[cfg_attr(rdos, link_name = "__rdos_backtrace_syminfo")] - #[cfg_attr(rbt, link_name = "__rbt_backtrace_syminfo")] - pub fn backtrace_syminfo( - state: *mut backtrace_state, - addr: uintptr_t, - cb: backtrace_syminfo_callback, - error: backtrace_error_callback, - data: *mut c_void, - ) -> c_int; - #[cfg_attr(rdos, link_name = "__rdos_backtrace_pcinfo")] - #[cfg_attr(rbt, link_name = "__rbt_backtrace_pcinfo")] - pub fn backtrace_pcinfo( - state: *mut backtrace_state, - addr: uintptr_t, - cb: backtrace_full_callback, - error: backtrace_error_callback, - data: *mut c_void, - ) -> c_int; - } -} diff --git a/crates/backtrace-sys/src/libbacktrace b/crates/backtrace-sys/src/libbacktrace deleted file mode 160000 index 5c88e094a..000000000 --- a/crates/backtrace-sys/src/libbacktrace +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 5c88e094a691bb803d4bba342403a10459abad9e diff --git a/src/symbolize/libbacktrace.rs b/src/symbolize/libbacktrace.rs deleted file mode 100644 index 665733605..000000000 --- a/src/symbolize/libbacktrace.rs +++ /dev/null @@ -1,460 +0,0 @@ -//! Symbolication strategy using the DWARF-parsing code in libbacktrace. -//! -//! The libbacktrace C library, typically distributed with gcc, supports not -//! only generating a backtrace (which we don't actually use) but also -//! symbolicating the backtrace and handling dwarf debug information about -//! things like inlined frames and whatnot. -//! -//! This is relatively complicated due to lots of various concerns here, but the -//! basic idea is: -//! -//! * First we call `backtrace_syminfo`. This gets symbol information from the -//! dynamic symbol table if we can. -//! * Next we call `backtrace_pcinfo`. This will parse debuginfo tables if -//! they're available and allow us to recover information about inline frames, -//! filenames, line numbers, etc. -//! -//! There's lots of trickery about getting the dwarf tables into libbacktrace, -//! but hopefully it's not the end of the world and is clear enough when reading -//! below. -//! -//! This is the default symbolication strategy for non-MSVC and non-OSX -//! platforms. In libstd though this is the default strategy for OSX. - -#![allow(bad_style)] - -extern crate backtrace_sys as bt; - -use core::{marker, ptr, slice}; -use libc::{self, c_char, c_int, c_void, uintptr_t}; - -use crate::symbolize::{ResolveWhat, SymbolName}; -use crate::types::BytesOrWideString; - -pub enum Symbol<'a> { - Syminfo { - pc: uintptr_t, - symname: *const c_char, - _marker: marker::PhantomData<&'a ()>, - }, - Pcinfo { - pc: uintptr_t, - filename: *const c_char, - lineno: c_int, - function: *const c_char, - symname: *const c_char, - }, -} - -impl Symbol<'_> { - pub fn name(&self) -> Option> { - let symbol = |ptr: *const c_char| unsafe { - if ptr.is_null() { - None - } else { - let len = libc::strlen(ptr); - Some(SymbolName::new(slice::from_raw_parts( - ptr as *const u8, - len, - ))) - } - }; - match *self { - Symbol::Syminfo { symname, .. } => symbol(symname), - Symbol::Pcinfo { - function, symname, .. - } => { - // If possible prefer the `function` name which comes from - // debuginfo and can typically be more accurate for inline - // frames for example. If that's not present though fall back to - // the symbol table name specified in `symname`. - // - // Note that sometimes `function` can feel somewhat less - // accurate, for example being listed as `try` - // isntead of `std::panicking::try::do_call`. It's not really - // clear why, but overall the `function` name seems more accurate. - if let Some(sym) = symbol(function) { - return Some(sym); - } - symbol(symname) - } - } - } - - pub fn addr(&self) -> Option<*mut c_void> { - let pc = match *self { - Symbol::Syminfo { pc, .. } => pc, - Symbol::Pcinfo { pc, .. } => pc, - }; - if pc == 0 { - None - } else { - Some(pc as *mut _) - } - } - - fn filename_bytes(&self) -> Option<&[u8]> { - match *self { - Symbol::Syminfo { .. } => None, - Symbol::Pcinfo { filename, .. } => { - let ptr = filename as *const u8; - if ptr.is_null() { - return None; - } - unsafe { - let len = libc::strlen(filename); - Some(slice::from_raw_parts(ptr, len)) - } - } - } - } - - pub fn filename_raw(&self) -> Option> { - self.filename_bytes().map(BytesOrWideString::Bytes) - } - - #[cfg(feature = "std")] - pub fn filename(&self) -> Option<&::std::path::Path> { - use std::path::Path; - - #[cfg(unix)] - fn bytes2path(bytes: &[u8]) -> Option<&Path> { - use std::ffi::OsStr; - use std::os::unix::prelude::*; - Some(Path::new(OsStr::from_bytes(bytes))) - } - - #[cfg(windows)] - fn bytes2path(bytes: &[u8]) -> Option<&Path> { - use std::str; - str::from_utf8(bytes).ok().map(Path::new) - } - - self.filename_bytes().and_then(bytes2path) - } - - pub fn lineno(&self) -> Option { - match *self { - Symbol::Syminfo { .. } => None, - Symbol::Pcinfo { lineno, .. } => Some(lineno as u32), - } - } - - pub fn colno(&self) -> Option { - None - } -} - -extern "C" fn error_cb(_data: *mut c_void, _msg: *const c_char, _errnum: c_int) { - // do nothing for now -} - -/// Type of the `data` pointer passed into `syminfo_cb` -struct SyminfoState<'a> { - cb: &'a mut (dyn FnMut(&super::Symbol) + 'a), - pc: usize, -} - -extern "C" fn syminfo_cb( - data: *mut c_void, - pc: uintptr_t, - symname: *const c_char, - _symval: uintptr_t, - _symsize: uintptr_t, -) { - let mut bomb = crate::Bomb { enabled: true }; - - // Once this callback is invoked from `backtrace_syminfo` when we start - // resolving we go further to call `backtrace_pcinfo`. The - // `backtrace_pcinfo` function will consult debug information and attemp tto - // do things like recover file/line information as well as inlined frames. - // Note though that `backtrace_pcinfo` can fail or not do much if there's - // not debug info, so if that happens we're sure to call the callback with - // at least one symbol from the `syminfo_cb`. - unsafe { - let syminfo_state = &mut *(data as *mut SyminfoState<'_>); - let mut pcinfo_state = PcinfoState { - symname, - called: false, - cb: syminfo_state.cb, - }; - bt::backtrace_pcinfo( - init_state(), - syminfo_state.pc as uintptr_t, - pcinfo_cb, - error_cb, - &mut pcinfo_state as *mut _ as *mut _, - ); - if !pcinfo_state.called { - (pcinfo_state.cb)(&super::Symbol { - inner: Symbol::Syminfo { - pc: pc, - symname: symname, - _marker: marker::PhantomData, - }, - }); - } - } - - bomb.enabled = false; -} - -/// Type of the `data` pointer passed into `pcinfo_cb` -struct PcinfoState<'a> { - cb: &'a mut (dyn FnMut(&super::Symbol) + 'a), - symname: *const c_char, - called: bool, -} - -extern "C" fn pcinfo_cb( - data: *mut c_void, - pc: uintptr_t, - filename: *const c_char, - lineno: c_int, - function: *const c_char, -) -> c_int { - let mut bomb = crate::Bomb { enabled: true }; - - unsafe { - let state = &mut *(data as *mut PcinfoState<'_>); - state.called = true; - (state.cb)(&super::Symbol { - inner: Symbol::Pcinfo { - pc: pc, - filename: filename, - lineno: lineno, - symname: state.symname, - function, - }, - }); - } - - bomb.enabled = false; - return 0; -} - -// The libbacktrace API supports creating a state, but it does not -// support destroying a state. I personally take this to mean that a -// state is meant to be created and then live forever. -// -// I would love to register an at_exit() handler which cleans up this -// state, but libbacktrace provides no way to do so. -// -// With these constraints, this function has a statically cached state -// that is calculated the first time this is requested. Remember that -// backtracing all happens serially (one global lock). -// -// Note the lack of synchronization here is due to the requirement that -// `resolve` is externally synchronized. -unsafe fn init_state() -> *mut bt::backtrace_state { - static mut STATE: *mut bt::backtrace_state = 0 as *mut _; - - if !STATE.is_null() { - return STATE; - } - - STATE = bt::backtrace_create_state( - load_filename(), - // Don't exercise threadsafe capabilities of libbacktrace since - // we're always calling it in a synchronized fashion. - 0, - error_cb, - ptr::null_mut(), // no extra data - ); - - return STATE; - - // Note that for libbacktrace to operate at all it needs to find the DWARF - // debug info for the current executable. It typically does that via a - // number of mechanisms including, but not limited to: - // - // * /proc/self/exe on supported platforms - // * The filename passed in explicitly when creating state - // - // The libbacktrace library is a big wad of C code. This naturally means - // it's got memory safety vulnerabilities, especially when handling - // malformed debuginfo. Libstd has run into plenty of these historically. - // - // If /proc/self/exe is used then we can typically ignore these as we - // assume that libbacktrace is "mostly correct" and otherwise doesn't do - // weird things with "attempted to be correct" dwarf debug info. - // - // If we pass in a filename, however, then it's possible on some platforms - // (like BSDs) where a malicious actor can cause an arbitrary file to be - // placed at that location. This means that if we tell libbacktrace about a - // filename it may be using an arbitrary file, possibly causing segfaults. - // If we don't tell libbacktrace anything though then it won't do anything - // on platforms that don't support paths like /proc/self/exe! - // - // Given all that we try as hard as possible to *not* pass in a filename, - // but we must on platforms that don't support /proc/self/exe at all. - cfg_if::cfg_if! { - if #[cfg(any(target_os = "macos", target_os = "ios"))] { - // Note that ideally we'd use `std::env::current_exe`, but we can't - // require `std` here. - // - // Use `_NSGetExecutablePath` to load the current executable path - // into a static area (which if it's too small just give up). - // - // Note that we're seriously trusting libbacktrace here to not die - // on corrupt executables, but it surely does... - unsafe fn load_filename() -> *const libc::c_char { - const N: usize = 256; - static mut BUF: [u8; N] = [0; N]; - extern { - fn _NSGetExecutablePath( - buf: *mut libc::c_char, - bufsize: *mut u32, - ) -> libc::c_int; - } - let mut sz: u32 = BUF.len() as u32; - let ptr = BUF.as_mut_ptr() as *mut libc::c_char; - if _NSGetExecutablePath(ptr, &mut sz) == 0 { - ptr - } else { - ptr::null() - } - } - } else if #[cfg(windows)] { - use crate::windows::*; - - // Windows has a mode of opening files where after it's opened it - // can't be deleted. That's in general what we want here because we - // want to ensure that our executable isn't changing out from under - // us after we hand it off to libbacktrace, hopefully mitigating the - // ability to pass in arbitrary data into libbacktrace (which may be - // mishandled). - // - // Given that we do a bit of a dance here to attempt to get a sort - // of lock on our own image: - // - // * Get a handle to the current process, load its filename. - // * Open a file to that filename with the right flags. - // * Reload the current process's filename, making sure it's the same - // - // If that all passes we in theory have indeed opened our process's - // file and we're guaranteed it won't change. FWIW a bunch of this - // is copied from libstd historically, so this is my best - // interpretation of what was happening. - unsafe fn load_filename() -> *const libc::c_char { - load_filename_opt().unwrap_or(ptr::null()) - } - - unsafe fn load_filename_opt() -> Result<*const libc::c_char, ()> { - const N: usize = 256; - // This lives in static memory so we can return it.. - static mut BUF: [i8; N] = [0; N]; - // ... and this lives on the stack since it's temporary - let mut stack_buf = [0; N]; - let name1 = query_full_name(&mut BUF)?; - - let handle = CreateFileA( - name1.as_ptr(), - GENERIC_READ, - FILE_SHARE_READ | FILE_SHARE_WRITE, - ptr::null_mut(), - OPEN_EXISTING, - 0, - ptr::null_mut(), - ); - if handle.is_null() { - return Err(()); - } - - let name2 = query_full_name(&mut stack_buf)?; - if name1 != name2 { - CloseHandle(handle); - return Err(()) - } - // intentionally leak `handle` here because having that open - // should preserve our lock on this file name. - Ok(name1.as_ptr()) - } - - unsafe fn query_full_name(buf: &mut [i8]) -> Result<&[i8], ()> { - let dll = GetModuleHandleA(b"kernel32.dll\0".as_ptr() as *const i8); - if dll.is_null() { - return Err(()) - } - let ptrQueryFullProcessImageNameA = - GetProcAddress(dll, b"QueryFullProcessImageNameA\0".as_ptr() as *const _) as usize; - if ptrQueryFullProcessImageNameA == 0 - { - return Err(()); - } - use core::mem; - let p1 = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, GetCurrentProcessId()); - let mut len = buf.len() as u32; - let pfnQueryFullProcessImageNameA : extern "system" fn( - hProcess: HANDLE, - dwFlags: DWORD, - lpExeName: LPSTR, - lpdwSize: PDWORD, - ) -> BOOL = mem::transmute(ptrQueryFullProcessImageNameA); - - let rc = pfnQueryFullProcessImageNameA(p1, 0, buf.as_mut_ptr(), &mut len); - CloseHandle(p1); - - // We want to return a slice that is nul-terminated, so if - // everything was filled in and it equals the total length - // then equate that to failure. - // - // Otherwise when returning success make sure the nul byte is - // included in the slice. - if rc == 0 || len == buf.len() as u32 { - Err(()) - } else { - assert_eq!(buf[len as usize], 0); - Ok(&buf[..(len + 1) as usize]) - } - } - } else if #[cfg(target_os = "vxworks")] { - unsafe fn load_filename() -> *const libc::c_char { - use libc; - use core::mem; - - const N: usize = libc::VX_RTP_NAME_LENGTH as usize + 1; - static mut BUF: [libc::c_char; N] = [0; N]; - - let mut rtp_desc : libc::RTP_DESC = mem::zeroed(); - if (libc::rtpInfoGet(0, &mut rtp_desc as *mut libc::RTP_DESC) == 0) { - BUF.copy_from_slice(&rtp_desc.pathName); - BUF.as_ptr() - } else { - ptr::null() - } - } - } else { - unsafe fn load_filename() -> *const libc::c_char { - ptr::null() - } - } - } -} - -pub unsafe fn resolve(what: ResolveWhat<'_>, cb: &mut dyn FnMut(&super::Symbol)) { - let symaddr = what.address_or_ip() as usize; - - // backtrace errors are currently swept under the rug - let state = init_state(); - if state.is_null() { - return; - } - - // Call the `backtrace_syminfo` API which (from reading the code) - // should call `syminfo_cb` exactly once (or fail with an error - // presumably). We then handle more within the `syminfo_cb`. - // - // Note that we do this since `syminfo` will consult the symbol table, - // finding symbol names even if there's no debug information in the binary. - let mut syminfo_state = SyminfoState { pc: symaddr, cb }; - bt::backtrace_syminfo( - state, - symaddr as uintptr_t, - syminfo_cb, - error_cb, - &mut syminfo_state as *mut _ as *mut _, - ); -} - -pub unsafe fn clear_symbol_cache() {} diff --git a/src/symbolize/mod.rs b/src/symbolize/mod.rs index 961c63d99..699d54e51 100644 --- a/src/symbolize/mod.rs +++ b/src/symbolize/mod.rs @@ -471,17 +471,6 @@ cfg_if::cfg_if! { mod dbghelp; use dbghelp as imp; } else if #[cfg(all( - feature = "libbacktrace", - any(unix, all(windows, not(target_vendor = "uwp"), target_env = "gnu")), - not(target_os = "fuchsia"), - not(target_os = "emscripten"), - not(target_env = "uclibc"), - not(target_env = "libnx"), - ))] { - mod libbacktrace; - use libbacktrace as imp; - } else if #[cfg(all( - feature = "gimli-symbolize", any(unix, windows), not(target_vendor = "uwp"), not(target_os = "emscripten"), diff --git a/tests/accuracy/main.rs b/tests/accuracy/main.rs index a2aba071b..54409c179 100644 --- a/tests/accuracy/main.rs +++ b/tests/accuracy/main.rs @@ -20,8 +20,6 @@ fn doit() { // Skip musl which is by default statically linked and doesn't support // dynamic libraries. !cfg!(target_env = "musl") - // Skip MinGW on libbacktrace which doesn't have support for DLLs. - && !(cfg!(windows) && cfg!(target_env = "gnu") && cfg!(feature = "libbacktrace")) // Skip Miri, since it doesn't support dynamic libraries. && !cfg!(miri) { diff --git a/tests/smoke.rs b/tests/smoke.rs index 0c990e066..12777b59a 100644 --- a/tests/smoke.rs +++ b/tests/smoke.rs @@ -1,38 +1,6 @@ use backtrace::Frame; use std::thread; -// Reflects the conditional compilation logic at end of src/symbolize/mod.rs -static NOOP: bool = false; -static DBGHELP: bool = !NOOP - && cfg!(all( - windows, - target_env = "msvc", - not(target_vendor = "uwp") - )); -static LIBBACKTRACE: bool = !NOOP - && !DBGHELP - && cfg!(all( - feature = "libbacktrace", - any( - unix, - all(windows, not(target_vendor = "uwp"), target_env = "gnu") - ), - not(target_os = "fuchsia"), - not(target_os = "emscripten"), - not(target_env = "uclibc"), - not(target_env = "libnx"), - )); -static GIMLI_SYMBOLIZE: bool = !NOOP - && !DBGHELP - && !LIBBACKTRACE - && cfg!(all( - feature = "gimli-symbolize", - any(unix, windows), - not(target_vendor = "uwp"), - not(target_os = "emscripten"), - )); -static MIRI_SYMBOLIZE: bool = cfg!(miri); - #[test] // FIXME: shouldn't ignore this test on i686-msvc, unsure why it's failing #[cfg_attr(all(target_arch = "x86", target_env = "msvc"), ignore)] @@ -159,8 +127,6 @@ fn smoke_test_frames() { } let mut resolved = 0; - let can_resolve = LIBBACKTRACE || GIMLI_SYMBOLIZE || MIRI_SYMBOLIZE; - let can_resolve_cols = GIMLI_SYMBOLIZE || MIRI_SYMBOLIZE; let mut name = None; let mut addr = None; @@ -175,31 +141,22 @@ fn smoke_test_frames() { line = sym.lineno(); file = sym.filename().map(|v| v.to_path_buf()); }); + assert!(resolved > 0); - // dbghelp doesn't always resolve symbols right now - match resolved { - 0 => return assert!(!can_resolve), - _ => {} - } - - if can_resolve { - let name = name.expect("didn't find a name"); + let name = name.expect("didn't find a name"); - // in release mode names get weird as functions can get merged - // together with `mergefunc`, so only assert this in debug mode - if cfg!(debug_assertions) { - assert!( - name.contains(expected_name), - "didn't find `{}` in `{}`", - expected_name, - name - ); - } + // in release mode names get weird as functions can get merged + // together with `mergefunc`, so only assert this in debug mode + if cfg!(debug_assertions) { + assert!( + name.contains(expected_name), + "didn't find `{}` in `{}`", + expected_name, + name + ); } - if can_resolve { - addr.expect("didn't find a symbol"); - } + addr.expect("didn't find a symbol"); if cfg!(debug_assertions) { let line = line.expect("didn't find a line number"); @@ -221,17 +178,15 @@ fn smoke_test_frames() { expected_line ); } - if can_resolve_cols { - let col = col.expect("didn't find a column number"); - if expected_col != 0 { - assert!( - col == expected_col, - "bad column number on frame for `{}`: {} != {}", - expected_name, - col, - expected_col - ); - } + let col = col.expect("didn't find a column number"); + if expected_col != 0 { + assert!( + col == expected_col, + "bad column number on frame for `{}`: {} != {}", + expected_name, + col, + expected_col + ); } } } @@ -321,9 +276,8 @@ fn sp_smoke_test() { let mut is_recursive_stack_references = false; backtrace::resolve(frame.ip(), |sym| { - is_recursive_stack_references |= (LIBBACKTRACE || GIMLI_SYMBOLIZE) - && sym - .name() + is_recursive_stack_references |= + sym.name() .and_then(|name| name.as_str()) .map_or(false, |name| { eprintln!("name = {}", name); From c5e9522842bcb279008e2d3bf04448ea0cb95991 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 5 May 2021 07:08:56 -0700 Subject: [PATCH 2/3] Fix MSVC test --- tests/smoke.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/tests/smoke.rs b/tests/smoke.rs index 12777b59a..683a6f0db 100644 --- a/tests/smoke.rs +++ b/tests/smoke.rs @@ -178,15 +178,19 @@ fn smoke_test_frames() { expected_line ); } - let col = col.expect("didn't find a column number"); - if expected_col != 0 { - assert!( - col == expected_col, - "bad column number on frame for `{}`: {} != {}", - expected_name, - col, - expected_col - ); + + // dbghelp on MSVC doesn't support column numbers + if !cfg!(target_env = "msvc") { + let col = col.expect("didn't find a column number"); + if expected_col != 0 { + assert!( + col == expected_col, + "bad column number on frame for `{}`: {} != {}", + expected_name, + col, + expected_col + ); + } } } } From 02f08355437cc2f28ea23a30bcb13b1f6052b3ef Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 5 May 2021 07:21:12 -0700 Subject: [PATCH 3/3] Fix a macOS test --- .github/workflows/main.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index d4cd25120..f9ab18dc2 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -61,7 +61,11 @@ jobs: - run: cargo test --no-default-features - run: cargo test --no-default-features --features "std" - run: cargo test --manifest-path crates/cpp_smoke_test/Cargo.toml + # This test is specifically about packed debuginfo with `*.dSYM` files - run: cargo test --manifest-path crates/macos_frames_test/Cargo.toml + env: + CARGO_PROFILE_DEV_SPLIT_DEBUGINFO: packed + CARGO_PROFILE_TEST_SPLIT_DEBUGINFO: packed - run: cargo test --features gimli-symbolize --manifest-path crates/without_debuginfo/Cargo.toml - run: cargo test --manifest-path crates/line-tables-only/Cargo.toml --features gimli-symbolize