Root<T>: Data race allowed on T

[kuzeyardabulut]

Hi, I found a memory-safety/soundness issue in this crate while scanning Rust code for potential vulnerabilities. This PR contains a fix for the issue.

Issue Description

Root<T> unconditionally implements Sync. This allows users to create data races on T: !Sync. Such data races can lead to undefined behavior. https://github.com/neon-bindings/neon/blob/4c2e455a9e6814f1ba0178616d63caec7f4df317/crates/neon/src/handle/root.rs#L71-L73

[kjvalencik]

Can you give an example of such a data race? The only thing that Root can ever hold is a napi_ref, which can be sent across threads.

Root can't hold any T, it's specifically a wrapper to allow safely holding references to JS values across threads. They can only ever be dereferenced on the JS main thread.

[kuzeyardabulut]

I agree with what you said. This may not cause a direct data race. But making the above changes is useful if it doesn't break the flow of the code.

[kjvalencik]

But making the above changes is useful if it doesn't break the flow of the code.

It does because T is the type that will be pulled from the Root on the main thread and that type is explicitly not Sync.

If you take a look at CI, there are a bunch of use cases no longer compiling (the most common one is dropping a Root in a global).

[kjvalencik]

Closing the loop on this, after discussing and examining with a few folks:

• This is sound as Root may only be constructed with T: Object and does not contain any references to T
• Requiring T: Send and T: Sync is not possible because the intent of this type is a safe Send/Sync wrapper around normally !Send + !Sync references to JS values on the V8 heap.
• We could add a bound on struct Root to require T: Value, but this does not increase external safety (already protected by private fields) and would propagate T bounds unnecessarily.