Skip to content

Commit

Permalink
Auto merge of #277 - saethlin:fix-aliasing, r=mbrubeck
Browse files Browse the repository at this point in the history
Fix all problems encounted with Miri -Ztag-raw-pointers

I poked at this crate with `-Zmiri-tag-raw-pointers` before, and I was unable to fix what I found (I just added a test case that ruled out one of my wrong ideas #271). I tried again just now and I guess I just understand better this time.

This PR fixes 3 separate pointer invalidation problems, which are detected by running `MIRIFLAGS=-Zmiri-tag-raw-pointers cargo miri test`.

Depending on how you squint, 2 or 3 of these are rust-lang/unsafe-code-guidelines#133. The last one is _probably_ still present even with late invalidation, because `set_len` does a write through a `&mut`.

It's unclear to me if any of these things that Miri complains about are potentially a miscompilation in rustc due to the use of LLVM `noalias`. But perhaps given how subtle this codebase is overall, it would be best to run the tools on their pickiest settings, even if there are a few things more like a false positive than a real problem.
  • Loading branch information
bors-servo committed Feb 1, 2022
2 parents 0a4fdff + b257aad commit deb82ed
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 4 deletions.
2 changes: 2 additions & 0 deletions .github/workflows/main.yml
Expand Up @@ -61,6 +61,8 @@ jobs:
- name: miri
if: matrix.toolchain == 'nightly'
run: bash ./scripts/run_miri.sh
env:
MIRIFLAGS: '-Zmiri-tag-raw-pointers'

- name: fuzz
if: env.DO_FUZZ == '1'
Expand Down
16 changes: 12 additions & 4 deletions src/lib.rs
Expand Up @@ -392,8 +392,11 @@ impl<'a, T: 'a + Array> Drop for Drain<'a, T> {
let start = source_vec.len();
let tail = self.tail_start;
if tail != start {
let src = source_vec.as_ptr().add(tail);
let dst = source_vec.as_mut_ptr().add(start);
// as_mut_ptr creates a &mut, invalidating other pointers.
// This pattern avoids calling it with a pointer already present.
let ptr = source_vec.as_mut_ptr();
let src = ptr.add(tail);
let dst = ptr.add(start);
ptr::copy(src, dst, self.tail_len);
}
source_vec.set_len(start + self.tail_len);
Expand Down Expand Up @@ -813,13 +816,14 @@ impl<A: Array> SmallVec<A> {
unsafe {
self.set_len(start);

let range_slice = slice::from_raw_parts_mut(self.as_mut_ptr().add(start), end - start);
let range_slice = slice::from_raw_parts(self.as_ptr().add(start), end - start);

Drain {
tail_start: end,
tail_len: len - end,
iter: range_slice.iter(),
vec: NonNull::from(self),
// Since self is a &mut, passing it to a function would invalidate the slice iterator.
vec: NonNull::new_unchecked(self as *mut _),
}
}
}
Expand Down Expand Up @@ -1112,6 +1116,10 @@ impl<A: Array> SmallVec<A> {
len: old_len + lower_size_bound,
};

// The set_len above invalidates the previous pointers, so we must re-create them.
let start = self.as_mut_ptr();
let ptr = start.add(index);

while num_added < lower_size_bound {
let element = match iter.next() {
Some(x) => x,
Expand Down

0 comments on commit deb82ed

Please sign in to comment.