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

stdlib is instantiating Arc<T> with T's that are not Send+Sync #23584

Closed
pnkfelix opened this issue Mar 21, 2015 · 8 comments · Fixed by #23638
Closed

stdlib is instantiating Arc<T> with T's that are not Send+Sync #23584

pnkfelix opened this issue Mar 21, 2015 · 8 comments · Fixed by #23638

Comments

@pnkfelix
Copy link
Member

From arc.rs, we have:

impl<T: Sync + Send> Drop for Arc<T> { ... }

From thread.rs, we have:

struct Packet<T>(Arc<UnsafeCell<Option<Result<T>>>>);

And, for completeness, from cell.rs, we have:

impl<T> !Sync for UnsafeCell<T> {}

This is bad. The Drop method of Arc gets to assume that T adheres to the stated bounds, but Packet is violating that promise. (The current Rust implementation just blindly emits a Drop impl and invokes it; so presumably the invariants of Arc itself are maintained, but the global invariants of the system need not be.)

We have not been checking for this; such a check is the task described in #8142.

But we need to fix this; certainly as long as it is the case, the planned implementation of #8142 cannot land.

cc @alexcrichton @aturon @nikomatsakis

@pnkfelix
Copy link
Member Author

(or maybe I can get away with just removing the bounds from the Drop impl for Arc<T> ...

@pnkfelix
Copy link
Member Author

see also #23176, since it may help remove some of the bounds that we causing us to put these bounds on the Drop impl in the first place.

@pnkfelix
Copy link
Member Author

see also rust-lang/rfcs#590, which proposes a coding convention which may have helped us avoid this bug.

@alexcrichton
Copy link
Member

Yes this is just a mistake, it's fine to just remove the bounds from the Drop impl as they're not really needed other than to maybe call a helper method or two (which also don't need the Send+Sync bounds).

@steveklabnik
Copy link
Member

is this backcompat-libs?

@pnkfelix
Copy link
Member Author

removing the bounds from the Drop impl for Arc should be a backwards-compatible change.

But there are other cases where the right thing to do is to follow rust-lang/rfcs#590 and put the bounds on the struct/enum definition itself, and those cases would not be backwards-compatible changes.

@pnkfelix
Copy link
Member Author

i am currently thinking I may try to land an incomplete fix for #8142, namely one that accepts some programs that it should reject (namely programs that put outlives-predicates on the Drop implementations without a corresponding bound on the struct/enum definition), just to catch the majority of cases that we expect to encounter and thus limit our exposure to this bug.

@pnkfelix
Copy link
Member Author

actually i got a complete fix for #8142 working; see PR #23638. That PR should resolve this ticket.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 24, 2015
Reject specialized Drop impls.

See Issue rust-lang#8142 for discussion.

This makes it illegal for a Drop impl to be more specialized than the original item.

So for example, all of the following are now rejected (when they would have been blindly accepted before):

```rust
struct S<A> { ... };
impl Drop for S<i8> { ... } // error: specialized to concrete type

struct T<'a> { ... };
impl Drop for T<'static> { ... } // error: specialized to concrete region

struct U<A> { ... };
impl<A:Clone> Drop for U<A> { ... } // error: added extra type requirement

struct V<'a,'b>;
impl<'a,'b:a> Drop for V<'a,'b> { ... } // error: added extra region requirement
```

Due to examples like the above, this is a [breaking-change].

(The fix is to either remove the specialization from the `Drop` impl, or to transcribe the requirements into the struct/enum definition; examples of both are shown in the PR's fixed to `libstd`.)

----

This is likely to be the last thing blocking the removal of the `#[unsafe_destructor]` attribute.

Fix rust-lang#8142
Fix rust-lang#23584
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants