New lint: Unnecessary use of std::ptr::{copy,copy_nonoverlapping} with slices

[nicoonoclaste]

During a Secure Code WG audit, I ran into the following unsafe code patterns, which can always be replaced with safe code with essentially no drawback or overhead:

• std::ptr::copy{,_nonoverlapping}(&src_slice[i] as *const T, &mut dst_slice[j] as *mut T, n)
  All non-UB uses on Copy types can be replaced with dst.slice[j..j+n].copy_from_slice(src_slice[i..i+n]).
  Moreover, if T isn't Copy, this is UB and should be replaced with dst.slice[j..j+n].clone_from_slice(src_slice[i..i+n])

• std::ptr::copy{,_nonoverlapping}(&s[i] as *const T, &mut s[j] as *mut T, n)
  Same idea, non-UB uses can be replaced with s.copy_within(i..i+n, j).
  If the ranges are non-overlapping, it might be faster to use slice::split_at_mut and copy_from_slice (resulting in a call to std::ptr::copy_nonoverlapping), but that might be too much static analysis to ask from Clippy (though it can be safely assumed in the copy_nonoverlapping case)

[nicoonoclaste]

I believe this lint shouldn't default to allow, given that there seem to be no legitimate reason to prefer the std::ptr API in those cases: it is unsafe, doesn't provide bound-checks, and the safe slice API calls into the std::ptr equivalents right after the bounds checks anyhow.

[Shnatsel]

This is clearly a good idea for Copy types. However, I'm not sure we should extend this lint to non-Copy types. For example, the following unholy abomination actually passes MIRI:

let mut src: Vec<&str> = vec!["foo", "bar"];
let mut dst: Vec<&str> = Vec::with_capacity(2);
unsafe {
    std::ptr::copy_nonoverlapping(&src[0] as *const &str, dst.get_unchecked_mut(0) as *mut &str, 2);
    src.set_len(0);
};

This would have been a double free risk if there was some code that could panic between copy_nonoverlapping and set_len calls, but as-is it's fine.

Or maybe it's a bug in MIRI and the aliasing that is briefly created before the set_len() call is actually UB under stacked borrows. @RalfJung any idea?

[Shnatsel]

Also, I do not see a better way than ptr::copy_nonoverlapping() to move references to heap-allocated objects from one slice to another. So this is probably legitimate code for non-Copy types after all.

[nicoonoclaste]

@Shnatsel Great point; I updated the issue description to restrict it to Copy types, and we should probably move the case of non-Copy types to a separate issue (if a lint is appropriate for them)

[RalfJung]

std::ptr::copy{,_nonoverlapping}(&src_slice[i] as *const T, &mut dst_slice[j] as *mut T, n)

Note that this code is also aliasing-incorrect: &src_slice[i] as *const T is a pointer that must only be used for that one slice element, it cannot sway into neighboring elements. If you want a raw pointer to a slice, always use .as_(mut_)ptr(). In this case I'd use either src_slice[i..i+n].as_ptr() or src_slice.as_ptr().offset(i).

Or maybe it's a bug in MIRI and the aliasing that is briefly created before the set_len() call is actually UB under stacked borrows. @RalfJung any idea?

Aliasing only matters for pointers that you are actually "touching", at least under current Stacked Borrows. This does not happen recursively. So at least currently it is by design that Miri does not error here. (And anyway with &str aliasing wouldn't even be a problem, you'd have to make it Box to make the test case interesting. You say there is a double-free risk in your code but I don't see it with &str.)

What Miri should complain about is that you did not use src.as_ptr(), but instead uses (unchecked) indexing. So far we do not track raw pointers precisely enough to detect that, but this is a planned extension.