Skip to content
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

fix for undefined behavior in <[T]>::copy_within method #85675

Closed
wants to merge 1 commit into from

Conversation

Pointerbender
Copy link
Contributor

<[T]>::copy_within() currently contains undefined behavior according to the Stacked Borrows model. This PR brings a fix to restore its soundness.

To help understand where the UB comes from, consider the following code snippet from the original implementation of copy_within before the fix:

        unsafe {
            ptr::copy(self.as_ptr().add(src_start), self.as_mut_ptr().add(dest), count);
        }

The arguments to ptr::copy are evaluated from left to right. self.as_ptr() creates an immutable reference (which is tagged as SharedReadOnly by Stacked Borrows) to the array and derives a valid *const pointer from it. When jumping to the next argument, self.as_mut_ptr() creates a mutable reference (tagged as Unique) to the array, which invalidates the existing SharedReadOnly reference and any pointers derived from it. The invalidated *const pointer (the first argument to ptr::copy) is then used after the fact when ptr::copy is called, which triggers the undefined behavior.

The fix is to obtain only one mutable reference (tagged Unique) at the start, then cast it to a mutable pointer (tagged SharedReadWrite), for which both of the pointer arguments to ptr::copy can be safely derived without invalidating either pointer, like so:

        let ptr = self.as_mut_ptr();
        // SAFETY: the conditions for `ptr::copy` have all been checked above,
        // as have those for `ptr::add`.
        unsafe {
            ptr::copy(ptr.add(src_start), ptr.add(dest), count);
        }

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2021
@SkiFire13
Copy link
Contributor

Sorry but you're a bit late, I opened #85610 2 days ago

@Pointerbender
Copy link
Contributor Author

Oh! I had totally missed that. Apologies 😄 Thanks for the fix!

@Mark-Simulacrum
Copy link
Member

Closing in favor of #85610, but thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants