Skip to content

fix soundness issue in make_contiguous #79814

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

Merged
merged 1 commit into from
Dec 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions library/alloc/src/collections/vec_deque/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1469,6 +1469,8 @@ impl<T> VecDeque<T> {

#[inline]
fn is_contiguous(&self) -> bool {
// FIXME: Should we consider `head == 0` to mean
// that `self` is contiguous?
self.tail <= self.head
}

Expand Down Expand Up @@ -2198,7 +2200,7 @@ impl<T> VecDeque<T> {
if self.is_contiguous() {
let tail = self.tail;
let head = self.head;
return unsafe { &mut self.buffer_as_mut_slice()[tail..head] };
return unsafe { RingSlices::ring_slices(self.buffer_as_mut_slice(), head, tail).0 };
}

let buf = self.buf.ptr();
Expand All @@ -2224,7 +2226,13 @@ impl<T> VecDeque<T> {
self.tail = 0;
self.head = len;
}
} else if free >= self.head {
} else if free > self.head {
// FIXME: We currently do not consider ....ABCDEFGH
// to be contiguous because `head` would be `0` in this
// case. While we probably want to change this it
// isn't trivial as a few places expect `is_contiguous`
// to mean that we can just slice using `buf[tail..head]`.

// there is enough free space to copy the head in one go,
// this means that we first shift the tail forwards, and then
// copy the head to the correct position.
Expand All @@ -2238,7 +2246,7 @@ impl<T> VecDeque<T> {
// ...ABCDEFGH.

self.tail = self.head;
self.head = self.tail + len;
self.head = self.wrap_add(self.tail, len);
}
} else {
// free is smaller than both head and tail,
Expand Down Expand Up @@ -2278,7 +2286,7 @@ impl<T> VecDeque<T> {

let tail = self.tail;
let head = self.head;
unsafe { &mut self.buffer_as_mut_slice()[tail..head] }
unsafe { RingSlices::ring_slices(self.buffer_as_mut_slice(), head, tail).0 }
}

/// Rotates the double-ended queue `mid` places to the left.
Expand Down Expand Up @@ -2839,7 +2847,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

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

ptr::copy(buf.add(other.tail), buf, len);
}
Vec::from_raw_parts(buf, len, cap)
Expand Down
14 changes: 14 additions & 0 deletions library/alloc/src/collections/vec_deque/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,20 @@ fn make_contiguous_small_free() {
);
}

#[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());
}
Comment on lines +213 to +225
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.


#[test]
fn test_remove() {
// This test checks that every single combination of tail position, length, and
Expand Down