-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Tracking issue for {Rc, Arc}::get_mut_unchecked #63292
Comments
These functions should be renamed to |
Are the safety docs for this correct? They currently say
This means that this is sound: let mut arc = Arc::new(0);
let clone = Arc::clone(&arc);
unsafe {
*Arc::get_mut_unchecked(&mut arc) = 1;
}
if *clone == 1 {
println!("1"); // this branch is always taken
} else {
println!("0");
} I don't think this is necessarily incorrect (and Miri passes), but it should be noted that |
As far as I understand, the absence of Can you say some more about what is subtle and how you conclude that this is sound? |
The main thing is that I previously considered an I just found it nonobvious that |
We previously had |
I initially documented this as safe when there is only one reference (no sharing), then Ralf pointed out this can be relaxed: #62451 (comment) |
I wrote a bit more about this over on irlo. TL;DR I really think making the |
cc @RalfJung on this tracking issue since it plays into pointer provenance rules. |
The fix for that, IMO, is to not go through |
https://internals.rust-lang.org/t/rc-and-internal-mutability/11463/13
#![feature(get_mut_unchecked)]
use std::rc::Rc;
unsafe fn f<'a>(mut r: Rc<&'a str>, s: &'a str) {
*Rc::get_mut_unchecked(&mut r) = &s;
}
fn main() {
let x = Rc::new("Hello, world!");
{
let s = String::from("Replaced");
unsafe { f(x.clone(), &s) };
}
println!("{}", x);
} Found by @xfix So either a "this doesn't do bad things with lifetimes" rule needs to be added, or it should go back to only allowing when Later edit: for a clearer picture of what is happening, see a version with the reference-of operator lifted. Changing it from |
Good find! As far as I can tell this happens because This wouldn’t happen with
This wouldn’t happen with Making
Unfortunately "bad things" is hard to define precisely because it involves talking about variance, and
I think this sounds more reasonable. @RalfJung, what do you think? |
Consider this example: /// Safe abstraction for initializing array
struct Array<T, const N: usize> {
arr: [MaybeUninit<T>; N],
position: usize
}
impl<const N: usize> Array<u8, N> {
/// Helper for safely, efficiently reading from reader
read<R: Read>(&mut self, reader: R) -> Result<usize, Error> {
// ...
}
} This abstraction is completely safe, but can't be used behind
I believe Rust standard libraries should provide important safe abstractions such as this by default. |
I agree with that principle, only I don’t always know which abstractions are important enough. If you have a particular one in mind feel free to also propose it in a new PR or RFC. |
FWIW, I have mostly-complete WIP single-file library implementations of ( |
There’s also https://crates.io/crates/triomphe (which was initially started to remove weak references) |
Fair enough. I don't happen to need it right now (I experienced a similar situation as I was describing with some C code before), but I will keep in mind contributing if I happen to need it. |
I'd say the "bad thing" that happens here is that a "bad value" is written into the For the docs, I think we could explain this. But if you think that is too complicated, I'm also fine saying something like "to be safe, make sure this is the only |
Since this is an unsafe, unchecked, optimization API, updating the docs to mention being careful around variance with some examples of what to watch out for seems like enough to me. |
re: #63292 (comment) so this is why it's unsound to have Arc::borrow_mut with Weak references around... unless we had an Invariant type and Arc::borrow_mut was an impl on |
The docs as written do not sufficiently describe the safety concerns. For example, the following code is definitely UB: let mut a = Rc::new(5_i32);
let b = Rc::clone(&a);
let violated_ref: &i32 = b.deref();
unsafe {
*Rc::get_mut_unchecked(&mut a) = 10;
};
println!("{}", *violate_ref); The Essentially, this change would turn every The correct approach here is imo to require the same safety contract as |
Also relevant to note is that existing APIs may have already been doing things semi-equivalent to this: fn use_rc(&mut self, b: Rc<i32>) {
self.p = b.deref() as *const _;
self.rc = b;
} Writing code like this: let a = Rc::new(5);
something.use_rc(Rc::clone(&a);
Rc::get_mut_unchecked(&mut a); is now UB. This is not technically a breaking change, but it still feels weird to add a method that is UB in conjunction with many existing APIs for that type in a way that is sort of unclear. There's an example of this in a recent crate I wrote (that example also shows that the |
In this comment I use
Just to be clear up front: yes. This example is clearly UB, and we agree here.
It depends on how exactly you define dereferenced. If you define it as the instantaneous act of calling It's this latter definition, where the condition to be avoided is "a reference to the interior is alive," that needs to be upheld. I agree that the documentation could make this clearer.
I've actually previously argued that
Perhaps we should even loosen the function signature further to
I agree that using this correctly outside of when
This function was actually originally documented as only being valid when
The fact of the matter is that Footnotes
|
This really looks like it's wanting for |
See the example code I linked for why that wouldn't actually help in slightly more complicated (but still very practical and very realistic) code.
I mean, I suppose, but I don't buy at all that "dereferenced" is a state instead of an operation.
It is definitely not. As a matter of fact, the pre-condition on that method makes it very clear that it the mutability is specifically not shared. The fact that the pointer returned by With regards to the rest of your response: For those cases, it really sounds like stuff should just be inside a |
How about changing the documentation to say this:
Anyway, even better to have the safe API and just point people to it. |
That safe API is |
@xfix not really, we were talking about |
(This was worded a bit more strongly than intended due to a prior draft before I fixed up some accidental issues due to fallible memory.) The intent here was to note that I think the two reasonable approaches here are
Personally, I think it's fine if a projected rc makes
(Btw, my rc-box crate is publicly available now and provides a safe API for known-unique |
I think that making shared mutability public is just breaking separation of concerns which is generally followed in Rust. People who want it should just use
It's really great that you made it and I think it should be included in the standard library because there were already multiple cases of people getting |
In addition to variance, this also has problems with the existing (stabilized in #![feature(get_mut_unchecked)]
use std::rc::Rc;
fn main() {
let a: Rc<str> = "Hello, world!".into();
let mut b: Rc<[u8]> = a.clone().into();
unsafe {
Rc::get_mut_unchecked(&mut b).fill(0xc0); // any non-ascii byte
}
dbg!(&a);
} (Explanation of linked bytemuck PR)(skipping over size and alignment for simplicity) Bytemuck allows converting a |
I think regarding these safety issues it'd be better if people explicitly used |
I think it's necessary because &self: Rc |
In #101310 (comment) I commented:
|
@SimonSapin so can I make a PR? :) |
I agree with that. I've seen several people use it, and none used it correctly. They look too much like "this is what I need" and they are too hard/tricky to use correctly. Overall I think it would be better if these methods did not exist, and that if stabilized I feel like we'd regret it. |
…soundness, r=Mark-Simulacrum Clarify and restrict when `{Arc,Rc}::get_unchecked_mut` is allowed. (Tracking issue for `{Arc,Rc}::get_unchecked_mut`: rust-lang#63292) (I'm using `Rc` in this comment, but it applies for `Arc` all the same). As currently documented, `Rc::get_unchecked_mut` can lead to unsoundness when multiple `Rc`/`Weak` pointers to the same allocation exist. The current documentation only requires that other `Rc`/`Weak` pointers to the same allocation "must not be dereferenced for the duration of the returned borrow". This can lead to unsoundness in (at least) two ways: variance, and `Rc<str>`/`Rc<[u8]>` aliasing. ([playground link](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=d7e2d091c389f463d121630ab0a37320)). This PR changes the documentation of `Rc::get_unchecked_mut` to restrict usage to when all `Rc<T>`/`Weak<T>` have the exact same `T` (including lifetimes). I believe this is sufficient to prevent unsoundness, while still allowing `get_unchecked_mut` to be called on an aliased `Rc` as long as the safety contract is upheld by the caller. ## Alternatives * A less strict, but still sound alternative would be to say that the caller must only write values which are valid for all aliased `Rc`/`Weak` inner types. (This was [mentioned](rust-lang#63292 (comment)) in the tracking issue). This may be too complicated to clearly express in the documentation. * A more strict alternative would be to say that there must not be any aliased `Rc`/`Weak` pointers, i.e. it is required that get_mut would return `Some(_)`. (This was also mentioned in the tracking issue). There is at least one codebase that this would cause to become unsound ([here](https://github.com/kaimast/lsm-rs/blob/be5a164d770d850d905e510e2966ad4b1cc9aa5e/src/memtable.rs#L166), where additional locking is used to ensure unique access to an aliased `Rc<T>`; I saw this because it was linked on the tracking issue).
The way to do so would probably be but that was previously abandoned due to compile time regressions, which seem to be due just to LLVM having more code to chew through in the widely monomorphized allocation path. It's possible that the intervening LLVM upgrades and MIR inlining improve the situation there, if someone wants to try reviving that PR. (Unfortunately I'm too busy and have no idea how to go about diagnosing and addressing the regression if it's not just magically gone.) (For wanderers, I also have a crate providing known-unique versions of ( |
It’s one way but I think it doesn’t have to? |
…, r=Mark-Simulacrum Clarify and restrict when `{Arc,Rc}::get_unchecked_mut` is allowed. (Tracking issue for `{Arc,Rc}::get_unchecked_mut`: #63292) (I'm using `Rc` in this comment, but it applies for `Arc` all the same). As currently documented, `Rc::get_unchecked_mut` can lead to unsoundness when multiple `Rc`/`Weak` pointers to the same allocation exist. The current documentation only requires that other `Rc`/`Weak` pointers to the same allocation "must not be dereferenced for the duration of the returned borrow". This can lead to unsoundness in (at least) two ways: variance, and `Rc<str>`/`Rc<[u8]>` aliasing. ([playground link](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=d7e2d091c389f463d121630ab0a37320)). This PR changes the documentation of `Rc::get_unchecked_mut` to restrict usage to when all `Rc<T>`/`Weak<T>` have the exact same `T` (including lifetimes). I believe this is sufficient to prevent unsoundness, while still allowing `get_unchecked_mut` to be called on an aliased `Rc` as long as the safety contract is upheld by the caller. ## Alternatives * A less strict, but still sound alternative would be to say that the caller must only write values which are valid for all aliased `Rc`/`Weak` inner types. (This was [mentioned](rust-lang/rust#63292 (comment)) in the tracking issue). This may be too complicated to clearly express in the documentation. * A more strict alternative would be to say that there must not be any aliased `Rc`/`Weak` pointers, i.e. it is required that get_mut would return `Some(_)`. (This was also mentioned in the tracking issue). There is at least one codebase that this would cause to become unsound ([here](https://github.com/kaimast/lsm-rs/blob/be5a164d770d850d905e510e2966ad4b1cc9aa5e/src/memtable.rs#L166), where additional locking is used to ensure unique access to an aliased `Rc<T>`; I saw this because it was linked on the tracking issue).
On
Rc
andArc
an new unsafeget_mut_unchecked
method provides&mut T
access without checking the reference count.Arc::get_mut
involves multiple atomic operations whose cost can be non-trivial.Rc::get_mut
is less costly, but we addRc::get_mut_unchecked
anyway for symmetry withArc
.These can be useful independently, but they will presumably be typical when uninitialized constructors (tracked in #63291) are used.
An alternative with a safe API would be to introduce
UniqueRc
andUniqueArc
types that have the same memory layout asRc
andArc
(and so zero-cost conversion to them) but are guaranteed to have only one reference. But introducing entire new types feels “heavier” than new constructors on existing types, and initialization ofMaybeUninit<T>
typically requires unsafe code anyway.PR #62451 adds:
Open questions
get_unchecked_mut
to match https://doc.rust-lang.org/std/?search=get_unchecked_mut ?The text was updated successfully, but these errors were encountered: