New lint: Sync types which are generic over T, but T is not Send

[alex]

What it does

Catches un-soudness due to implmenting Sync for a type that's generic over T, but T is not Send. This often leads to unsoundness by allowing arbitrary types to be shared across threads.

It should suggest adding a T: Send bound as a solution.

Categories (optional)

• Kind: clippy::correctness (potentially also clippy::pedantic because this may have false positives)

What is the advantage of the recommended code over the original code

It is sound :-) This is (unfortunately) a relatively common issue, as you can see by going to https://rustsec.org/advisories/ and searching "Bound". Almost all of those issues have this root case. (There's even more that are currently PRs, awaiting review to become advisories: https://github.com/RustSec/advisory-db/pulls)

Drawbacks

May have false positives.

Example

struct Foo<T> {
    t: T
}

unsafe impl<T> Sync for Foo<T> {}

Could be written as:

struct Foo<T> {
    t: T
}

unsafe impl<T: Send> Sync for Foo<T> {}

[Shnatsel]

I've asked in #black-magic in Rust community Discord, and I'm told that the exceptions - when this is actually safe - include:

• PhantomData<T>, where T sometimes does not have to be Sync for the struct to soundly implement Send
• fn(T) is Sync even if T is not Send

[cuviper]

struct Foo<T> {
    t: T
}

unsafe impl<T: Send> Sync for Foo<T> {}

This is a weird example since it would get the trait automatically anyway, but that would have a T: Sync bound. I think it's actually dangerous to suggest T: Send here, because that would be incorrect in many cases. For example, if T is a Cell<_>, you may be able to send it to another thread depending on the inner type, but it's never correct to share it (Sync). The unsafe exception would be if &Foo<T> never actually accesses the inner T at all, but this is something the author needs to consider.

[chorman0773]

I've asked in #black-magic in Rust community Discord, and I'm told that the exceptions - when this is actually safe - include:

• PhantomData<T>, where T sometimes does not have to be Sync for the struct to soundly implement Send
• fn(T) is Sync even if T is not Send

Other exceptions include &T and types that deref into T (without providing a way to move out of it using only shared references). For example, Box<T,A>: Sync depends on T: Sync and A: Sync, only. This is valid because you can't move T out of a &Box<T,A>.
I would submit that this lint is completely at all, because it is trivial to do soundly, and suggesting T: Send over simply T is very much wrong as previously mentioned (in most cases, it would be required for T: Sync). The only cases this would not be valid is something like an Rc/Arc, which can be cloned and used to drop or get a &mut to the pointee.

[Qwaz]

This is (unfortunately) a relatively common issue, as you can see by going to https://rustsec.org/advisories/ and searching "Bound". Almost all of those issues have this root case. (There's even more that are currently PRs, awaiting review to become advisories: https://github.com/RustSec/advisory-db/pulls)

It seems to me that the synchronization bugs reported to RustSec DB are mostly the violation of "Send types which are generic over T, but T is not Send" and "Sync types which are generic over T, but T is not Sync."

[Qwaz]

Sync where T: Send bound is correct for types such as Channel and Mutex, which is used to move the ownership of the inner type (T) or to obtain the mut reference to the inner type (&mut T) from a reference of the wrapper type (&self). For most of the other type, Sync where T: Sync would be correct.

I support the general idea of having a lint that warns against unbounded Sync/Send implementations (this mistake was even found in the Rust official futures library), but the exact bound & how we detect them may require more discussion.

[alex]

While I filed this with a somewhat prescriptive description, I'm certainly not wedded to the set of constraints/suggested fixes I described. My real interest is in finding ways to capture the set of issues that we've seen really prevelantly in rustsec.

[cuviper]

I think it only makes sense to suggest bounds if the type is only used in fields that could have contributed to an automatic implementation, but something else forces this to be a manual implementation overall.

struct Foo<T> {
    raw: *const u8, // blocks auto-`Send`/`Sync`
    value: T, // suggest `T: Sync for Sync`, `T: Send for Send`
}
struct Bar<T> {
    raw: *const u8, // blocks auto-`Send`/`Sync`
    value: &'static T, // suggest `T: Sync` for both `Send` and `Sync`
}
struct Baz<T> {
    raw: *const T, // we can't guess the thread-safety here
}

[tarcieri]

+1 to what @alex just said.

We just saw a huge number of this class of vulnerability reported to RustSec, and as I was watching that I was wondering if there is some way to catch this class of error more automatically.

Perhaps it'd be good to make a small summary post which includes a select set of specific examples of the vulnerabilities.

[alex]

FWIW one of the reporters of these issues has indicated that their team intends to OSS the analysis and port it to clippy: rustsec/advisory-db#807 (comment)