From 9393e52d4d2705698a6dfdd2834d41154ee23b64 Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Sun, 8 May 2016 12:07:50 -0700 Subject: [PATCH] Don't use env::current_exe with libbacktrace If the path we give to libbacktrace doesn't actually correspond to the current process, libbacktrace will segfault *at best*. cc #21889 --- src/libstd/sys/common/gnu/libbacktrace.rs | 49 ++++++----------------- src/test/run-pass/backtrace-debuginfo.rs | 14 ++++--- src/test/run-pass/backtrace.rs | 2 +- 3 files changed, 22 insertions(+), 43 deletions(-) diff --git a/src/libstd/sys/common/gnu/libbacktrace.rs b/src/libstd/sys/common/gnu/libbacktrace.rs index db719ccce61e8..b5802afc10943 100644 --- a/src/libstd/sys/common/gnu/libbacktrace.rs +++ b/src/libstd/sys/common/gnu/libbacktrace.rs @@ -15,7 +15,6 @@ use sys_common::backtrace::{output, output_fileline}; pub fn print(w: &mut Write, idx: isize, addr: *mut libc::c_void, symaddr: *mut libc::c_void) -> io::Result<()> { - use env; use ffi::CStr; use ptr; @@ -110,46 +109,22 @@ pub fn print(w: &mut Write, idx: isize, addr: *mut libc::c_void, // that is calculated the first time this is requested. Remember that // backtracing all happens serially (one global lock). // - // An additionally oddity in this function is that we initialize the - // filename via self_exe_name() to pass to libbacktrace. It turns out - // that on Linux libbacktrace seamlessly gets the filename of the - // current executable, but this fails on freebsd. by always providing - // it, we make sure that libbacktrace never has a reason to not look up - // the symbols. The libbacktrace API also states that the filename must - // be in "permanent memory", so we copy it to a static and then use the - // static as the pointer. + // Things don't work so well on not-Linux since libbacktrace can't track + // down that executable this is. We at one point used env::current_exe but + // it turns out that there are some serious security issues with that + // approach. // - // FIXME: We also call self_exe_name() on DragonFly BSD. I haven't - // tested if this is required or not. + // Specifically, on certain platforms like BSDs, a malicious actor can cause + // an arbitrary file to be placed at the path returned by current_exe. + // libbacktrace does not behave defensively in the presence of ill-formed + // DWARF information, and has been demonstrated to segfault in at least one + // case. There is no evidence at the moment to suggest that a more carefully + // constructed file can't cause arbitrary code execution. As a result of all + // of this, we don't hint libbacktrace with the path to the current process. unsafe fn init_state() -> *mut backtrace_state { static mut STATE: *mut backtrace_state = ptr::null_mut(); - static mut LAST_FILENAME: [libc::c_char; 256] = [0; 256]; if !STATE.is_null() { return STATE } - let selfname = if cfg!(target_os = "freebsd") || - cfg!(target_os = "dragonfly") || - cfg!(target_os = "bitrig") || - cfg!(target_os = "openbsd") || - cfg!(target_os = "windows") { - env::current_exe().ok() - } else { - None - }; - let filename = match selfname.as_ref().and_then(|s| s.to_str()) { - Some(path) => { - let bytes = path.as_bytes(); - if bytes.len() < LAST_FILENAME.len() { - let i = bytes.iter(); - for (slot, val) in LAST_FILENAME.iter_mut().zip(i) { - *slot = *val as libc::c_char; - } - LAST_FILENAME.as_ptr() - } else { - ptr::null() - } - } - None => ptr::null(), - }; - STATE = backtrace_create_state(filename, 0, error_cb, + STATE = backtrace_create_state(ptr::null(), 0, error_cb, ptr::null_mut()); STATE } diff --git a/src/test/run-pass/backtrace-debuginfo.rs b/src/test/run-pass/backtrace-debuginfo.rs index 8b2b26948824f..f42a6ab162b70 100644 --- a/src/test/run-pass/backtrace-debuginfo.rs +++ b/src/test/run-pass/backtrace-debuginfo.rs @@ -32,11 +32,15 @@ macro_rules! dump_and_die { ($($pos:expr),*) => ({ // FIXME(#18285): we cannot include the current position because // the macro span takes over the last frame's file/line. - if cfg!(target_os = "macos") || - cfg!(target_os = "ios") || - cfg!(target_os = "android") || - cfg!(all(target_os = "linux", target_arch = "arm")) || - cfg!(all(windows, target_env = "gnu")) { + if cfg!(any(target_os = "macos", + target_os = "ios", + target_os = "android", + all(target_os = "linux", target_arch = "arm"), + target_os = "windows", + target_os = "freebsd", + target_os = "dragonfly", + target_os = "bitrig", + target_os = "openbsd")) { // skip these platforms as this support isn't implemented yet. } else { dump_filelines(&[$($pos),*]); diff --git a/src/test/run-pass/backtrace.rs b/src/test/run-pass/backtrace.rs index 5b364358a59dd..ad38dc8f45252 100644 --- a/src/test/run-pass/backtrace.rs +++ b/src/test/run-pass/backtrace.rs @@ -115,7 +115,7 @@ fn runtest(me: &str) { } fn main() { - if cfg!(windows) && cfg!(target_arch = "x86") && cfg!(target_env = "gnu") { + if cfg!(windows) && cfg!(target_env = "gnu") { return }