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.

Sign up for GitHub

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

Jump to bottom

Concurrency bugs caused by multiple linked-in versions of libloading #32

Closed
nagisa opened this issue Sep 23, 2017 · 7 comments
Closed

Concurrency bugs caused by multiple linked-in versions of libloading #32

nagisa opened this issue Sep 23, 2017 · 7 comments

Comments

@nagisa
Copy link
Owner

nagisa commented Sep 23, 2017

It is possible for multiple versions of libloading to be linked into a binary. In that case static mutexes, such as one used to guard uses of dlerror have no effect.
@nagisa
Copy link
Owner Author

nagisa commented Sep 23, 2017

Having a separate version 0.1 crate that contains a mutex shared by all versions of libloading would help here. Maybe there is a better solution?

@nagisa nagisa mentioned this issue Dec 6, 2017
Merged
@GabrielMajeri
Copy link
Contributor

GabrielMajeri commented Jan 3, 2018 •
edited

Perhaps we should be inspired by the log crate? I've done a simple test and using different versions of the log crate and it works without any issue.

And their locks are simple AtomicUsizes in the main crate, no workarounds used. Perhaps Rust automatically handles static variables from different versions of the crate?

@nagisa
Copy link
Owner Author

nagisa commented Jan 6, 2018

I see nothing in log crate which would be resistant to this issue – namely, it seems to me that it would be trivial to have two different global loggers set up in a binary that links to two different versions of log crates.

In log case, though, all that would happen is two distinct loggers at the same time which is nothing too serious.

@nagisa nagisa closed this as completed in d870a38 Jan 11, 2018
@acmcarther acmcarther mentioned this issue Mar 28, 2018
Open
@jethrogb
Copy link

jethrogb commented Dec 8, 2018

Why don't you use the links key in Cargo.toml? That was specifically designed to avoid issues where multiple crate versions link to the same native library. https://doc.rust-lang.org/cargo/reference/build-scripts.html#the-links-manifest-key

@nagisa
Copy link
Owner Author

nagisa commented Dec 8, 2018 •
edited

There quite a few reasons that make using links inappropriate in this case:

  1. There isn’t always a logical "library" to specify there. For example on linux dl* family of functions are provided by libc rather than a separate library. We could specify a dummy value of some sort, but that would still not prevent the same sort of issues from occurring if two distinct crates using similar functions are linked in, or this crate is used in a larger project that uses dl* functions independently...
  2. This issue is not relevant to most of the targets at all. Most of the targets have implemented dlerror in a thread-safe manner and e.g. Windows does not have this problem in the first place. Why would I strive to prevent users from doing valid things only to maintain safety for some obscure platform that implements an old version of the POSIX standard (or has a non-thread-safe dlerror)?
  3. Experience with other crates has been that links is an anti-pattern, especially with system libraries. winapi specifically had to go through a fairly painful version bump in the past after it has been found that links is not appropriate to it.

@jethrogb
Copy link

jethrogb commented Dec 8, 2018

  1. There isn’t always a logical "library" to specify there. For example on linux dl* family of functions are provided by libc rather than a separate library. We could specify a dummy value of some sort, but that would still not prevent the same sort of issues from occurring if two distinct crates using similar functions are linked in, or this crate is used in a larger project that uses dl* functions independently...
  2. This issue is not relevant to most of the targets at all. Most of the targets have implemented dlerror in a thread-safe manner and e.g. Windows does not have this problem in the first place. Why would I strive to prevent users from doing valid things only to maintain safety for some obscure platform that implements an old version of the POSIX standard (or has a non-thread-safe dlerror)?

The way to do this would be to create a separate dl-sys crate with the appropriate functions and links = ["dl"]. This crate could then optionally depend on that crate (with target-specific dependencies).

  1. Experience with other crates has been that links is an anti-pattern, especially with system libraries. winapi specifically had to go through a fairly painful version bump in the past after it has been found that links is not appropriate to it.

I see. This sounds like more of a concern. I tried to find more info on this, but couldn't really find anything. Is there a cargo issue discussing this?

@nagisa
Copy link
Owner Author

nagisa commented Dec 8, 2018

The way to do this would be to create a separate dl-sys crate with the appropriate functions and links = ["dl"]. This crate could then optionally depend on that crate (with target-specific dependencies).

It still successfully prevents valid use-cases on platforms that do implement the dl API, but in a MT-safe manner (which is most of them). Either way, I strongly prefer the solution that I ended up implementing for this library. It is technically superior and has better UX compared to links in about every way.

The only downside is that currently it requires a C compiler to stay in stable-land. Need for the C compiler can be removed later once the linkage attribute is stabilized in rustc.

I see. This sounds like more of a concern. I tried to find more info on this, but couldn't really find anything. Is there a cargo issue discussing this?

There is likely no cargo issue about this. There is retep998/winapi-rs#238 related to this, though.

ashie added a commit to webdino/meta-browser that referenced this issue Dec 18, 2018
@ashie
firefox: Avoid build error of rust_libloading
1ac08c7 
It reverts the following change of rust_libloading:

  nagisa/rust_libloading@d870a38

See also:

  nagisa/rust_libloading#32

The build error log of rust_libloading:

 0:01.78 Internal error occurred: Command "/build/ebisu-bsp/build/tmp/hosttools/gcc" "-std=gnu99" "-O2" "-ffunction-sections" "-fdata-sections
" "-fPIC" "-MD" "-MP" "-MF" ".deps/force-cargo-library-build.pp" "-m64" "-o" "/build/ebisu-bsp/build/tmp/work/aarch64-poky-linux/firefox/60.4.
0esr-r0/firefox-60.4.0/firefox-build-dir/toolkit/library/release/build/libloading-4bfcbe83545248d4/out/src/os/unix/global_static.o" "-c" "src/
os/unix/global_static.c" with args "gcc" did not execute successfully (status code exit code: 1).
 0:01.78
 0:01.78 ', /build/ebisu-bsp/build/tmp/work/aarch64-poky-linux/firefox/60.4.0esr-r0/firefox-60.4.0/third_party/rust/cc/src/lib.rs:2260:5
 0:01.78 stack backtrace:
 0:01.78    0:     0x5605d756ef8e - std::sys::unix::backtrace::tracing:imp:unwind_backtrace::he02ced09d6ef9634
 0:01.78                                at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
 0:01.78    1:     0x5605d7577a26 - std::sys_common::backtrace::print::h899c99cf51528fce
 0:01.78                                at libstd/sys_common/backtrace.rs:71
 0:01.78                                at libstd/sys_common/backtrace.rs:59
 0:01.78    2:     0x5605d755d70b - std::panicking::default_hook::{{closure}}::h1293938f39991188
 0:01.78                                at libstd/panicking.rs:211
 0:01.78    3:     0x5605d755d3e3 - std::panicking::default_hook::h21d85e0fe60e7542
 0:01.78                                at libstd/panicking.rs:227
 0:01.78    4:     0x5605d755dd2e - std::panicking::rust_panic_with_hook::heef4f32e5b81276f
 0:01.78                                at libstd/panicking.rs:463
 0:01.78    5:     0x5605d755d8cc - std::panicking::begin_panic_fmt::h7b91617991a35914
 0:01.78                                at libstd/panicking.rs:350
 0:01.78    6:     0x5605d755a111 - cc::fail::hb8a3db7ae214ddd8
 0:01.78    7:     0x5605d754eb1e - cc::Build::compile::hec719bba1d1593cd
 0:01.78    8:     0x5605d7540c01 - build_script_build::main::h83038a714584dca2
 0:01.78    9:     0x5605d753fd92 - std::rt::lang_start::{{closure}}::hc8b1f864a05e672f
 0:01.78   10:     0x5605d755d812 - std::panicking::try::do_call::h9ce18ce7a2108605
 0:01.78                                at libstd/rt.rs:59
 0:01.78                                at libstd/panicking.rs:310
 0:01.78   11:     0x5605d757eab9 - __rust_maybe_catch_panic
 0:01.78                                at libpanic_unwind/lib.rs:105
 0:01.78   12:     0x5605d75666c5 - std::rt::lang_start_internal::h93415d3576bc8676
 0:01.78                                at libstd/panicking.rs:289
 0:01.78                                at libstd/panic.rs:374
 0:01.78                                at libstd/rt.rs:58
 0:01.78   13:     0x5605d7540d93 - main
 0:01.78   14:     0x7fbd1ab7e82f - __libc_start_main
 0:01.78   15:     0x5605d753f978 - _start
 0:01.78   16:                0x0 - <unknown>

Signed-off-by: Takuro Ashie <ashie@clear-code.com>
ashie added a commit to webdino/meta-browser that referenced this issue Dec 18, 2018
@ashie
firefox: Avoid build error of rust_libloading
1c86759 
It reverts the following change of rust_libloading:

  nagisa/rust_libloading@d870a38

See also:

  nagisa/rust_libloading#32

The build error log of rust_libloading:

 0:01.78 Internal error occurred: Command "/build/ebisu-bsp/build/tmp/hosttools/gcc" "-std=gnu99" "-O2" "-ffunction-sections" "-fdata-sections
" "-fPIC" "-MD" "-MP" "-MF" ".deps/force-cargo-library-build.pp" "-m64" "-o" "/build/ebisu-bsp/build/tmp/work/aarch64-poky-linux/firefox/60.4.
0esr-r0/firefox-60.4.0/firefox-build-dir/toolkit/library/release/build/libloading-4bfcbe83545248d4/out/src/os/unix/global_static.o" "-c" "src/
os/unix/global_static.c" with args "gcc" did not execute successfully (status code exit code: 1).
 0:01.78
 0:01.78 ', /build/ebisu-bsp/build/tmp/work/aarch64-poky-linux/firefox/60.4.0esr-r0/firefox-60.4.0/third_party/rust/cc/src/lib.rs:2260:5
 0:01.78 stack backtrace:
 0:01.78    0:     0x5605d756ef8e - std::sys::unix::backtrace::tracing:imp:unwind_backtrace::he02ced09d6ef9634
 0:01.78                                at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
 0:01.78    1:     0x5605d7577a26 - std::sys_common::backtrace::print::h899c99cf51528fce
 0:01.78                                at libstd/sys_common/backtrace.rs:71
 0:01.78                                at libstd/sys_common/backtrace.rs:59
 0:01.78    2:     0x5605d755d70b - std::panicking::default_hook::{{closure}}::h1293938f39991188
 0:01.78                                at libstd/panicking.rs:211
 0:01.78    3:     0x5605d755d3e3 - std::panicking::default_hook::h21d85e0fe60e7542
 0:01.78                                at libstd/panicking.rs:227
 0:01.78    4:     0x5605d755dd2e - std::panicking::rust_panic_with_hook::heef4f32e5b81276f
 0:01.78                                at libstd/panicking.rs:463
 0:01.78    5:     0x5605d755d8cc - std::panicking::begin_panic_fmt::h7b91617991a35914
 0:01.78                                at libstd/panicking.rs:350
 0:01.78    6:     0x5605d755a111 - cc::fail::hb8a3db7ae214ddd8
 0:01.78    7:     0x5605d754eb1e - cc::Build::compile::hec719bba1d1593cd
 0:01.78    8:     0x5605d7540c01 - build_script_build::main::h83038a714584dca2
 0:01.78    9:     0x5605d753fd92 - std::rt::lang_start::{{closure}}::hc8b1f864a05e672f
 0:01.78   10:     0x5605d755d812 - std::panicking::try::do_call::h9ce18ce7a2108605
 0:01.78                                at libstd/rt.rs:59
 0:01.78                                at libstd/panicking.rs:310
 0:01.78   11:     0x5605d757eab9 - __rust_maybe_catch_panic
 0:01.78                                at libpanic_unwind/lib.rs:105
 0:01.78   12:     0x5605d75666c5 - std::rt::lang_start_internal::h93415d3576bc8676
 0:01.78                                at libstd/panicking.rs:289
 0:01.78                                at libstd/panic.rs:374
 0:01.78                                at libstd/rt.rs:58
 0:01.78   13:     0x5605d7540d93 - main
 0:01.78   14:     0x7fbd1ab7e82f - __libc_start_main
 0:01.78   15:     0x5605d753f978 - _start
 0:01.78   16:                0x0 - <unknown>

Signed-off-by: Takuro Ashie <ashie@clear-code.com>
@matklad matklad mentioned this issue Apr 1, 2020
Closed
@bjorn3 bjorn3 mentioned this issue Apr 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nagisa @jethrogb @GabrielMajeri