Niches of Cell and others still not hidden

@RalfJung

Original conversation can be found in this IRLO thread, including a request from @RalfJung to file a bug report.

PR 68491 was accepted to hide niches of types that are in an UnsafeCell. However, experiments show that niches are still exposed within Cell, Mutex, and others.

Consider this code, adapted from a playground by @SkiFire13. It currently runs on the playground without panicking and optimizes down to a ret in the Godbolt explorer.

    assert_eq!( 8, mem::size_of::<       UnsafeCell<&()> >()); //  8
    assert_eq!(16, mem::size_of::<Option<UnsafeCell<&()>>>()); // 16 (✗ niche opt)
    assert_eq!( 8, mem::size_of::<             Cell<&()> >()); //  8
    assert_eq!( 8, mem::size_of::<Option<      Cell<&()>>>()); //  8 (✓ niche opt)
    assert_eq!(16, mem::size_of::<          RefCell<&()> >()); // 16
    assert_eq!(24, mem::size_of::<Option<   RefCell<&()>>>()); // 24 (✗ niche opt)
    assert_eq!(24, mem::size_of::<           RwLock<&()> >()); // 24
    assert_eq!(24, mem::size_of::<Option<    RwLock<&()>>>()); // 24 (✓ niche opt)
    assert_eq!(24, mem::size_of::<            Mutex<&()> >()); // 24
    assert_eq!(24, mem::size_of::<Option<     Mutex<&()>>>()); // 24 (✓ niche opt)

I would have expected Option<Cell<&()>> to not exhibit the niche-exploiting size optimization. The RwLock and Mutex cases may or may not be problematic, depending on their platform-specific implementations. But Cell in particular is a #[repr(transparent)] wrapper around UnsafeCell.

Here is another playground by @SkiFire13 from which they observed:

Even more interesting, looks like the current behaviour of a type that contains an UnsafeCell is to allow niche optimizations only if it doesn't contains something else before the UnsafeCell, and that something is not a ZST.

This indicates that the behavior is not Cell-specific. And although there has been some discussion about revealing the niches for Cell in particular, the discussion in the PR and in this hackmd seem to indicate that the niches being exploited is not intentional, even for Cell. The behavior can be observed back until Rust 1.43 when the PR landed.

Meta

Current playground (all versions) and Godbolt on all versions I tried from 1.43 forward.

@CAD97

(Tiny correction to give proper credits: the first playground link you posted (the one with Mutex ecc ecc) is pretty much the one from @CAD97 from here. My playground (the one with custom types) is also adapted from that one.)

@eddyb

Cc @rust-lang/lang @eddyb

Note: Mutex and RwLock are probably using the niche in Box on some platforms. (Raw os locking primitives need to be boxed on some platforms). Otherwise we'd run into this bug

#68206

I guess this logic introduced by #68491 isn't useful unless Cell also had #[repr(no_niche)]:

rust/compiler/rustc_middle/src/ty/layout.rs

Lines 956 to 960 in 18840b0

    
           let niche = if def.repr.hide_niche() { 
        
               None 
        
           } else { 
        
               Niche::from_scalar(dl, Size::ZERO, scalar.clone()) 
        
           };

The issue is that UnsafeCell's niche was removed, but it was only done without touching the abi field, so it still looks like there's an available niche, it's just that the "cached computation" is missing it. And Cell being a newtype means the niche is rediscovered from abi being Scalar with its validity range having a hole in it (at 0).

What needs to happen for #[repr(no_niche)] (tho tbh I would kind of prefer doing it for the UnsafeCell lang item, I don't remember if there was a reason to add a new #[repr] instead) is what we do for unions, i.e. resetting validity ranges to the full size for the bitwidth of the scalar:

rust/compiler/rustc_middle/src/ty/layout.rs

