Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consult dlerror() only if a dl*() call fails #74469

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 25 additions & 6 deletions src/librustc_metadata/dynamic_lib.rs
Expand Up @@ -63,9 +63,9 @@ mod dl {
})
}

fn check_for_errors_in<T, F>(f: F) -> Result<T, String>
fn check_for_errors_in<T, F>(f: F) -> Result<*mut T, String>
where
F: FnOnce() -> T,
F: FnOnce() -> *mut T,
{
use std::sync::{Mutex, Once};
static INIT: Once = Once::new();
Expand All @@ -77,16 +77,35 @@ mod dl {
// dlerror isn't thread safe, so we need to lock around this entire
// sequence
let _guard = (*LOCK).lock();

// dlerror reports the most recent failure that occured during a
// dynamic linking operation and then clears that error; we call
// once in advance of our operation in an attempt to discard any
// stale prior error report that may exist:
let _old_error = libc::dlerror();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this call still needed? Surely any prior error will be replaced if there's a new error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @ollie27 said, there's no need to do this anymore if we don't use the return value of dlerror to determine whether an error occurred.


let result = f();

let last_error = libc::dlerror() as *const _;
if ptr::null() == last_error {
// We should only check dlerror() in the event that the operation
// fails, which we determine by checking for a NULL return. This
// covers at least dlopen() and dlsym().
Comment on lines +89 to +91
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document these semantics at the function level? Specifically, if f returns a null pointer, this function returns Err with the string in dlerror.

Also, just to be sure, do all the functions we pass to this helper return NULL and only NULL to indicate an error? There's no (void *) 1 weirdness or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For dlsym at least, the current approach is explicitly recommended on linux and seems to be necessary on illumos as well, since NULL can indicate either a "symbol not found" error or a found symbol with the value NULL. We should be checking the return value of dlopen, but we will need to find a different workaround here.

//
// While we are able to exclude other callers within this library,
// we are not able to exclude external callers such as those in the
// system libraries. If dynamic linking activity is induced in
// another thread, it may destroy our dlerror() report or it may
// inject one that does not apply to us -- this error report must be
// treated as advisory.
if ptr::null() != result {
Ok(result)
} else {
Comment on lines +99 to 101
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the else block now has a condition inside, could you switch to an early return for the happy path?

let s = CStr::from_ptr(last_error).to_bytes();
Err(str::from_utf8(s).unwrap().to_owned())
let last_error = libc::dlerror() as *const _;
if ptr::null() == last_error {
Err("unknown dl error".to_string())
} else {
let s = CStr::from_ptr(last_error).to_bytes();
Err(str::from_utf8(s).unwrap().to_owned())
Comment on lines +106 to +107
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let s = CStr::from_ptr(last_error).to_bytes();
Err(str::from_utf8(s).unwrap().to_owned())
let s = CStr::from_ptr(last_error).to_str().unwrap();
Err(s.to_owned())

}
}
}
}
Expand Down