Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix soundness issue in make_contiguous #79814

Merged
merged 1 commit into from
Dec 10, 2020
Merged

fix soundness issue in make_contiguous #79814

merged 1 commit into from
Dec 10, 2020

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Dec 7, 2020

fixes #79808

@lcnr lcnr added A-collections Area: `std::collection` beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Dec 7, 2020
@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 7, 2020
lcnr
lcnr commented Dec 7, 2020
View reviewed changes
library/alloc/src/collections/vec_deque/mod.rs
@@ -2839,7 +2839,7 @@ impl<T> From<VecDeque<T>> for Vec<T> {
let len = other.len();
let cap = other.cap();

if other.head != 0 {
if other.tail != 0 {
Copy link
Contributor Author

@lcnr lcnr Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

this was only a perfomance issue because is_contiguous returns false if the head is 0

@lcnr lcnr marked this pull request as draft December 7, 2020 23:48
@lcnr lcnr marked this pull request as ready for review December 7, 2020 23:53
@LeSeulArtichaut LeSeulArtichaut added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 8, 2020
@camelid camelid added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 8, 2020
camelid
camelid reviewed Dec 8, 2020
View reviewed changes
library/alloc/src/collections/vec_deque/tests.rs
Comment on lines +213 to +225
#[test]
fn make_contiguous_head_to_end() {
let mut dq = VecDeque::with_capacity(3);
dq.push_front('B');
dq.push_front('A');
dq.push_back('C');
dq.make_contiguous();
let expected_tail = 0;
let expected_head = 3;
assert_eq!(expected_tail, dq.tail);
assert_eq!(expected_head, dq.head);
assert_eq!((&['A', 'B', 'C'] as &[_], &[] as &[_]), dq.as_slices());
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to write a version of this that uses Boxes so that we ensure it doesn't segfault?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that shouldn't matter i think. The reason this could segfault is if the result is wrong. But as the used algorithm doesn't depend on T asserting that it is correct for i32 also means that it would be correct for Box and therefore can't segfault.

Copy link
Member

Choose a reason for hiding this comment

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

Probably, but I figured better safe than sorry :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, don't have the capacity for this rn though.

@spastorino
Copy link
Member

discussed in T-compiler meeting.

@rustbot modify labels: beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Dec 10, 2020
@Mark-Simulacrum
Copy link
Member

This seems ok, I'd like to get it merged ASAP so we can get testing to the extent possible. @bors r+ p=1

I admit the code is pretty high-context I think in terms of being able to follow it.

@bors
Copy link
Contributor

bors commented Dec 10, 2020

📌 Commit 4fb9f1d has been approved by Mark-Simulacrum

@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 Dec 10, 2020
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 10, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Dec 10, 2020
Merged
@spastorino
Copy link
Member

discussed in T-compiler meeting.

@rustbot modify labels: stable-accepted

@rustbot rustbot added the stable-accepted Accepted for backporting to the compiler in the stable channel. label Dec 10, 2020
@bors
Copy link
Contributor

bors commented Dec 10, 2020

⌛ Testing commit 4fb9f1d with merge d32c320...

@camelid camelid removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Dec 10, 2020
@camelid
Copy link
Member

camelid commented Dec 10, 2020

(I removed stable-nominated because it's been accepted.)

@bors
Copy link
Contributor

bors commented Dec 10, 2020

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing d32c320 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 10, 2020
@bors bors merged commit d32c320 into rust-lang:master Dec 10, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 10, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 10, 2020
@bors
Auto merge of rust-lang#79903 - Mark-Simulacrum:beta-next, r=Mark-Sim…
877c7cb 
…ulacrum

[beta] backports

* Revert rust-lang#77534 fixing rust-lang#77713 on beta, principled fix landed on master
* fix soundness issue in `make_contiguous` rust-lang#79814
* Fix exhaustiveness in case a byte string literal is used at slice type rust-lang#79072
@Mark-Simulacrum
Copy link
Member

The process for stable and beta nominations is the same - until they happen, both nominated and stable should be left. Reading.

@Mark-Simulacrum Mark-Simulacrum added the stable-nominated Nominated for backporting to the compiler in the stable channel. label Dec 10, 2020
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.50.0, 1.49.0 Dec 10, 2020
@camelid
Copy link
Member

camelid commented Dec 10, 2020

Oops, didn't realize that! Sorry.

@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Dec 23, 2020
Closed
@Mark-Simulacrum Mark-Simulacrum removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Feb 12, 2021
@lcnr lcnr deleted the deque-f branch May 25, 2022 06:02
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@camelid camelid camelid left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-collections Area: `std::collection` beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. stable-accepted Accepted for backporting to the compiler in the stable channel. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.49.0
9 participants
@lcnr @rust-highfive @spastorino @Mark-Simulacrum @bors @camelid @cramertj @LeSeulArtichaut @rustbot