native-lib: allow multiple libraries and/or dirs #4372
Conversation
src/shims/native_lib.rs
Outdated
| return None; | ||
| // FIXME: this is a hack! | ||
| // The `libloading` crate will automatically load system libraries like `libc`. | ||
| // On linux `libloading` is based on `dlsym`: https://docs.rs/libloading/0.7.3/src/libloading/os/unix/mod.rs.html#202 | ||
| // and `dlsym`(https://linux.die.net/man/3/dlsym) looks through the dependency tree of the | ||
| // library if it can't find the symbol in the library itself. | ||
| // So, in order to check if the function was actually found in the specified | ||
| // `machine.external_so_lib` we need to check its `dli_fname` and compare it to | ||
| // the specified SO file path. | ||
| // This code is a reimplementation of the mechanism for getting `dli_fname` in `libloading`, | ||
| // from: https://docs.rs/libloading/0.7.3/src/libloading/os/unix/mod.rs.html#411 | ||
| // using the `libc` crate where this interface is public. | ||
| let mut info = std::mem::MaybeUninit::<libc::Dl_info>::zeroed(); | ||
| unsafe { | ||
| if libc::dladdr(fn_ptr, info.as_mut_ptr()) != 0 { | ||
| let info = info.assume_init(); | ||
| #[cfg(target_os = "cygwin")] | ||
| let fname_ptr = info.dli_fname.as_ptr(); | ||
| #[cfg(not(target_os = "cygwin"))] | ||
| let fname_ptr = info.dli_fname; | ||
| assert!(!fname_ptr.is_null()); | ||
| if std::ffi::CStr::from_ptr(fname_ptr).to_str().unwrap() | ||
| != lib_path.to_str().unwrap() | ||
| { | ||
| continue; |
There was a problem hiding this comment.
if we've found the symbol, but the loading fails, should that really just continue to the next lib?
There was a problem hiding this comment.
Fair enough, I guess it's on the user to make sure not to pass us any broken/wonky shared objects. I'll change that
There was a problem hiding this comment.
I think continue actually makes sense here... this check failing means the symbol wasn't found in this library (only in one of its dependencies). The symbol might still be found in another library.
This is not about the loading of the library failing.
There was a problem hiding this comment.
I can change that back then and put an else on the dladdr bit which returns none to address the other comment also?
There was a problem hiding this comment.
I think the else branch should panic, TBH.
nia-e
force-pushed
the
multiple-native-libs
branch
from
9ca54ce to
88e0787
Compare
| Unix systems. Functions not provided by that file are still executed via the usual Miri shims. | ||
| Unix systems. Functions not provided by that file are still executed via the usual Miri shims. If | ||
| a path to a directory is specified, all files in that directory are included nonrecursively. This | ||
| flag can be passed multiple times to specify multiple files and/or directories. |
There was a problem hiding this comment.
What does passing a directory here even do?
There was a problem hiding this comment.
-Zmiri-native-lib=/path/to/dir/ -> -Zmiri-native-lib=/path/to/dir/file1 -Zmiri-native-lib=/path/to/dir/file2 -Zmiri-native-lib=/path/to/dir/file3 ...
There was a problem hiding this comment.
Okay; the docs should say that.
There was a problem hiding this comment.
Oh wait, they do, I just missed it. 🤦
| // using the `libc` crate where this interface is public. | ||
| let mut info = std::mem::MaybeUninit::<libc::Dl_info>::zeroed(); | ||
| unsafe { | ||
| if libc::dladdr(fn_ptr, info.as_mut_ptr()) != 0 { |
There was a problem hiding this comment.
The more concerning point is that we just ignore dladdr failing...
There was a problem hiding this comment.
Do we want to panic on fail? With the current dir implementation it would allow e.g. passing a directory which also contains things that aren't libraries and it will just ignore those. For instance it could be a directory of random build artifacts from some library, where only some are actually .so files, and this will transparently ignore all other files
There was a problem hiding this comment.
I already implemented this in #4389. At least for single-file arguments I think it should definitely be an error if that file cannot be loaded.
3 participants
Since I'm expanding native-lib mode it will hopefully become more useful and we may want to pass in more complex libraries with dependencies. This makes that possible. Since defining the same symbol multiple times in the one native-lib was already UB, I didn't add a note warning against defining the same symbol multiple times across libs but maybe I should?
I'm also looking into how we could use the ptrace stuff to lift some of the other requirements, like how native-lib mode is not allowed to spawn threads that run concommittantly with the Rust code - since we're ptracing we might be able to e.g. park all C threads, let Miri do its thing for x amount of time, then re-enable FFI mode and let the foreign threads do their thing for x amount of time and track their accesses. This way some much more complex multithreaded programs might be runnable. But, for now that's in the future.