Lines 836 to 848 in 18840b0

    
           // Normalize scalar_unit to the maximal valid range 
        
           let field_abi = match &field.abi { 
        
               Abi::Scalar(x) => Abi::Scalar(scalar_unit(x.value)), 
        
               Abi::ScalarPair(x, y) => { 
        
                   Abi::ScalarPair(scalar_unit(x.value), scalar_unit(y.value)) 
        
               } 
        
               Abi::Vector { element: x, count } => { 
        
                   Abi::Vector { element: scalar_unit(x.value), count: *count } 
        
               } 
        
               Abi::Uninhabited | Abi::Aggregate { .. } => { 
        
                   Abi::Aggregate { sized: true } 
        
               } 
        
           };

However, that might impact e.g. miri negatively, as it may make it seem like Cell<&T> could be null. Then again, IIRC miri's such validation has to traverse the fields, so it would see the &T field inside the UnsafeCell anyway.

However, that might impact e.g. miri negatively, as it may make it seem like Cell<&T> could be null. Then again, IIRC miri's such validation has to traverse the fields, so it would see the &T field inside the UnsafeCell anyway.

Miri should be fine -- it both checks the "validity range" in abi::Scalar, and recursively traverses the fields to look at their types.

Can we get some clarification on what the question being asked of @rust-lang/lang is here? Is the desire here to fix the semantics (making all of the cell types not have niches)? Or is there a question about the correct semantics? I see in the internals thread several questions about what transmutes are valid (e.g. between Option<NonZeroU32> and u32), but this issue doesn't seem to be referencing those bits of the internals thread.

Could we get a specific ask to go with this nomination?

It sounds like I should have explicitly included testing of Cell when I wrote PR #68491. At least, based on all the dialogue I can find, we were all operating under the assumption that removing the niche from UnsafeCell would imply it would also be removed from Cell (unless some action were taken to remove it from Cell as well), while the tests given on this issue demonstrate that the assumption did not hold.

However, these experimental results do concern me a little bit. Do they imply that under the current implementation, any wrapper type around UnsafeCell is also going to re-expose the niche? (Or just transparent wrapper types around UnsafeCell?)

Anyway, I'm pretty sure we are justified to make changes to hide any niche under Cell et al. There may be questions of how we deploy that change; its not something we can easily issue a future-incompatibility warning for. And I'm not sure it belongs under an edition boundary. Maybe best to go review how the original deployment for UnsafeCell was handled...

Do they imply that under the current implementation, any wrapper type around UnsafeCell is also going to re-expose the niche? (Or just transparent wrapper types around UnsafeCell?)

Any wrapper that has a non-zero-sized field before the UnsafeCell. #[repr(transparent)] is not required. See this playground (also linked in the first post)

Can we get some clarification on what the question being asked of @rust-lang/lang is here?

Basically, the question is: should Cell hide its niches? And what about other user-defined UnsafeCell wrappers?

I think the intention was that yes the niche should be hidden for all UnsafeCell wrappers. But I wanted to confirm with t-lang to be sure.

@RalfJung

Do they imply that under the current implementation, any wrapper type around UnsafeCell is also going to re-expose the niche? (Or just transparent wrapper types around UnsafeCell?)

Any wrapper that has a non-zero-sized field before the UnsafeCell. #[repr(transparent)] is not required. See this playground (also linked in the first post)

I was pretty confused by this response, because I took it to mean that the niche is only reexposed when there is another field before the UnsafeCell.

After reading the linked playground, I think that interpretation was backwards. I.e., I think the intended statement of the current behavior is that: Any struct holding an UnsafeCell that has a non-zero-sized field before the UnsafeCell will not re-expose any niche within the UnsafeCell.

(Which sounds pretty weird, in terms of why that ends up being the case where the rule is enforced...)

Anyway, I'm personally in agreement with @RalfJung that the intention was to hide the niche for all occurrences of UnsafeCell in another data type. All of the dialogue I can find supports that interpretation. It looks like I just wildly missed the mark in my implementation.

I was pretty confused by this response, because I took it to mean that the niche is only reexposed when there is another field before the UnsafeCell.

After reading the linked playground, I think that interpretation was backwards. I.e., I think the intended statement of the current behavior is that: Any struct holding an UnsafeCell that has a non-zero-sized field before the UnsafeCell will not re-expose any niche within the UnsafeCell.

You're right, I wrote it backwards. Your final interpretation is the right one.

