From 766fcb0b01986396b2b87ba194dcd4392b57b613 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Thu, 20 Aug 2020 14:34:40 -0700 Subject: [PATCH 1/4] Refactor dynamic library error checking on *nix The old code was checking `dlerror` more often than necessary, since the return value of `dlopen` indicates whether an error occurred. --- src/librustc_metadata/dynamic_lib.rs | 94 ++++++++++++++++++---------- src/librustc_metadata/lib.rs | 1 + 2 files changed, 61 insertions(+), 34 deletions(-) diff --git a/src/librustc_metadata/dynamic_lib.rs b/src/librustc_metadata/dynamic_lib.rs index ce19240a009d0..6867097d37294 100644 --- a/src/librustc_metadata/dynamic_lib.rs +++ b/src/librustc_metadata/dynamic_lib.rs @@ -51,51 +51,77 @@ mod tests; #[cfg(unix)] mod dl { - use std::ffi::{CStr, CString, OsStr}; + use std::ffi::{CString, OsStr}; use std::os::unix::prelude::*; - use std::ptr; - use std::str; - pub(super) fn open(filename: &OsStr) -> Result<*mut u8, String> { - check_for_errors_in(|| unsafe { - let s = CString::new(filename.as_bytes()).unwrap(); - libc::dlopen(s.as_ptr(), libc::RTLD_LAZY) as *mut u8 - }) - } + // `dlerror` is process global, so we can only allow a single thread at a + // time to call `dlsym` and `dlopen` if we want to check the error message. + mod error { + use std::ffi::CStr; + use std::lazy::SyncLazy; + use std::sync::{Mutex, MutexGuard}; - fn check_for_errors_in(f: F) -> Result - where - F: FnOnce() -> T, - { - use std::sync::{Mutex, Once}; - static INIT: Once = Once::new(); - static mut LOCK: *mut Mutex<()> = ptr::null_mut(); - unsafe { - INIT.call_once(|| { - LOCK = Box::into_raw(Box::new(Mutex::new(()))); - }); - // dlerror isn't thread safe, so we need to lock around this entire - // sequence - let _guard = (*LOCK).lock(); - let _old_error = libc::dlerror(); - - let result = f(); - - let last_error = libc::dlerror() as *const _; - if ptr::null() == last_error { - Ok(result) - } else { - let s = CStr::from_ptr(last_error).to_bytes(); - Err(str::from_utf8(s).unwrap().to_owned()) + pub fn lock() -> MutexGuard<'static, Guard> { + static LOCK: SyncLazy> = SyncLazy::new(|| Mutex::new(Guard { _priv: () })); + LOCK.lock().unwrap() + } + + pub struct Guard { + _priv: (), + } + + impl Guard { + pub fn get(&mut self) -> Result<(), String> { + let msg = unsafe { libc::dlerror() }; + if msg.is_null() { + Ok(()) + } else { + let msg = unsafe { CStr::from_ptr(msg as *const _) }; + Err(msg.to_string_lossy().into_owned()) + } } + + pub fn clear(&mut self) { + let _ = unsafe { libc::dlerror() }; + } + } + } + + pub(super) fn open(filename: &OsStr) -> Result<*mut u8, String> { + let s = CString::new(filename.as_bytes()).unwrap(); + + let mut dlerror = error::lock(); + let ret = unsafe { libc::dlopen(s.as_ptr(), libc::RTLD_LAZY) } as *mut u8; + + if !ret.is_null() { + return Ok(ret); } + + // A NULL return from `dlopen` indicates that an error has + // definitely occurred, so if nothing is in `dlerror`, we are + // racing with another thread that has stolen our error message. + dlerror.get().and_then(|()| Err("Unknown error".to_string())) } pub(super) unsafe fn symbol( handle: *mut u8, symbol: *const libc::c_char, ) -> Result<*mut u8, String> { - check_for_errors_in(|| libc::dlsym(handle as *mut libc::c_void, symbol) as *mut u8) + let mut dlerror = error::lock(); + + // Flush `dlerror` since we need to use it to determine whether the subsequent call to + // `dlsym` is successful. + dlerror.clear(); + + let ret = libc::dlsym(handle as *mut libc::c_void, symbol) as *mut u8; + + // A non-NULL return value *always* indicates success. There's no need + // to check `dlerror`. + if !ret.is_null() { + return Ok(ret); + } + + dlerror.get().map(|()| ret) } pub(super) unsafe fn close(handle: *mut u8) { diff --git a/src/librustc_metadata/lib.rs b/src/librustc_metadata/lib.rs index e50fa34554d51..85490f5f6e91a 100644 --- a/src/librustc_metadata/lib.rs +++ b/src/librustc_metadata/lib.rs @@ -5,6 +5,7 @@ #![feature(drain_filter)] #![feature(in_band_lifetimes)] #![feature(nll)] +#![feature(once_cell)] #![feature(or_patterns)] #![feature(proc_macro_internals)] #![feature(min_specialization)] From e2326a1eecfe5562712649e8696c579bc919a9af Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Sat, 22 Aug 2020 11:13:32 -0700 Subject: [PATCH 2/4] Treat a NULL return from `dlsym` as an error on illumos This works around behavior observed on illumos in #74469, in which foreign code (libc according to the OP) was racing with rustc to check `dlerror`. --- src/librustc_metadata/dynamic_lib.rs | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/librustc_metadata/dynamic_lib.rs b/src/librustc_metadata/dynamic_lib.rs index 6867097d37294..b49a145604652 100644 --- a/src/librustc_metadata/dynamic_lib.rs +++ b/src/librustc_metadata/dynamic_lib.rs @@ -107,11 +107,26 @@ mod dl { handle: *mut u8, symbol: *const libc::c_char, ) -> Result<*mut u8, String> { + // HACK(#74469): On some platforms, users observed foreign code + // (specifically libc) invoking `dlopen`/`dlsym` in parallel with the + // functions in this module. This is problematic because, according to + // the POSIX API documentation, `dlerror` must be called to determine + // whether `dlsym` succeeded. Unlike `dlopen`, a NULL return value may + // indicate a successfully resolved symbol with an address of zero. + // + // Because symbols with address zero shouldn't occur in practice, we + // treat them as errors on platforms with misbehaving libc + // implementations. + const DLSYM_NULL_IS_ERROR: bool = cfg!(target_os = "illumos"); + let mut dlerror = error::lock(); - // Flush `dlerror` since we need to use it to determine whether the subsequent call to - // `dlsym` is successful. - dlerror.clear(); + // No need to flush `dlerror` if we aren't using it to determine whether + // the subsequent call to `dlsym` succeeded. If an error occurs, any + // stale value will be overwritten. + if !DLSYM_NULL_IS_ERROR { + dlerror.clear(); + } let ret = libc::dlsym(handle as *mut libc::c_void, symbol) as *mut u8; @@ -121,7 +136,12 @@ mod dl { return Ok(ret); } - dlerror.get().map(|()| ret) + match dlerror.get() { + Ok(()) if DLSYM_NULL_IS_ERROR => Err("Unknown error".to_string()), + Ok(()) => Ok(ret), + + Err(msg) => Err(msg), + } } pub(super) unsafe fn close(handle: *mut u8) { From f07011bad8a4ca3bd65ce1ed236f017f55108816 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 25 Aug 2020 12:02:21 -0700 Subject: [PATCH 3/4] Always treat `dlsym` returning NULL as an error This simplifies the code somewhat. Also updates comments to reflect notes from reviw about thread-safety of `dlerror`. --- src/librustc_metadata/dynamic_lib.rs | 55 ++++++++++++---------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/src/librustc_metadata/dynamic_lib.rs b/src/librustc_metadata/dynamic_lib.rs index b49a145604652..8c3c7b70f6c7c 100644 --- a/src/librustc_metadata/dynamic_lib.rs +++ b/src/librustc_metadata/dynamic_lib.rs @@ -54,8 +54,16 @@ mod dl { use std::ffi::{CString, OsStr}; use std::os::unix::prelude::*; - // `dlerror` is process global, so we can only allow a single thread at a - // time to call `dlsym` and `dlopen` if we want to check the error message. + // As of the 2017 revision of the POSIX standard (IEEE 1003.1-2017), it is + // implementation-defined whether `dlerror` is thread-safe (in which case it returns the most + // recent error in the calling thread) or not thread-safe (in which case it returns the most + // recent error in *any* thread). + // + // There's no easy way to tell what strategy is used by a given POSIX implementation, so we + // lock around all calls that can modify `dlerror` in this module lest we accidentally read an + // error from a different thread. This is bulletproof when we are the *only* code using the + // dynamic library APIs at a given point in time. However, it's still possible for us to race + // with other code (see #74469) on platforms where `dlerror` is not thread-safe. mod error { use std::ffi::CStr; use std::lazy::SyncLazy; @@ -97,9 +105,9 @@ mod dl { return Ok(ret); } - // A NULL return from `dlopen` indicates that an error has - // definitely occurred, so if nothing is in `dlerror`, we are - // racing with another thread that has stolen our error message. + // A NULL return from `dlopen` indicates that an error has definitely occurred, so if + // nothing is in `dlerror`, we are racing with another thread that has stolen our error + // message. See the explanation on the `dl::error` module for more information. dlerror.get().and_then(|()| Err("Unknown error".to_string())) } @@ -107,41 +115,26 @@ mod dl { handle: *mut u8, symbol: *const libc::c_char, ) -> Result<*mut u8, String> { - // HACK(#74469): On some platforms, users observed foreign code - // (specifically libc) invoking `dlopen`/`dlsym` in parallel with the - // functions in this module. This is problematic because, according to - // the POSIX API documentation, `dlerror` must be called to determine - // whether `dlsym` succeeded. Unlike `dlopen`, a NULL return value may - // indicate a successfully resolved symbol with an address of zero. - // - // Because symbols with address zero shouldn't occur in practice, we - // treat them as errors on platforms with misbehaving libc - // implementations. - const DLSYM_NULL_IS_ERROR: bool = cfg!(target_os = "illumos"); - let mut dlerror = error::lock(); - // No need to flush `dlerror` if we aren't using it to determine whether - // the subsequent call to `dlsym` succeeded. If an error occurs, any - // stale value will be overwritten. - if !DLSYM_NULL_IS_ERROR { - dlerror.clear(); - } + // Unlike `dlopen`, it's possible for `dlsym` to return NULL without overwriting `dlerror`. + // Because of this, we clear `dlerror` before calling `dlsym` to avoid picking up a stale + // error message by accident. + dlerror.clear(); let ret = libc::dlsym(handle as *mut libc::c_void, symbol) as *mut u8; - // A non-NULL return value *always* indicates success. There's no need - // to check `dlerror`. if !ret.is_null() { return Ok(ret); } - match dlerror.get() { - Ok(()) if DLSYM_NULL_IS_ERROR => Err("Unknown error".to_string()), - Ok(()) => Ok(ret), - - Err(msg) => Err(msg), - } + // If `dlsym` returns NULL but there is nothing in `dlerror` it means one of two things: + // - We tried to load a symbol mapped to address 0. This is not technically an error but is + // unlikely to occur in practice and equally unlikely to be handled correctly by calling + // code. Therefore we treat it as an error anyway. + // - An error has occurred, but we are racing with another thread that has stolen our error + // message. See the explanation on the `dl::error` module for more information. + dlerror.get().and_then(|()| Err("Tried to load symbol mapped to address 0".to_string())) } pub(super) unsafe fn close(handle: *mut u8) { From aae6c0fbfe3051e5539f47d0e9d84ddee53f72bd Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 25 Aug 2020 12:11:30 -0700 Subject: [PATCH 4/4] Explicitly pass `RTLD_LOCAL` to `dlopen` This happens to be the default on Linux, but the default is unspecified in the POSIX standard. Also switches to `cast` to keep line lengths in check. --- src/librustc_metadata/dynamic_lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc_metadata/dynamic_lib.rs b/src/librustc_metadata/dynamic_lib.rs index 8c3c7b70f6c7c..bdb53e3f75a40 100644 --- a/src/librustc_metadata/dynamic_lib.rs +++ b/src/librustc_metadata/dynamic_lib.rs @@ -99,10 +99,10 @@ mod dl { let s = CString::new(filename.as_bytes()).unwrap(); let mut dlerror = error::lock(); - let ret = unsafe { libc::dlopen(s.as_ptr(), libc::RTLD_LAZY) } as *mut u8; + let ret = unsafe { libc::dlopen(s.as_ptr(), libc::RTLD_LAZY | libc::RTLD_LOCAL) }; if !ret.is_null() { - return Ok(ret); + return Ok(ret.cast()); } // A NULL return from `dlopen` indicates that an error has definitely occurred, so if @@ -122,10 +122,10 @@ mod dl { // error message by accident. dlerror.clear(); - let ret = libc::dlsym(handle as *mut libc::c_void, symbol) as *mut u8; + let ret = libc::dlsym(handle as *mut libc::c_void, symbol); if !ret.is_null() { - return Ok(ret); + return Ok(ret.cast()); } // If `dlsym` returns NULL but there is nothing in `dlerror` it means one of two things: