Skip to content

Commit

Permalink
Prevent invalid values from existing in Vec::swap_remove
Browse files Browse the repository at this point in the history
  • Loading branch information
@SkiFire13
SkiFire13 committed Oct 20, 2021
1 parent 6162529 commit 0aa68a8
Showing 1 changed file with 4 additions and 3 deletions.
7 changes: 4 additions & 3 deletions library/alloc/src/vec/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1305,10 +1305,11 @@ impl<T, A: Allocator> Vec<T, A> {
// We replace self[index] with the last element. Note that if the
// bounds check above succeeds there must be a last element (which
// can be self[index] itself).
let last = ptr::read(self.as_ptr().add(len - 1));
let hole = self.as_mut_ptr().add(index);
let value = ptr::read(self.as_ptr().add(index));
let base_ptr = self.as_mut_ptr();
ptr::copy(base_ptr.add(len - 1), base_ptr.add(index), 1);
self.set_len(len - 1);
ptr::replace(hole, last)
value
}
}

Expand Down

6 comments on commit 0aa68a8

@qm3ster
Copy link
Contributor

@qm3ster qm3ster commented on 0aa68a8 Dec 6, 2021

Choose a reason for hiding this comment

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

Is there a performance reason for putting the set_len after the copy?

@SkiFire13
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this makes any difference

@qm3ster
Copy link
Contributor

@qm3ster qm3ster commented on 0aa68a8 Dec 6, 2021

Choose a reason for hiding this comment

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

Wouldn't

self.set_len(len - 1); // make out-of-bounds
let value = ptr::read(self.as_ptr().add(index)) // get item
let base_ptr = self.as_mut_ptr();
ptr::copy(base_ptr.add(len - 1), base_ptr.add(index), 1); // replace in-bounds
value

make the most sense semantically then?

or with named pointers

self.set_len(len - 1);
let last = self.as_mut_ptr().add(len - 1);
let hole = self.as_mut_ptr().add(index);
let value = ptr::read(hole);
ptr::copy(last, hole, 1);
value

These avoid the in-bounds slice containing two copies of a value at any point (a condition that was introduced by this commit)

However, if there is no assumption that ptr::copy is (or is intended to) optimize better than a ptr::read+ptr::write on some (potential) platform targets, this can just be reverted.

@SkiFire13
Copy link
Contributor Author

@SkiFire13 SkiFire13 commented on 0aa68a8 Dec 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't [...] make the most sense semantically then?

Yes, but to be fair the code had this semantics before my PR, I just kept it. Feel free to open a PR that switches the order if this bothers you.

However, if there is no assumption that ptr::copy is (or is intended to) optimize better than a ptr::read+ptr::write on some (potential) platform targets,

That's not the point. The reason I used ptr::copy is that it doesn't require the T to be valid, while ptr::read+ptr::write does. This is a problem when last == hole because in that case the read from last will either be invalid or will invalidate value. This was the cause of the original bug that my commit aimed to fix.

@qm3ster
Copy link
Contributor

@qm3ster qm3ster commented on 0aa68a8 Dec 7, 2021

Choose a reason for hiding this comment

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

Thank you, I found the issue/PR now.

@SkiFire13
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to let you know, I just saw I made a typo in the previous comment, I meant to say "The reason I used ptr::copy" (now it's edited)

Please sign in to comment.