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

fmt::Arguments is Send+Sync regardless of its captured arguments #45197

Closed
cuviper opened this issue Oct 11, 2017 · 0 comments
Closed

fmt::Arguments is Send+Sync regardless of its captured arguments #45197

cuviper opened this issue Oct 11, 2017 · 0 comments
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@cuviper
Copy link
Member

cuviper commented Oct 11, 2017

fmt::Arguments<'a> is only parameterized on lifetime. The types of the captured arguments are erased in the contained [ArgumentV1<'a>]. Nothing restricts the Arguments aggregate from being Send or Sync, so by OIBIT it's both. Thus this compiles:

fn send<T: Send>(_: T) {}
fn sync<T: Sync>(_: T) {}

fn main() {
    // `Cell` is not `Sync`, so `&Cell` is neither `Sync` nor `Send`,
    // yet `std::fmt::Arguments` forgets this...
    let c = std::cell::Cell::new(42);
    send(format_args!("{:?}", c));
    sync(format_args!("{:?}", c));
}

I'm not sure if there are any realistic ways to accidentally abuse this, but here's a deliberate example. The spawned thread will read the Cell through the arguments, even while the main thread modifies it.

extern crate crossbeam;

use std::io::Write;
use std::cell::Cell;

fn main() {
    let c = Cell::new(1);
    threader(&c, format_args!("{:?}\n", c));
}

fn threader(c: &Cell<i32>, a: std::fmt::Arguments) {
    crossbeam::scope(|scope| {
        let delay = std::time::Duration::from_millis(100);
        
        let guard = scope.spawn(move || {
            for _ in 0..10 {
                std::thread::sleep(delay);
                std::io::stdout().write_fmt(a).unwrap();
            }
        });
        
        for _ in 0..10 {
            std::thread::sleep(delay);
            c.set(c.get() * 2);
        }
        
        guard.join();
    });
}

playground

oli-obk added a commit to oli-obk/rust that referenced this issue Oct 11, 2017
@bluss bluss closed this as completed Oct 11, 2017
@bluss bluss reopened this Oct 11, 2017
@sfackler sfackler added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Oct 11, 2017
kennytm added a commit to kennytm/rust that referenced this issue Oct 11, 2017
Prevent fmt::Arguments from being shared across threads

Fixes rust-lang#45197

This is a **breaking change**! Without doing this it's very easy to create race conditions.

There's probably a way to do this without breaking valid use cases, but it would require quite an overhaul of the formatting machinery.
@bstrie bstrie added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 11, 2017
@alexcrichton alexcrichton added P-high High priority and removed I-nominated labels Oct 31, 2017
@steveklabnik steveklabnik added the C-bug Category: This is a bug. label Nov 1, 2017
bors added a commit that referenced this issue Nov 22, 2017
Prevent fmt::Arguments from being shared across threads

Fixes #45197

This is a **breaking change**! Without doing this it's very easy to create race conditions.

There's probably a way to do this without breaking valid use cases, but it would require quite an overhaul of the formatting machinery.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High 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

6 participants