Detect simultaneous borrow and drop of UnsafeCell #351
Conversation
Dropping an UnsafeCell or calling into_inner can be seen as mutable access, which is not allowed to happen while another borrow exists. The downside of this change is the added Drop implementation of UnsafeCell. This restricts some uses of UnsafeCell that were previously allowed related to drop check. I went with an approach using unsafe. It is possible to implement this safely but it has too much overhead. We would have to wrap the data field with Option to allow safely taking. This is fine but we would also need to Box it because the type parameter T is ?Sized, which cannot be put into Option. fixes tokio-rs#349
| // Observe that we have mutable access. | ||
| self.get_mut(); |
There was a problem hiding this comment.
Should std::mem::needs_drop() be used to solve this issue:
You restrict the type's use relative to std's UnsafeCell. When Drop is implemented, drop check applies, making some uses that worked before no longer work.
| // Observe that we have mutable access. | |
| self.get_mut(); | |
| // Observe that we have mutable access if the inner type needs to be dropped. | |
| if std::mem::needs_drop::<T>() { | |
| self.get_mut(); | |
| } |
There was a problem hiding this comment.
I don't agree with this.
-
Even if the inner type doesn't need to be dropped and thus its drop implementation does nothing, dropping the cell while a reference to the inner type exists is still wrong.
-
The documentation for needs_drop says that "this is purely an optimization hint". But we're changing observable behavior here. Using it would lead to safety assertions changing whether they get triggered based on the return value of needs_drop, which can change.
There was a problem hiding this comment.
Even if the inner type doesn't need to be dropped and thus its drop implementation does nothing, dropping the cell while a reference to the inner type exists is still wrong.
Actually, this is isn't wrong to me. If an object is dropped, it means that all references pointing to it have reach the end of their lifetime, otherwise, the code wouldn't even compile.
In fact, after thinking a second time about this sentence, it means that #349 is not an issue after all, and doesn't need to be "fixed".
My bad, the abstraction around UnsafeCell makes me forget that it was about pointers and not references.
There was a problem hiding this comment.
The documentation for needs_drop says that "this is purely an optimization hint". But we're changing observable behavior here.
This is an optimization hint, but the documentation still states "However if this function actually returns false, then you can be certain dropping T has no side effect". So needs_drop result is still guaranteed to have no false-negative, and can be relied on.
Of course, as written in the documentation, it could lead to false-positives, but a few false positive may be still better than a too much conservative behavior leading to more false-positives. And if the result of needs_drop is changing with compiler versions, it's still guaranteed to not introduce false-negative.
There was a problem hiding this comment.
Anyway, see my comment in the issue, I don't think loom is doing things right here. Dropping a cell should not be counted a mutable access but as a cell invalidation, for further ConstPtr/MutPtr dereference.
Dropping an UnsafeCell or calling into_inner can be seen as mutable access, which is not allowed to happen while another borrow exists.
The downside of this change is the added Drop implementation of UnsafeCell. This restricts some uses of UnsafeCell that were previously allowed related to drop check.
I removed one of the tests because it stops working with this change due to double panic. Fixing it is awkward.
I went with an approach using unsafe. It is possible to implement this safely but it has too much overhead. We would have to wrap the data field with Option to allow safely taking. This is fine but we would also need to Box it because the type parameter T is ?Sized, which cannot be put into Option.
fixes #349