-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Fix VecDeque::shrink_to
and add tests.
#108475
Conversation
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine.
r? @thomcc |
This adds both a test specific to rust-lang#108453 as well as an exhaustive test that goes through all possible combinations of head index, length and target capacity for a deque with capacity 16.
881db3f
to
4a4f43e
Compare
Looks great. Thanks for the fix. @bors r+ |
@@ -944,65 +944,72 @@ impl<T, A: Allocator> VecDeque<T, A> { | |||
return; | |||
} | |||
|
|||
if target_cap < self.capacity() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, heh, guess this wasn't ever doing anything, after the check on 943.
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#107941 (Treat `str` as containing `[u8]` for auto trait purposes) - rust-lang#108299 (Require `literal`s for some `(u)int_impl!` parameters) - rust-lang#108337 (hir-analysis: make a helpful note) - rust-lang#108379 (Add `ErrorGuaranteed` to `hir::{Expr,Ty}Kind::Err` variants) - rust-lang#108418 (Replace parse_[sth]_expr with parse_expr_[sth] function names) - rust-lang#108424 (rustc_infer: Consolidate obligation elaboration de-duplication) - rust-lang#108475 (Fix `VecDeque::shrink_to` and add tests.) - rust-lang#108482 (statically guarantee that current error codes are documented) - rust-lang#108484 (Remove `from` lang item) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
[beta] stage0 bump and backports - Bump stage0 to stable - Revert to using `RtlGenRandom` as a fallback rust-lang#108060 - Fix `VecDeque::shrink_to` and add tests. rust-lang#108475 - Fix `VecDeque::append` capacity overflow for ZSTs rust-lang#108462 - Clippy: Fix array-size-threshold config deserialization error rust-lang#108673 / rust-lang/rust-clippy#10423 - Yeet point_at_expr_source_of_inferred_type for now rust-lang#108703 r? `@ghost`
## Issue Addressed There was a [`VecDeque` bug](rust-lang/rust#108453) in some recent versions of the Rust standard library (1.67.0 & 1.67.1) that could cause Lighthouse to panic (reported by `@Sea Monkey` on discord). See full logs below. The issue was likely introduced in Rust 1.67.0 and [fixed](rust-lang/rust#108475) in 1.68, and we were able to reproduce the panic ourselves using [@michaelsproul's fuzz tests](https://github.com/michaelsproul/lighthouse/blob/fuzz-lru-time-cache/beacon_node/lighthouse_network/src/peer_manager/fuzz.rs#L111) on both Rust 1.67.0 and 1.67.1. Users that uses our Docker images or binaries are unlikely affected, as our Docker images were built with `1.66`, and latest binaries were built with latest stable (`1.68.2`). It likely impacts user that builds from source using Rust versions 1.67.x. ## Proposed Changes Bump Rust version (MSRV) to latest stable `1.68.2`. ## Additional Info From `@Sea Monkey` on Lighthouse Discord: > Crash on goerli using `unstable` `dd124b2d6804d02e4e221f29387a56775acccd08` ``` thread 'tokio-runtime-worker' panicked at 'Key must exist', /mnt/goerli/goerli/lighthouse/common/lru_cache/src/time.rs:68:28 stack backtrace: Apr 15 09:37:36.993 WARN Peer sent invalid block in single block lookup, peer_id: 16Uiu2HAm6ZuyJpVpR6y51X4Enbp8EhRBqGycQsDMPX7e5XfPYznG, error: WouldRevertFinalizedSlot { block_slot: Slot(5420212), finalized_slot: Slot(5420224) }, root: 0x10f6…3165, service: sync 0: rust_begin_unwind at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:575:5 1: core::panicking::panic_fmt at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:64:14 2: core::panicking::panic_display at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:135:5 3: core::panicking::panic_str at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:119:5 4: core::option::expect_failed at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/option.rs:1879:5 5: lru_cache::time::LRUTimeCache<Key>::raw_remove 6: lighthouse_network::peer_manager::PeerManager<TSpec>::handle_ban_operation 7: lighthouse_network::peer_manager::PeerManager<TSpec>::handle_score_action 8: lighthouse_network::peer_manager::PeerManager<TSpec>::report_peer 9: network::service::NetworkService<T>::spawn_service::{{closure}} 10: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll 11: <futures_util::future::future::map::Map<Fut,F> as core::future::future::Future>::poll 12: <futures_util::future::future::flatten::Flatten<Fut,<Fut as core::future::future::Future>::Output> as core::future::future::Future>::poll 13: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut 14: tokio::runtime::task::core::Core<T,S>::poll 15: tokio::runtime::task::harness::Harness<T,S>::poll 16: tokio::runtime::scheduler::multi_thread::worker::Context::run_task 17: tokio::runtime::scheduler::multi_thread::worker::Context::run 18: tokio::macros::scoped_tls::ScopedKey<T>::set 19: tokio::runtime::scheduler::multi_thread::worker::run 20: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut 21: tokio::runtime::task::core::Core<T,S>::poll 22: tokio::runtime::task::harness::Harness<T,S>::poll 23: tokio::runtime::blocking::pool::Inner::run note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. Apr 15 09:37:37.069 INFO Saved DHT state service: network Apr 15 09:37:37.070 INFO Network service shutdown service: network Apr 15 09:37:37.132 CRIT Task panic. This is a bug! advice: Please check above for a backtrace and notify the developers, message: <none>, task_name: network Apr 15 09:37:37.132 INFO Internal shutdown received reason: Panic (fatal error) Apr 15 09:37:37.133 INFO Shutting down.. reason: Failure("Panic (fatal error)") Apr 15 09:37:37.135 WARN Unable to free worker error: channel closed, msg: did not free worker, shutdown may be underway Apr 15 09:37:39.350 INFO Saved beacon chain to disk service: beacon Panic (fatal error) ```
## Issue Addressed There was a [`VecDeque` bug](rust-lang/rust#108453) in some recent versions of the Rust standard library (1.67.0 & 1.67.1) that could cause Lighthouse to panic (reported by `@Sea Monkey` on discord). See full logs below. The issue was likely introduced in Rust 1.67.0 and [fixed](rust-lang/rust#108475) in 1.68, and we were able to reproduce the panic ourselves using [@michaelsproul's fuzz tests](https://github.com/michaelsproul/lighthouse/blob/fuzz-lru-time-cache/beacon_node/lighthouse_network/src/peer_manager/fuzz.rs#L111) on both Rust 1.67.0 and 1.67.1. Users that uses our Docker images or binaries are unlikely affected, as our Docker images were built with `1.66`, and latest binaries were built with latest stable (`1.68.2`). It likely impacts user that builds from source using Rust versions 1.67.x. ## Proposed Changes Bump Rust version (MSRV) to latest stable `1.68.2`. ## Additional Info From `@Sea Monkey` on Lighthouse Discord: > Crash on goerli using `unstable` `dd124b2d6804d02e4e221f29387a56775acccd08` ``` thread 'tokio-runtime-worker' panicked at 'Key must exist', /mnt/goerli/goerli/lighthouse/common/lru_cache/src/time.rs:68:28 stack backtrace: Apr 15 09:37:36.993 WARN Peer sent invalid block in single block lookup, peer_id: 16Uiu2HAm6ZuyJpVpR6y51X4Enbp8EhRBqGycQsDMPX7e5XfPYznG, error: WouldRevertFinalizedSlot { block_slot: Slot(5420212), finalized_slot: Slot(5420224) }, root: 0x10f6…3165, service: sync 0: rust_begin_unwind at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:575:5 1: core::panicking::panic_fmt at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:64:14 2: core::panicking::panic_display at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:135:5 3: core::panicking::panic_str at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:119:5 4: core::option::expect_failed at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/option.rs:1879:5 5: lru_cache::time::LRUTimeCache<Key>::raw_remove 6: lighthouse_network::peer_manager::PeerManager<TSpec>::handle_ban_operation 7: lighthouse_network::peer_manager::PeerManager<TSpec>::handle_score_action 8: lighthouse_network::peer_manager::PeerManager<TSpec>::report_peer 9: network::service::NetworkService<T>::spawn_service::{{closure}} 10: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll 11: <futures_util::future::future::map::Map<Fut,F> as core::future::future::Future>::poll 12: <futures_util::future::future::flatten::Flatten<Fut,<Fut as core::future::future::Future>::Output> as core::future::future::Future>::poll 13: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut 14: tokio::runtime::task::core::Core<T,S>::poll 15: tokio::runtime::task::harness::Harness<T,S>::poll 16: tokio::runtime::scheduler::multi_thread::worker::Context::run_task 17: tokio::runtime::scheduler::multi_thread::worker::Context::run 18: tokio::macros::scoped_tls::ScopedKey<T>::set 19: tokio::runtime::scheduler::multi_thread::worker::run 20: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut 21: tokio::runtime::task::core::Core<T,S>::poll 22: tokio::runtime::task::harness::Harness<T,S>::poll 23: tokio::runtime::blocking::pool::Inner::run note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. Apr 15 09:37:37.069 INFO Saved DHT state service: network Apr 15 09:37:37.070 INFO Network service shutdown service: network Apr 15 09:37:37.132 CRIT Task panic. This is a bug! advice: Please check above for a backtrace and notify the developers, message: <none>, task_name: network Apr 15 09:37:37.132 INFO Internal shutdown received reason: Panic (fatal error) Apr 15 09:37:37.133 INFO Shutting down.. reason: Failure("Panic (fatal error)") Apr 15 09:37:37.135 WARN Unable to free worker error: channel closed, msg: did not free worker, shutdown may be underway Apr 15 09:37:39.350 INFO Saved beacon chain to disk service: beacon Panic (fatal error) ```
## Issue Addressed There was a [`VecDeque` bug](rust-lang/rust#108453) in some recent versions of the Rust standard library (1.67.0 & 1.67.1) that could cause Lighthouse to panic (reported by `@Sea Monkey` on discord). See full logs below. The issue was likely introduced in Rust 1.67.0 and [fixed](rust-lang/rust#108475) in 1.68, and we were able to reproduce the panic ourselves using [@michaelsproul's fuzz tests](https://github.com/michaelsproul/lighthouse/blob/fuzz-lru-time-cache/beacon_node/lighthouse_network/src/peer_manager/fuzz.rs#L111) on both Rust 1.67.0 and 1.67.1. Users that uses our Docker images or binaries are unlikely affected, as our Docker images were built with `1.66`, and latest binaries were built with latest stable (`1.68.2`). It likely impacts user that builds from source using Rust versions 1.67.x. ## Proposed Changes Bump Rust version (MSRV) to latest stable `1.68.2`. ## Additional Info From `@Sea Monkey` on Lighthouse Discord: > Crash on goerli using `unstable` `dd124b2d6804d02e4e221f29387a56775acccd08` ``` thread 'tokio-runtime-worker' panicked at 'Key must exist', /mnt/goerli/goerli/lighthouse/common/lru_cache/src/time.rs:68:28 stack backtrace: Apr 15 09:37:36.993 WARN Peer sent invalid block in single block lookup, peer_id: 16Uiu2HAm6ZuyJpVpR6y51X4Enbp8EhRBqGycQsDMPX7e5XfPYznG, error: WouldRevertFinalizedSlot { block_slot: Slot(5420212), finalized_slot: Slot(5420224) }, root: 0x10f6…3165, service: sync 0: rust_begin_unwind at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:575:5 1: core::panicking::panic_fmt at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:64:14 2: core::panicking::panic_display at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:135:5 3: core::panicking::panic_str at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:119:5 4: core::option::expect_failed at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/option.rs:1879:5 5: lru_cache::time::LRUTimeCache<Key>::raw_remove 6: lighthouse_network::peer_manager::PeerManager<TSpec>::handle_ban_operation 7: lighthouse_network::peer_manager::PeerManager<TSpec>::handle_score_action 8: lighthouse_network::peer_manager::PeerManager<TSpec>::report_peer 9: network::service::NetworkService<T>::spawn_service::{{closure}} 10: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll 11: <futures_util::future::future::map::Map<Fut,F> as core::future::future::Future>::poll 12: <futures_util::future::future::flatten::Flatten<Fut,<Fut as core::future::future::Future>::Output> as core::future::future::Future>::poll 13: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut 14: tokio::runtime::task::core::Core<T,S>::poll 15: tokio::runtime::task::harness::Harness<T,S>::poll 16: tokio::runtime::scheduler::multi_thread::worker::Context::run_task 17: tokio::runtime::scheduler::multi_thread::worker::Context::run 18: tokio::macros::scoped_tls::ScopedKey<T>::set 19: tokio::runtime::scheduler::multi_thread::worker::run 20: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut 21: tokio::runtime::task::core::Core<T,S>::poll 22: tokio::runtime::task::harness::Harness<T,S>::poll 23: tokio::runtime::blocking::pool::Inner::run note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. Apr 15 09:37:37.069 INFO Saved DHT state service: network Apr 15 09:37:37.070 INFO Network service shutdown service: network Apr 15 09:37:37.132 CRIT Task panic. This is a bug! advice: Please check above for a backtrace and notify the developers, message: <none>, task_name: network Apr 15 09:37:37.132 INFO Internal shutdown received reason: Panic (fatal error) Apr 15 09:37:37.133 INFO Shutting down.. reason: Failure("Panic (fatal error)") Apr 15 09:37:37.135 WARN Unable to free worker error: channel closed, msg: did not free worker, shutdown may be underway Apr 15 09:37:39.350 INFO Saved beacon chain to disk service: beacon Panic (fatal error) ```
There was a [`VecDeque` bug](rust-lang/rust#108453) in some recent versions of the Rust standard library (1.67.0 & 1.67.1) that could cause Lighthouse to panic (reported by `@Sea Monkey` on discord). See full logs below. The issue was likely introduced in Rust 1.67.0 and [fixed](rust-lang/rust#108475) in 1.68, and we were able to reproduce the panic ourselves using [@michaelsproul's fuzz tests](https://github.com/michaelsproul/lighthouse/blob/fuzz-lru-time-cache/beacon_node/lighthouse_network/src/peer_manager/fuzz.rs#L111) on both Rust 1.67.0 and 1.67.1. Users that uses our Docker images or binaries are unlikely affected, as our Docker images were built with `1.66`, and latest binaries were built with latest stable (`1.68.2`). It likely impacts user that builds from source using Rust versions 1.67.x. Bump Rust version (MSRV) to latest stable `1.68.2`. From `@Sea Monkey` on Lighthouse Discord: > Crash on goerli using `unstable` `dd124b2d6804d02e4e221f29387a56775acccd08` ``` thread 'tokio-runtime-worker' panicked at 'Key must exist', /mnt/goerli/goerli/lighthouse/common/lru_cache/src/time.rs:68:28 stack backtrace: Apr 15 09:37:36.993 WARN Peer sent invalid block in single block lookup, peer_id: 16Uiu2HAm6ZuyJpVpR6y51X4Enbp8EhRBqGycQsDMPX7e5XfPYznG, error: WouldRevertFinalizedSlot { block_slot: Slot(5420212), finalized_slot: Slot(5420224) }, root: 0x10f6…3165, service: sync 0: rust_begin_unwind at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/std/src/panicking.rs:575:5 1: core::panicking::panic_fmt at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:64:14 2: core::panicking::panic_display at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:135:5 3: core::panicking::panic_str at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/panicking.rs:119:5 4: core::option::expect_failed at /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/option.rs:1879:5 5: lru_cache::time::LRUTimeCache<Key>::raw_remove 6: lighthouse_network::peer_manager::PeerManager<TSpec>::handle_ban_operation 7: lighthouse_network::peer_manager::PeerManager<TSpec>::handle_score_action 8: lighthouse_network::peer_manager::PeerManager<TSpec>::report_peer 9: network::service::NetworkService<T>::spawn_service::{{closure}} 10: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll 11: <futures_util::future::future::map::Map<Fut,F> as core::future::future::Future>::poll 12: <futures_util::future::future::flatten::Flatten<Fut,<Fut as core::future::future::Future>::Output> as core::future::future::Future>::poll 13: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut 14: tokio::runtime::task::core::Core<T,S>::poll 15: tokio::runtime::task::harness::Harness<T,S>::poll 16: tokio::runtime::scheduler::multi_thread::worker::Context::run_task 17: tokio::runtime::scheduler::multi_thread::worker::Context::run 18: tokio::macros::scoped_tls::ScopedKey<T>::set 19: tokio::runtime::scheduler::multi_thread::worker::run 20: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut 21: tokio::runtime::task::core::Core<T,S>::poll 22: tokio::runtime::task::harness::Harness<T,S>::poll 23: tokio::runtime::blocking::pool::Inner::run note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. Apr 15 09:37:37.069 INFO Saved DHT state service: network Apr 15 09:37:37.070 INFO Network service shutdown service: network Apr 15 09:37:37.132 CRIT Task panic. This is a bug! advice: Please check above for a backtrace and notify the developers, message: <none>, task_name: network Apr 15 09:37:37.132 INFO Internal shutdown received reason: Panic (fatal error) Apr 15 09:37:37.133 INFO Shutting down.. reason: Failure("Panic (fatal error)") Apr 15 09:37:37.135 WARN Unable to free worker error: channel closed, msg: did not free worker, shutdown may be underway Apr 15 09:37:39.350 INFO Saved beacon chain to disk service: beacon Panic (fatal error) ```
…acrum Fix `VecDeque::shrink_to` UB when `handle_alloc_error` unwinds. Fixes rust-lang#123369 For `VecDeque` it's relatively simple to restore the buffer into a consistent state so this PR does just that. Note that with its current implementation, `shrink_to` may change the internal arrangement of elements in the buffer, so e.g. `[D, <uninit>, A, B, C]` will become `[<uninit>, A, B, C, D]` and `[<uninit>, <uninit>, A, B, C]` may become `[B, C, <uninit>, <uninit>, A]` if `shrink_to` unwinds. This shouldn't be an issue though as we don't make any guarantees about the stability of the internal buffer arrangement (and this case is impossible to hit on stable anyways). This PR also includes a test with code adapted from rust-lang#123369 which fails without the new `shrink_to` code. Does this suffice or do we maybe need more exhaustive tests like in rust-lang#108475? cc `@Amanieu` `@rustbot` label +T-libs
…acrum Fix `VecDeque::shrink_to` UB when `handle_alloc_error` unwinds. Fixes rust-lang#123369 For `VecDeque` it's relatively simple to restore the buffer into a consistent state so this PR does just that. Note that with its current implementation, `shrink_to` may change the internal arrangement of elements in the buffer, so e.g. `[D, <uninit>, A, B, C]` will become `[<uninit>, A, B, C, D]` and `[<uninit>, <uninit>, A, B, C]` may become `[B, C, <uninit>, <uninit>, A]` if `shrink_to` unwinds. This shouldn't be an issue though as we don't make any guarantees about the stability of the internal buffer arrangement (and this case is impossible to hit on stable anyways). This PR also includes a test with code adapted from rust-lang#123369 which fails without the new `shrink_to` code. Does this suffice or do we maybe need more exhaustive tests like in rust-lang#108475? cc `@Amanieu` `@rustbot` label +T-libs
Rollup merge of rust-lang#123803 - Sp00ph:shrink_to_fix, r=Mark-Simulacrum Fix `VecDeque::shrink_to` UB when `handle_alloc_error` unwinds. Fixes rust-lang#123369 For `VecDeque` it's relatively simple to restore the buffer into a consistent state so this PR does just that. Note that with its current implementation, `shrink_to` may change the internal arrangement of elements in the buffer, so e.g. `[D, <uninit>, A, B, C]` will become `[<uninit>, A, B, C, D]` and `[<uninit>, <uninit>, A, B, C]` may become `[B, C, <uninit>, <uninit>, A]` if `shrink_to` unwinds. This shouldn't be an issue though as we don't make any guarantees about the stability of the internal buffer arrangement (and this case is impossible to hit on stable anyways). This PR also includes a test with code adapted from rust-lang#123369 which fails without the new `shrink_to` code. Does this suffice or do we maybe need more exhaustive tests like in rust-lang#108475? cc `@Amanieu` `@rustbot` label +T-libs
Fixes #108453.
Also adds both a specific test with the code from #108453 and an exhaustive test that checks all possible head positions, lengths and target capacities for deques with capacity 16.
cc @trinity-1686a @scottmcm