Skip to content

Overflow in mpsc::Sender::clone leads to illegal instruction #35847

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
apasel422 opened this issue Aug 20, 2016 · 6 comments
Closed

Overflow in mpsc::Sender::clone leads to illegal instruction #35847

apasel422 opened this issue Aug 20, 2016 · 6 comments
Labels
E-help-wanted Call for participation: Help is requested to fix this issue. I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@apasel422
Copy link
Contributor

The following crashes with an illegal instruction when compiled in release mode with 1.13.0-nightly (499484f56 2016-08-18) on i686-unknown-linux-gnu:

fn main() {
    let (tx, _) = std::sync::mpsc::channel::<()>();

    for _ in 0..std::usize::MAX {
        std::mem::forget(tx.clone());
    }
}

It's presumably the result of the unchecked addition in std::sync::mpsc::shared::Packet::clone_chan. Another unchecked addition is performed in std::sync::mpsc::sync::Packet::clone_chan, and other uses of fetch_add in the mpsc module should probably be audited as well.

@apasel422 apasel422 added A-libs I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. labels Aug 20, 2016
@alexcrichton alexcrichton added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 20, 2016
@arielb1
Copy link
Contributor

arielb1 commented Aug 20, 2016

The well-known "optimized leak attack".

@brson brson added the E-help-wanted Call for participation: Help is requested to fix this issue. label Aug 22, 2016
@brson
Copy link
Contributor

brson commented Aug 22, 2016

Fix the same way as with Rc/Arc.

@alexcrichton
Copy link
Member

Discussed today at @rust-lang/libs triage and the conclusion was that this'd be great to fix but isn't P-high priority b/c it's somewhat manufactured.

As @brson said the fix will probably to just do the same thing as Arc.

@alexcrichton alexcrichton added P-medium Medium priority and removed I-nominated labels Aug 23, 2016
@KiChjang
Copy link
Member

I'm looking into this, should I add a test for it? Because using the example provided in the issue description causes rustc to run for a loooooooooooooooooong time, and this would hurt PR turnover rates if I include the test.

@KiChjang
Copy link
Member

Also, why is std::sync::mpsc::shared::Packet's channels field an AtomicIsize? Shouldn't it be AtomicUsize instead?

@alexcrichton
Copy link
Member

@KiChjang yeah it's ok to omit a test in this case and also yeah I think it's fine to change to AtomicUsize as well. Thanks!

bors added a commit that referenced this issue Sep 3, 2016
Fix illegal instruction caused by overflow in channel cloning

Fixes #35847.

r? @alexcrichton
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
E-help-wanted Call for participation: Help is requested to fix this issue. I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants