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

When are references allowed to be deallocated while a function they were passed to runs? #433

Open
RalfJung opened this issue Jul 25, 2023 · 8 comments
Labels
A-aliasing-model Topic: Related to the aliasing model (e.g. Stacked/Tree Borrows)

Comments

@RalfJung
Copy link
Member

Generally, this is UB:

fn f(x: &i32) {
  cause_x_to_be_deallocated();
}

This lets us use the dereferenceable attribute of LLVM, which means "dereferenceable for the entire duration this function runs". That is quite useful for e.g. hoisting memory accesses out of a loop without having to figure out if some other operation deallocated x.

However, we have an exception to that rule: if x is a reference to UnsafeCell<T> (or a newtype around that), then the memory is allowed to be deallocated while the function runs. (The reference must still be dereferenceable when the function starts.) This was done to resolve rust-lang/rust#55005; the PR that manifested this change is rust-lang/rust#98017.

IMO this was absolutely required; without a change like this, Atomic*::compare_exchange cannot be used as a signal to another thread "you may now free this memory". We would need to provide raw pointer alternatives to even implement something like Arc. However @JakobDegen indicated they disagree so this is part 1 of the issue tracked here. (Though I will note that this will be hard to take back since it has been FCP'd and documented.)

Part 2 is the problem that there are still footguns: with a type like

struct S {
  b: AtomicBool,
  i: AtomicUsize,
}

if we have a method on &S that sets a flag to indicate "this can be deallocated", we still have UB. This is because there is padding, and the rules say only memory inside the UnsafeCell is allowed to be deallocated.

There are a bunch of options here. Just to list a few:

  • Accept the current rules and tell people to use raw pointers instead.
  • Follow Tree Borrows and just don't care about where in a type the UnsafeCell lives ever (Cc Stacked Borrows: How precise should UnsafeCell be tracked? #236). Even if we track UnsafeCell precisely for mutation, we could say that deallocation is allowed the moment T: !Freeze.
  • Somehow treat padding that is adjacent to an UnsafeCell as also being inside the UnsafeCell.
@CAD97
Copy link

CAD97 commented Jul 26, 2023

A fourth option that's probably worth listing:

@RalfJung
Copy link
Member Author

RalfJung commented Jul 26, 2023 via email

@RalfJung
Copy link
Member Author

RalfJung commented Jul 26, 2023 via email

@CAD97
Copy link

CAD97 commented Jul 26, 2023

It's only a fully distinct option from option 3 with respect to partial deallocation. Without partial deallocation, "can the reference be deallocated" can be answered as "all bytes are unsafecell" (option 1), "any byte is unsafecell" (option 21), or "all bytes are unsafecell or padding" (option 3 or 4). With partial deallocation, the answer to "can this byte of the reference be deallocated" can be answered as "this byte is unsafecell" (option 1), "any byte is unsafecell" (option 21), "this byte is unsafecell or padding next to unsafecell" (option 3), "all bytes are unsafecell or padding" (option 4), or some other combinations.

So yeah it's basically just option 3 but without considering partial deallocation. The opsem difference would be that the "shan't be deallocated" protector always gets applied to all or none of the retagged bytes, and the decision tree for whether to apply it is:

  • Any non-padding, non-unsafecell bytes? If so, protect. Otherwise:
  • Any non-padding unsafecell bytes? If so, don't protect. Otherwise:
  • All bytes are padding. Protect.

If I'm grasping at straws in the face of partial deallocation I could say option 4 retags padding as unsafecell iff another unsafecell byte is present, whereas option 3 uses a more clever adjacency rule.

But yes, the option is likely less different than I initially thought.


As mentioned over in #430 and probably the wrong place, I do not know why my initial reaction is that it's fine for &noalias to be "no fields are unsafecell and (recursive)" but that &dereferencable should be "all fields are unsafecell or (recursive)" — I legitimately can't identify why I apparently feel a difference in polarity there.

Footnotes

  1. Assuming that all ZSTs are Freeze, anyway. 2

@RalfJung
Copy link
Member Author

RalfJung commented Jul 26, 2023

I do not know why my initial reaction is that it's fine for &noalias to be "no fields are unsafecell and (recursive)" but that &dereferencable should be "all fields are unsafecell or (recursive)" — I legitimately can't identify why I apparently feel a difference in polarity there.

Yeah, that does seem inconsistent to me.

I think my own option 1 is to declare all bytes mutable and dealloacteable when T: !Freeze. That's what Tree Borrows does. If we do decide for more precise UnsafeCell tracking, IMO we should add protectors if and only if we retag as read-only. But we should also not add LLVM dereferenceable for the S type from the OP. So at least the padding bytes adjacent to an UnsafeCell should be considered to basically be part of that UnsafeCell (and hence mutable and deallocateable).

@CAD97
Copy link

CAD97 commented Jul 27, 2023

Just for clarity, I'm in full agreement that the OP type should be allowed to be entirely deallocated. (I very deliberately used "fields (recursively)" rather than "bytes", to make padding irrelevant.)

@Darksonn
Copy link

How about mutex? Is it guaranteed that every byte of std::sync::Mutex<()> is part of an UnsafeCell?

@RalfJung
Copy link
Member Author

RalfJung commented Jun 16, 2024 via email

@RalfJung RalfJung added the A-aliasing-model Topic: Related to the aliasing model (e.g. Stacked/Tree Borrows) label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-aliasing-model Topic: Related to the aliasing model (e.g. Stacked/Tree Borrows)
Projects
None yet
Development

No branches or pull requests

3 participants