Audit suffix_array

[nicoonoclaste]

Dependency of qbsdiff by the same author.

Noticed 2 patterns:

• std::ptr::copy_nonoverlapping(&src_slice[i] as *const T, &mut dst_slice[j] as *mut T, n)
  All non-UB uses can be replaced with dst.slice[j..j+n].copy_from_slice(src_slice[i..i+n]).
• std::ptr::copy(&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)

Should we request a clippy lint?

Fixes sent upstream: hucsmn/suffix_array#1

[nicoonoclaste]

@hucsmn merged those changes; there are still unsafe functions in src/sa.rs (unchecked_{from_parts,load,load_{bytes,file}}) but they do not seem to contain potential memory unsafety themselves.

[Shnatsel]

Nice! This one is going to be easy to lint, too: when ptr::copy_nonoverlapping() is used on two slices of Copy types, it can be safely replaced with slice::copy_from_slice(). Same for ptr::copy and slice::copy_within().

[nicoonoclaste]

This one is going to be easy to lint, too: when ptr::copy_nonoverlapping() is used on two slices of Copy types, it can be safely replaced with slice::copy_from_slice(). Same for ptr::copy and slice::copy_within().

Yes, and there is slice::clone_from_slice when the type isn't Copy.

[Shnatsel]

slice::clone_from_slice() has different semantics than ptr::copy_nonoverlapping(): the former calls clone() on the objects, while the latter does not.

[nicoonoclaste]

@Shnatsel Sure, my point was that in the case of non-copy types, the use of ptr::copy_nonoverlapping (or ptr::copy) is UB; in those cases, the lint shouldn't suggest a replacement which doesn't work, but instead notify the user that there is UB afoot and suggest a sound (but slower) replacement.

Update: moving that discussion to the Clippy issue, your example there was fairly enlightening.

[slightlyoutofphase]

FYI, the use of ptr::copy_nonoverlapping and ptr::copy on non-Copy types is not UB in any way.

It's just... particularly unsafe, as it requires you to ensure that you're properly avoiding any double-drops that might result from it (depending on the particular way it's being used) as well as ensure that you're not actually directly exposing bitwise copies of non-Copy types to safe code at any time.

See the append_elements function that Vec::append calls internally for a perfectly valid use on not-necessarily-Copy types, for example.

[Shnatsel]

The upstream code is now 100% safe, Clippy lint is requested. Closing this issue.

I've also opened a PR to showcase these improvements in safety-dance trophy case: #62