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

Fix unsoundness for VecDeque #53571

Merged
merged 3 commits into from
Aug 23, 2018
Merged

Fix unsoundness for VecDeque #53571

merged 3 commits into from
Aug 23, 2018

Conversation

MaloJaffre
Copy link
Contributor

@MaloJaffre MaloJaffre commented Aug 21, 2018 •
edited by RalfJung
Loading

See individual commit for more details.

r? @RalfJung.

Fixes #53566, fixes #53529

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2018
@MaloJaffre MaloJaffre changed the title Vecdeque emergency Fix unsoudness for VecDeque Aug 21, 2018
@RalfJung
Copy link
Member

I fully agree with the Debug part.

For the revert, maybe we want to keep the tests and benches?

@RalfJung RalfJung mentioned this pull request Aug 21, 2018
Merged
@varkor
Copy link
Member

varkor commented Aug 21, 2018

Shouldn't there be a test to make sure the unsoundness couldn't be accidentally reintroduced in the future?

@RalfJung
Copy link
Member

We can't really test for UB...

... except miri can. I guess someone could add a test to the miri test suite. Ideally doing more than just a debug print; essentially running much of the rustc test suite covering VecDeque in miri. (At some point we want to run the rustc test suite proper on miri but that'll take a while.)

@MaloJaffre MaloJaffre changed the title Fix unsoudness for VecDeque Fix unsoundness for VecDeque Aug 22, 2018
@MaloJaffre
Copy link
Contributor Author

@RalfJung I've now restored the tests and the benches.
Apart from a test, I think this is ready.

@RalfJung
Copy link
Member

Apart from a test, I think this is ready.

What kind of test do you plan to add?

@MaloJaffre
Copy link
Contributor Author

I've just added a compile-pass test for #53529 by @Pazzaz.

@MaloJaffre
Revert "Auto merge of rust-lang#52553 - Pazzaz:vecdeque-append, r=Sim…
241b9b4 
…onSapin"

This partially reverts commit d5b6b95,
reversing changes made to 6b1ff19.

Fixes rust-lang#53529.
Cc: rust-lang#53564.
@RalfJung
Copy link
Member

Hm, I think this would make more sense as a unit test in liballoc/tests/vec_deque.rs. Could you make it one?

@MaloJaffre
Copy link
Contributor Author

@RalfJung Done!

@RalfJung
Copy link
Member

Awesome! r=me once traivs is happy.

@bors delegate+

@bors
Copy link
Contributor

bors commented Aug 22, 2018

✌️ @MaloJaffre can now approve this pull request

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
[00:48:03] ....................................................................................................
[00:48:06] ....................................................................................................
[00:48:09] ..........i.........................................................................................
[00:48:12] ....................................................................................................
[00:48:14] ...........................................................iiiiiiiii................................
[00:48:20] ....................................................................................................
[00:48:24] ....................................................................................................
[00:48:26] ........................................i...........................................................
[00:48:29] ..........................................................................................i.i..ii...
---
[01:01:28]    Compiling alloc v0.0.0 (file:///checkout/src/liballoc)
[01:01:29] error[E0433]: failed to resolve. Use of undeclared type or module `Box`
[01:01:29]     --> liballoc/collections/vec_deque.rs:2973:24
[01:01:29]      |
[01:01:29] 2973 |         dst.push_front(Box::new(1));
[01:01:29]      |                        ^^^ Use of undeclared type or module `Box`
[01:01:29] error[E0433]: failed to resolve. Use of undeclared type or module `Box`
[01:01:29]     --> liballoc/collections/vec_deque.rs:2974:24
[01:01:29]      |
[01:01:29]      |
[01:01:29] 2974 |         dst.push_front(Box::new(2));
[01:01:29]      |                        ^^^ Use of undeclared type or module `Box`
[01:01:29] error[E0433]: failed to resolve. Use of undeclared type or module `Box`
[01:01:29]     --> liballoc/collections/vec_deque.rs:2978:24
[01:01:29]      |
[01:01:29]      |
[01:01:29] 2978 |         src.push_front(Box::new(0));
[01:01:29]      |                        ^^^ Use of undeclared type or module `Box`
[01:01:32] error: aborting due to 3 previous errors
[01:01:32] 
[01:01:32] For more information about this error, try `rustc --explain E0433`.
[01:01:32] error: Could not compile `alloc`.
[01:01:32] error: Could not compile `alloc`.
[01:01:32] warning: build failed, waiting for other jobs to finish...
[01:02:06] error: build failed
[01:02:06] 
[01:02:06] 
[01:02:06] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "alloc" "--" "--quiet"
[01:02:06] 
[01:02:06] 
4776388 .
2940264 ./obj

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@RalfJung
Copy link
Member

Oh while you are fixing this anyway, maybe you could add some assertions checking that we are getting back the values we expect (in pop_back and in the iteration loop)?

@MaloJaffre
Copy link
Contributor Author

Sure!

@MaloJaffre
Copy link
Contributor Author

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Aug 22, 2018

📌 Commit f8d5ed4 has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 22, 2018
@bors
Copy link
Contributor

bors commented Aug 23, 2018

⌛ Testing commit f8d5ed4 with merge 54d82d0...

bors added a commit that referenced this pull request Aug 23, 2018
@bors
Auto merge of #53571 - MaloJaffre:vecdeque-emergency, r=RalfJung
54d82d0 
Fix unsoundness for VecDeque

 See individual commit for more details.

r? @RalfJung.

Fixes #53566, fixes #53529
@bors
Copy link
Contributor

bors commented Aug 23, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: RalfJung
Pushing 54d82d0 to master...

@bors bors merged commit f8d5ed4 into rust-lang:master Aug 23, 2018
@MaloJaffre MaloJaffre deleted the vecdeque-emergency branch August 23, 2018 20:22
bors added a commit that referenced this pull request Aug 29, 2018
@bors
Auto merge of #53564 - MaloJaffre:vecdeque, r=gnzlbg
02cb8f2 
Reoptimize VecDeque::append

~Unfortunately, I don't know if these changes fix the unsoundness mentioned in #53529, so it is stil a WIP.
This is also completely untested.
The VecDeque code contains other unsound code: one example : [reading unitialized memory](https://play.rust-lang.org/?gist=6ff47551769af61fd8adc45c44010887&version=nightly&mode=release&edition=2015) (detected by MIRI), so I think this code will need a bigger refactor to make it clearer and safer.~

Note: this is based on #53571.
r? @SimonSapin
Cc: #53529 #52553 @yorickpeterse @jonas-schievink @Pazzaz @shepmaster.
@Shnatsel
Copy link
Member

Regarding a regression test for this: Memory Sanitizer would also have discovered this issue, and it introduces tolerable (~3x) slowdown, which is much less extreme than MIRI.

@tabokie tabokie mentioned this pull request Sep 7, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2021
@bors
Auto merge of rust-lang#88717 - tabokie:vecdeque-fast-append, r=m-ou-se
af9b508 
Optimize VecDeque::append

Optimize `VecDeque::append` to do unsafe copy rather than iterating through each element.

On my `Intel(R) Xeon(R) CPU E5-2630 v4 @ 2.20GHz`, the benchmark shows 37% improvements:
```
Master:
custom-bench vec_deque_append 583164 ns/iter
custom-bench vec_deque_append 550040 ns/iter

Patched:
custom-bench vec_deque_append 349204 ns/iter
custom-bench vec_deque_append 368164 ns/iter
```

Additional notes on the context: this is the third attempt to implement a non-trivial version of `VecDeque::append`, the last two are reverted due to unsoundness or regression, see:
- rust-lang#52553, reverted in rust-lang#53571
- rust-lang#53564, reverted in rust-lang#54851

Both cases are covered by existing tests.

Signed-off-by: tabokie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@RalfJung RalfJung

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
6 participants
@MaloJaffre @RalfJung @varkor @bors @rust-highfive @Shnatsel