@RalfJung

Anyway, I'm personally in agreement with @RalfJung that the intention was to hide the niche for all occurrences of UnsafeCell in another data type.

My assumption (in #87341 (comment)) was that the initial implementation "just" has a bug, where it's hiding niches but not invalid values (which give rise to niches in the first place).

There may be questions of how we deploy that change; its not something we can easily issue a future-incompatibility warning for. And I'm not sure it belongs under an edition boundary. Maybe best to go review how the original deployment for UnsafeCell was handled...

Did we guarantee anything around Cell? Feels like a crater run might be enough.

I am labeling this issue I-unsound because inserting niches into types that we've agreed are not supposed to have niches is definitely unsound territory. This bug is causing surprising soundness issues in the ecosystem:

Additionally, there is no good way for libraries to work around this bug. Using UnsafeCell<MaybeUninit<T>> does not work in general because MaybeUninit does not support ?Sized as needed in crossbeam's AtomicCell.

Example of an observable/consequential data race in what should be correct code:

// [dependencies]
// crossbeam-utils = "=0.8.8"

use crossbeam_utils::atomic::AtomicCell;
use std::num::NonZeroU128;
use std::thread;

enum Enum {
    NeverConstructed,
    Cell(AtomicCell<NonZeroU128>),
}

static STATIC: Enum = Enum::Cell(AtomicCell::new(match NonZeroU128::new(1) {
    Some(nonzero) => nonzero,
    None => unreachable!(),
}));

fn main() {
    thread::spawn(|| {
        let cell = match &STATIC {
            Enum::NeverConstructed => unreachable!(),
            Enum::Cell(cell) => cell,
        };
        let x = NonZeroU128::new(0xFFFFFFFF_FFFFFFFF_00000000_00000000).unwrap();
        let y = NonZeroU128::new(0x00000000_00000000_FFFFFFFF_FFFFFFFF).unwrap();
        loop {
            cell.store(x);
            cell.store(y);
        }
    });

    loop {
        if let Enum::NeverConstructed = STATIC {
            unreachable!(":(");
        }
    }
}

Work around rust-lang/rust#87341

@rustbot

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-high

I described the correct fix in #87341 (comment) but didn't realize nobody picked it up last year, whoops.

Rollup merge of rust-lang#99011 - oli-obk:UnsoundCell, r=eddyb

Rollup merge of #99011 - oli-obk:UnsoundCell, r=eddyb

Navigation Menu

Repository navigation

Niches of `Cell` and others still not hidden #87341

Meta

4 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	let niche = if def.repr.hide_niche() {
	None
	} else {
	Niche::from_scalar(dl, Size::ZERO, scalar.clone())
	};

	// Normalize scalar_unit to the maximal valid range
	let field_abi = match &field.abi {
	Abi::Scalar(x) => Abi::Scalar(scalar_unit(x.value)),
	Abi::ScalarPair(x, y) => {
	Abi::ScalarPair(scalar_unit(x.value), scalar_unit(y.value))
	}
	Abi::Vector { element: x, count } => {
	Abi::Vector { element: scalar_unit(x.value), count: *count }
	}
	Abi::Uninhabited \| Abi::Aggregate { .. } => {
	Abi::Aggregate { sized: true }
	}
	};

Niches of Cell and others still not hidden #87341

Description

Meta

Activity

SkiFire13 commented on Jul 21, 2021

RalfJung commented on Jul 21, 2021

RustyYato commented on Jul 21, 2021

eddyb commented on Jul 24, 2021

RalfJung commented on Jul 24, 2021

joshtriplett commented on Aug 3, 2021

pnkfelix commented on Aug 3, 2021

SkiFire13 commented on Aug 4, 2021

RalfJung commented on Aug 4, 2021

pnkfelix commented on Aug 4, 2021

SkiFire13 commented on Aug 4, 2021

eddyb commented on Aug 5, 2021

4 remaining items

dtolnay commented on May 22, 2022

apiraino commented on May 26, 2022

eddyb commented on May 27, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Niches of `Cell` and others still not hidden #87341