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

Arc/Rc is creating references to uninitialized values #119241

Closed
taiki-e opened this issue Dec 23, 2023 · 1 comment · Fixed by #119433
Closed

Arc/Rc is creating references to uninitialized values #119241

taiki-e opened this issue Dec 23, 2023 · 1 comment · Fixed by #119433
Assignees
Labels
A-atomic Area: Atomics, barriers, and sync primitives C-cleanup Category: PRs that clean code up or issues documenting cleanup. C-discussion Category: Discussion or questions that doesn't represent real issues. T-opsem Relevant to the opsem team

Comments

@taiki-e
Copy link
Member

taiki-e commented Dec 23, 2023

addr_of_mut doc says:

Creating a reference with &/&mut is only allowed if the pointer is properly aligned and points to initialized data. For cases where those requirements do not hold, raw pointers should be used instead. However, &mut expr as *mut _ creates a reference before casting it to a raw pointer, and that reference is subject to the same rules as all other references. This macro can create a raw pointer without creating a reference first.

let mut uninit = MaybeUninit::<Demo>::uninit();
// `&uninit.as_mut().field` would create a reference to an uninitialized `bool`,
// and thus be Undefined Behavior!
let f1_ptr = unsafe { ptr::addr_of_mut!((*uninit.as_mut_ptr()).field) };

However, some code in Arc/Rc seems to create references to uninitialized values.

For example, in the following code, inner is a pointer to an uninitialized ArcInner, but Layout::for_value(&*inner) creates a reference to it, and &mut (*inner).strong and &mut (*inner).weak creates references to (uninitialized) counters.

rust/library/alloc/src/sync.rs

Lines 1825 to 1836 in 495203b

unsafe fn initialize_arcinner(
ptr: NonNull<[u8]>,
layout: Layout,
mem_to_arcinner: impl FnOnce(*mut u8) -> *mut ArcInner<T>,
) -> *mut ArcInner<T> {
let inner = mem_to_arcinner(ptr.as_non_null_ptr().as_ptr());
debug_assert_eq!(unsafe { Layout::for_value(&*inner) }, layout);
unsafe {
ptr::write(&mut (*inner).strong, atomic::AtomicUsize::new(1));
ptr::write(&mut (*inner).weak, atomic::AtomicUsize::new(1));
}

Such codes can also be found in some other places in Arc/Rc.

ptr::copy_nonoverlapping(vec_ptr, &mut (*rc_ptr).data as *mut [T] as *mut T, len);

&mut (*ptr).data as *mut _ as *mut u8,

ptr::copy_nonoverlapping(v.as_ptr(), &mut (*ptr).data as *mut [T] as *mut T, v.len());

let elems = &mut (*ptr).data as *mut [T] as *mut T;

rust/library/alloc/src/rc.rs

Lines 1888 to 1890 in 495203b

debug_assert_eq!(Layout::for_value(&*inner), layout);
ptr::write(&mut (*inner).strong, Cell::new(1));

&mut (*ptr).value as *mut _ as *mut u8,

ptr::copy_nonoverlapping(v.as_ptr(), &mut (*ptr).value as *mut [T] as *mut T, v.len());

let elems = &mut (*ptr).value as *mut [T] as *mut T;

ptr::copy_nonoverlapping(vec_ptr, &mut (*rc_ptr).value as *mut [T] as *mut T, len);

At least the following code should run into this issue because it calls Arc::initialize_arcinner via <Arc<_> as From<Box<_>>::from -> Arc::from_box_in -> Arc::allocate_for_ptr_in -> Arc::allocate_for_layout.

use std::{boxed::Box, sync::Arc};
let boxed: Box<str> = Box::from("abc");
let arc: Arc<str> = Arc::from(boxed);
assert_eq!("arc", &arc[..]);

Solution

This can be fixed in the following ways:

  • Use Layout::for_value_raw (unstable) instead of Layout::for_value.
  • Use addr_of_mut for code creating reference to uninitialized conters (.strong/.weak).
  • Use addr_of_mut or new_uninit/new_unint_slice for code creating reference to uninitialized data (.data/.value). (I think the latter would be clearer.)

cc @RalfJung: Miri doesn't seem to report this as UB, but is my understanding that this is the kind of UB that Miri does not currently support detection correct? Or is it missed or allowed for some reason?

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 23, 2023
@rustbot rustbot added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness I-prioritize Issue: Indicates that prioritization has been requested for this issue. A-atomic Area: Atomics, barriers, and sync primitives labels Dec 23, 2023
@RalfJung
Copy link
Member

RalfJung commented Dec 23, 2023

Cc @rust-lang/opsem

The status of reference to uninit memory is undecided. We document them as UB in the reference so that we can make this decision without code already relying on an outcome. Miri does not flag this UB because we are not sure if we really want to rule out all that code. The compiler does not actually make them UB and the standard library can rely on that, but user code cannot.

Also see discussion in

Evaluating what it takes to make Arc avoid this potential UB would still be valuable data for this decision.

My own position is that this should not be UB, expect when the reference points to an uninhabited type -- &! should itself be considered uninhabited.

@RalfJung RalfJung removed I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 23, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 22, 2024
rc,sync: Do not create references to uninitialized values

Closes rust-lang#119241

r? `@RalfJung`
@bors bors closed this as completed in dfe53af Jan 23, 2024
@fmease fmease added C-cleanup Category: PRs that clean code up or issues documenting cleanup. C-discussion Category: Discussion or questions that doesn't represent real issues. T-opsem Relevant to the opsem team and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-atomic Area: Atomics, barriers, and sync primitives C-cleanup Category: PRs that clean code up or issues documenting cleanup. C-discussion Category: Discussion or questions that doesn't represent real issues. T-opsem Relevant to the opsem team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants