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

Update dependencies and fix unsoundness and CI failure #29

Merged
merged 13 commits into from
Jun 19, 2022

Conversation

taiki-e
Copy link
Contributor

@taiki-e taiki-e commented Dec 22, 2020

No description provided.

@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #29 (5d85f31) into master (05f397e) will decrease coverage by 5.07%.
The diff coverage is 81.48%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   91.00%   85.92%   -5.08%     
==========================================
  Files           2        2              
  Lines         289      341      +52     
==========================================
+ Hits          263      293      +30     
- Misses         26       48      +22     
Impacted Files Coverage Δ
src/lib.rs 78.13% <81.48%> (-6.43%) ⬇️
tests/lib.rs 99.20% <0.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca8b16f...5d85f31. Read the comment docs.

@taiki-e
Copy link
Contributor Author

taiki-e commented Dec 22, 2020

Hmm, Miri reports UB, but this is a Stacked Borrows violation.

error: Undefined Behavior: trying to reborrow for SharedReadOnly at alloc83270+0x8, but parent tag <212137> does not have an appropriate item in the borrow stack
   --> /home/vsts/work/1/s/src/lib.rs:207:14
    |
207 |         drop(state);
    |              ^^^^^ trying to reborrow for SharedReadOnly at alloc83270+0x8, but parent tag <212137> does not have an appropriate item in the borrow stack
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
            
    = note: inside `bus::Seat::<bool>::take` at /home/vsts/work/1/s/src/lib.rs:207:14
    = note: inside `bus::BusReader::<bool>::recv_inner` at /home/vsts/work/1/s/src/lib.rs:659:19
    = note: inside `bus::BusReader::<bool>::try_recv` at /home/vsts/work/1/s/src/lib.rs:707:9
note: inside `it_detects_closure` at tests/lib.rs:105:16
   --> tests/lib.rs:105:16
    |
105 |     assert_eq!(rx.try_recv(), Ok(true));
    |                ^^^^^^^^^^^^^
note: inside closure at tests/lib.rs:101:1
   --> tests/lib.rs:101:1
    |
101 | / fn it_detects_closure() {
102 | |     let mut tx = bus::Bus::new(1);
103 | |     let mut rx = tx.add_rx();
104 | |     assert_eq!(tx.try_broadcast(true), Ok(()));
...   |
108 | |     assert_eq!(rx.try_recv(), Err(mpsc::TryRecvError::Disconnected));
109 | | }
    | |_^
    = note: inside `<[closure@tests/lib.rs:101:1: 109:2] as std::ops::FnOnce<()>>::call_once - shim` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
    = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
    = note: inside `test::__rust_begin_short_backtrace::<fn()>` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:516:5
    = note: inside closure at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:507:30
    = note: inside `<[closure@test::run_test::{closure#2}] as std::ops::FnOnce<()>>::call_once - shim(vtable)` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
    = note: inside `<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send> as std::ops::FnOnce<()>>::call_once` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1328:9
    = note: inside `<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>> as std::ops::FnOnce<()>>::call_once` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:322:9
    = note: inside `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:379:40
    = note: inside `std::panicking::r#try::<(), std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>>` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:343:19
    = note: inside `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:396:14
    = note: inside `test::run_test_in_process` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:538:18
    = note: inside closure at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:449:39
    = note: inside `test::run_test::run_test_inner` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:474:13
    = note: inside `test::run_test` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:504:28
    = note: inside `test::run_tests::<[closure@test::run_tests_console::{closure#2}]>` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:283:13
    = note: inside `test::run_tests_console` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/console.rs:289:5
    = note: inside `test::test_main` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:121:15
    = note: inside `test::test_main_static` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/test/src/lib.rs:140:5
    = note: inside `main`
    = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
    = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:125:18
    = note: inside closure at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:66:18
    = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:259:13
    = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:379:40
    = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:343:19
    = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:396:14
    = note: inside `std::rt::lang_start_internal` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:51:25
    = note: inside `std::rt::lang_start::<()>` at /usr/share/rust/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:65:5
    = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error; 1 warning emitted

error: test failed, to rerun pass '--test lib'

@jonhoo
Copy link
Owner

jonhoo commented Dec 25, 2020

@taiki-e Thanks! How should I think about this miri error? Like a likely false positive, or as a likely bug? It's been a while since I looked at this code, so I totally believe that it might contain bugs, but don't want to page it all in if this has a high chance of being a false positive 😅

@taiki-e
Copy link
Contributor Author

taiki-e commented Dec 26, 2020

I read a bit of the code, but I think the problem is probably due to we mutably accessing the value that already immutably referenced (i.e., I think this is a bug):

First, we get an immutable reference to the state:

bus/src/lib.rs

Lines 159 to 177 in 05f397e

// the writer will only modify this element when .read hits .max - writer.rleft[i]. we can
// be sure that this is not currently the case (which means it's safe for us to read)
// because:
//
// - .max is set to the number of readers at the time when the write happens
// - any joining readers will start at a later seat
// - so, at most .max readers will call .take() on this seat this time around the buffer
// - a reader must leave either *before* or *after* a call to recv. there are two cases:
//
// - it leaves before, rleft is decremented, but .take is not called
// - it leaves after, .take is called, but head has been incremented, so rleft will be
// decremented for the *next* seat, not this one
//
// so, either .take is called, and .read is incremented, or writer.rleft is incremented.
// thus, for a writer to modify this element, *all* readers at the time of the previous
// write to this seat must have either called .take or have left.
// - since we are one of those readers, this cannot be true, so it's safe for us to assume
// that there is no concurrent writer for this seat
let state = unsafe { &*self.state.get() };

Next, we are calling self.state.get() on this line and modifying (.take()) the returned state. Note that the immutable reference that we first got is still alive when this is called.

bus/src/lib.rs

Lines 194 to 196 in 05f397e

// since we're the last reader, no-one else will be cloning this value, so we can
// safely take a mutable reference, and just take the val instead of cloning it.
unsafe { &mut *self.state.get() }.val.take().unwrap()

Finally, drop the immutable reference that we first got. Dropping reference is a no-op. However, this tells the compiler that the reference is valid at least up to this line.

bus/src/lib.rs

Lines 204 to 207 in 05f397e

// let writer know that we no longer need this item.
// state is no longer safe to access.
#[allow(clippy::drop_ref)]
drop(state);

This can be fixed by removing drop(state) or moving it before mutable access.
I'll push a fix.

@taiki-e
Copy link
Contributor Author

taiki-e commented Dec 26, 2020

Hmm, There is another Miri error. (This is likely not a bug).

error: unsupported operation: the main thread terminated without waiting for other threads
  |
  = note: the main thread terminated without waiting for other threads
  = note: (no span available)
  = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support

I'm not sure what's happening, but I'll always get this error unless I disable all tests... 😅

@taiki-e
Copy link
Contributor Author

taiki-e commented Dec 26, 2020

= note: the main thread terminated without waiting for other threads

Considering that the following code also causes an error, probably related to the thread spawned in Bus::new, but I'm not sure if there is a workaround other than disabling Miri test.

#[test]
fn it_fails_when_full() {
    let _x = bus::Bus::<bool>::new(1);
}

bus/src/lib.rs

Lines 301 to 305 in 05f397e

thread::spawn(move || {
for t in unpark_rx.iter() {
t.unpark();
}
});

src/lib.rs Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Dec 27, 2020

Ouch, that's a pretty significant limitation. What program captures all of its running threads! Not sure what is best to do about this. One might be to have a test-only constructor that returns the Bus and a FnOnce + Send + 'static that the caller should spawn (and which tests can then wait for)? Is there a tracking issue for this limitation on the miri side @RalfJung?

@RalfJung
Copy link
Contributor

Yes: rust-lang/miri#1371

@jonhoo
Copy link
Owner

jonhoo commented Dec 31, 2020

Thanks Ralf! Is there a way for us to disable that panic for the time being? I would like to do this update if possible, but not if it means miri won't run.

@RalfJung
Copy link
Contributor

I suppose we could add a flag which ignores "thread leaks" -- it would have to also disable the memory leak checker though since otherwise the stack and thread-local storage of that other thread would all be considered leaked.

Do you have any idea what should happen wrt. thread-local destructors? Presumably they simply do not run for "leaked" threads?

I do have some other patches I want to get done before the end of my vacation though, so I cannot promise a timeline.

@jonhoo
Copy link
Owner

jonhoo commented Jan 1, 2021

@RalfJung I believe Rust will actually run the destructors for thread-locals in all threads on main exit using thread-local destructors (a feature provided by glibc under the name __cxa_thread_atexit_impl): https://github.com/rust-lang/rust/blob/0d97f7a96877a96015d70ece41ad08bb7af12377/library/std/src/sys/unix/thread_local_dtor.rs. This is also suggested by the UNIX implementation for thread locals, which uses pthread_key_create, which in turn says it will run the destructors on exit.

@RalfJung
Copy link
Contributor

RalfJung commented Jan 1, 2021

That should be the destructors for the main thread... wouldn't it be unsound to run other thread's thread local destructors? They could be in use by that thread!

Anyway that sounds more like a discussion for rust-lang/miri#1371 than for this issue.

@lespea
Copy link

lespea commented Jun 16, 2022

Any update on this by chance? There's now a vulnerability in the version of crossbeam-utils that this package depends on (crossbeam-rs/crossbeam#781)

@taiki-e
Copy link
Contributor Author

taiki-e commented Jun 17, 2022

  • Miri currently fails in various projects due to two issues. Although it is probably possible to work around them by disabling weak memory emulation and preemption.
  • This crate is not affected by crossbeam#781.
  • This crate depends on atomic-option with vulnerabilities that could affect this crate.
  • The latest version of parking_lot_core is 0.9 and it requires Rust 1.49.
  • CI uses ubuntu 16.04 which is no longer supported.

@taiki-e taiki-e changed the title Update crossbeam-channel to 0.5, parking_lot_core to 0.8 Update crossbeam-channel to 0.5, parking_lot_core to 0.9 Jun 17, 2022
@taiki-e taiki-e force-pushed the deps branch 2 times, most recently from 483911d to ae7de3f Compare June 17, 2022 03:39
@taiki-e taiki-e changed the title Update crossbeam-channel to 0.5, parking_lot_core to 0.9 Update dependencies and fix unsoundness Jun 17, 2022
@taiki-e taiki-e changed the title Update dependencies and fix unsoundness Update dependencies and fix unsoundness snd CI failure Jun 17, 2022
@taiki-e taiki-e force-pushed the deps branch 4 times, most recently from afef3c9 to 365c3f4 Compare June 17, 2022 04:59
@taiki-e taiki-e changed the title Update dependencies and fix unsoundness snd CI failure Update dependencies and fix unsoundness and CI failure Jun 17, 2022
@taiki-e
Copy link
Contributor Author

taiki-e commented Jun 17, 2022

CI is now green, except codecov which is angry that the comments (and impl blocks with no items) I added were not covered.

cov

@taiki-e taiki-e requested a review from jonhoo June 17, 2022 05:27
- bash: yes | cargo miri -Zmiri-ignore-leaks test
# disable preemption due to https://github.com/rust-lang/rust/issues/55005
# disable weak memory emulation due to https://github.com/rust-lang/miri/issues/2223
- bash: MIRIFLAGS="-Zmiri-disable-isolation -Zmiri-ignore-leaks -Zmiri-preemption-rate=0 -Zmiri-disable-weak-memory-emulation" cargo miri test
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to disable isolation now when that was not needed before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is needed for recv_timeout that uses Instant. (recv_timeout has only been tested at doc test, so that flag was not needed before.)

error: unsupported operation: `clock_gettime` not available when isolation is enabled
   --> /home/vsts/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/time.rs:341:26
    |
341 |             cvt(unsafe { libc::clock_gettime(clock, t.as_mut_ptr()) }).unwrap();
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `clock_gettime` not available when isolation is enabled
    |
    = help: pass the flag `-Zmiri-disable-isolation` to disable isolation;
    = help: or pass `-Zmiri-isolation-error=warn` to configure Miri to return an error code from isolated operations (if supported for that operation) and continue with a warning
            
    = note: inside `std::sys::unix::time::inner::<impl std::sys::unix::time::Timespec>::now` at /home/vsts/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/time.rs:341:26
    = note: inside `std::sys::unix::time::inner::Instant::now` at /home/vsts/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/time.rs:275:26
    = note: inside `std::time::Instant::now` at /home/vsts/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/time.rs:276:17
note: inside `bus::BusReader::<bool>::recv_inner` at /home/vsts/work/1/s/src/lib.rs:587:47
   --> /home/vsts/work/1/s/src/lib.rs:587:47
    |
587 |             RecvCondition::Timeout(_) => Some(time::Instant::now()),
    |                                               ^^^^^^^^^^^^^^^^^^^^
note: inside `bus::BusReader::<bool>::recv_timeout` at /home/vsts/work/1/s/src/lib.rs:762:9
   --> /home/vsts/work/1/s/src/lib.rs:762:9
    |
762 |         self.recv_inner(RecvCondition::Timeout(timeout))
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `main::_doctest_main_src_lib_rs_747_0` at src/lib.rs:12:44
   --> src/lib.rs:756:44
    |
12  | assert_eq!(Err(RecvTimeoutError::Timeout), rx.recv_timeout(timeout));
    |                                            ^^^^^^^^^^^^^^^^^^^^^^^^

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense.

Btw, the Futex issue is fixed in Miri (should ship with the next nightly).

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Amazing, thank you for following up on this after all this time ❤️

@jonhoo jonhoo merged commit 6e41fb6 into jonhoo:master Jun 19, 2022
@jonhoo
Copy link
Owner

jonhoo commented Jun 19, 2022

Released in 2.2.4 🎉

@taiki-e taiki-e deleted the deps branch June 19, 2022 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants