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

Safety of ptr::swap is unclear #81005

Closed
pandaman64 opened this issue Jan 14, 2021 · 5 comments · Fixed by #114794
Closed

Safety of ptr::swap is unclear #81005

pandaman64 opened this issue Jan 14, 2021 · 5 comments · Fixed by #114794
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@pandaman64
Copy link

pandaman64 commented Jan 14, 2021

The ptr::swap function allows passed pointers to overlap, and its safety section is as follows:

Behavior is undefined if any of the following conditions are violated:

  • Both x and y must be valid for both reads and writes.
  • Both x and y must be properly aligned.

Note that even if T has size 0, the pointers must be non-NULL and properly aligned.

It says that both pointers must be "valid," but IIUC, the definition of validity becomes unclear under Stacked Borrows.

Consider the following program (all code in this comment is checked using Miri with MIRIFLAGS="-Zmiri-track-raw-pointers"):

unsafe {
    let mut x: [i32; 3] = [100, 200, 300];
    let p: *mut i32 = x.as_mut_ptr();
    let p1: *mut [i32; 2] = p as *mut [i32; 2]; // pointer to the first two elements
    let p2: *mut [i32; 2] = &mut *(p.add(1) as *mut [i32; 2]); // pointer to the last two elements

    // ☆: each pointer is valid according to Stacked Borrows.
    *p2 = [100, 200];
    *p1 = [200, 300];
}

p1 and p2 are both "valid" at ☆ in the sense that this program is safe.
p2 is invalidated when we perform a write using p1, but it's ok because we don't use p2 anymore.

However, the following program exhibits UB in spite of the validity of p1 and p2, because ptr::swap first reads from p1 and the read invalidates p2.

// Example 1 (UB)
unsafe {
    let mut x: [i32; 3] = [100, 200, 300];
    let p: *mut i32 = x.as_mut_ptr();
    let p1: *mut [i32; 2] = p as *mut [i32; 2];
    let p2: *mut [i32; 2] = &mut *(p.add(1) as *mut [i32; 2]); // intermediate reference

    std::ptr::swap(p1, p2); // this is UB
}

Note that the intermediate unique reference (&mut *) is crucial for "separating" the two pointers.
If we remove the intermediate reference, those pointers have the same capabilities.
In other words, the following program does not exhibit UB:

// Example 2 (no UB)
unsafe {
    let mut x: [i32; 3] = [100, 200, 300];
    let p: *mut i32 = x.as_mut_ptr();
    let p1: *mut [i32; 2] = p as *mut [i32; 2];
    let p2: *mut [i32; 2] = p.add(1) as *mut [i32; 2]; // no intermediate reference

    std::ptr::swap(p1, p2); // this is not UB
}

The difference between the two examples is so subtle that I'm not sure how to spell out the exact safety conditions for ptr::swap.

This issue is found while formalizing the safety of ptr::swap in the Rustv project.

@pandaman64
Copy link
Author

By the way, the second example of ptr::swap contains unrelated UB.
The example calls .as_mut_ptr() twice, which accidentally invalidates the first pointer x.

use std::ptr;

let mut array = [0, 1, 2, 3];

let x = array[0..].as_mut_ptr() as *mut [u32; 3]; // this is `array[0..3]`

// this as_mut_ptr() invalidates x!
let y = array[1..].as_mut_ptr() as *mut [u32; 3]; // this is `array[1..4]`

unsafe {
    // using invalidated x
    ptr::swap(x, y);

    // (snip)
}

@pandaman64
Copy link
Author

possibly related: #80778 #80682

@camelid
Copy link
Member

camelid commented Jan 18, 2021

cc @RalfJung

@camelid camelid added T-libs Relevant to the library team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 18, 2021
@pandaman64
Copy link
Author

pandaman64 commented Jan 18, 2021

The same argument seems to apply to ptr::copy.

Miri (tracking raw pointers) reported undefined behavior for the following code:

unsafe {
    let mut x: [i32; 3] = [100, 200, 300];
    let p1: *mut i32 = x.as_mut_ptr();
    let p2: *mut i32 = &mut *(p1.add(1)); // intermediate reference

    std::ptr::copy(p1, p2, 2); // this is UB
}

When I set the count argument of the ptr::copy to 1, Miri did not report any undefined behavior. This may be an implementation detail.

unsafe {
    let mut x: [i32; 3] = [100, 200, 300];
    let p1: *mut i32 = x.as_mut_ptr();
    let p2: *mut i32 = &mut *p1; // intermediate reference

    std::ptr::copy(p1, p2, 1); // miri treated this line as safe
}

Note that the pointers are valid individually but not simultaneously in these examples.
I found this distinction very interesting, but I don't know how to explain this without being too scholarly 😂

@RalfJung
Copy link
Member

p1 and p2 are both "valid" at ☆ in the sense that this program is safe.

Well, they are valid individually, but they are not "both valid at the same time". In separation logic terms, we have valid(p1) ∧ valid(p2) but we do not have valid(p1) * valid(p2). The issue is to find a way to express that without using separation logic terminology.^^

possibly related: #80778 #80682

#80778 is entirely unrelated, as it concerns the safety invariant of Cell and the soundness of its swap method in ways other than aliasing.
#80682 is an example of code that uses ptr::swap the wrong way, at least wrong under Stacked Borrows. So it is the dual issue -- it is a consumer if the spec which this issue points could needs to be made more precise.

@the8472 the8472 added the A-raw-pointers Area: raw pointers, MaybeUninit, NonNull label Feb 8, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Sep 5, 2023
clarify safety documentation of ptr::swap and ptr::copy

Closes rust-lang#81005
@bors bors closed this as completed in 14c57f1 Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants