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

Unsound Sync implementation for ImmediateIO and TransactionalIO #1

Closed
Qwaz opened this issue Dec 19, 2020 · 3 comments
Closed

Unsound Sync implementation for ImmediateIO and TransactionalIO #1

Qwaz opened this issue Dec 19, 2020 · 3 comments

Comments

@Qwaz
Copy link

Qwaz commented Dec 19, 2020

Hello fellow Rustacean,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

unsafe impl<M, EI> Sync for ImmediateIO<M, EI>
where
M: IOMutex<Expander<EI>>,
EI: ExpanderInterface,
{
}

unsafe impl<M, EI> Sync for TransactionalIO<M, EI>
where
M: IOMutex<Expander<EI>>,
EI: ExpanderInterface,
{
}

ImmediateIO and TransactionIO implement Sync for M: IOMutex<Expander<EI>>, EI: ExpanderInterface. This bound is seemingly unsound; it allows to send a reference of ImmediateIO or TransactionIO to another thread, and from there a mutable reference of Expander<EI> can be accessed through IOMutex APIs. This could result in a thread-safety violation when EI is a non-Send type.

We suggest to add an Expander<EI>: Send bound (or an equivalent EI: Send bound) to those Sync implementations, following the stdlib Mutex's Sync implementation.

@Shnatsel
Copy link

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

@edarc
Copy link
Owner

edarc commented Mar 30, 2021

First of all thank you @Qwaz for the report, and apologies for somehow missing the original notification.

I'll test the suggested fix as soon as I am able and publish an update.

edarc added a commit that referenced this issue Mar 30, 2021
…0152 aka

#1.

Unsafe impls of Sync for both of these structs are removed, and Send bounds are
introduced where necessary to allow Sync to be properly deduced instead.

The unsafe impls were introduced due to an incorrect analysis of why Sync was
not being auto-deduced as expected for these structs. The removed comment
explains the original flawed analysis, which attributed it to the presence of a
struct member of type PhantomData<EI>, under the assumption that the only
"real" value of type EI in the struct is ultimately inside an IOMutex that
should bless it with Sync-ness regardless if EI is itself Sync.

In fact, Sync was not being deduced due to the absence of a *Send* bound on the
type EI, which is required for Expander<EI> to be Send, which is required for
IOMutex<Expander<EI>> to be *Sync*. The presence of the PhantomData<EI> member
turns out to be entirely irrelevant.

The bug could theoretically result in a data race if either type is used with
an EI that is not safe to send between threads.

This is a breaking change due to the addition of a trait bound in the public
API.
@edarc
Copy link
Owner

edarc commented Mar 30, 2021

This is fixed in 0.2.0. Additionally, the crate now no longer has any unsafe in it. I will send a PR on the advisory shortly.

@edarc edarc closed this as completed Mar 30, 2021
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

No branches or pull requests

3 participants