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

std: Prevent print panics when using TLS #29491

Merged
merged 1 commit into from Nov 6, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 26 additions & 8 deletions src/libstd/io/stdio.rs
Expand Up @@ -16,11 +16,12 @@ use cmp;
use fmt;
use io::lazy::Lazy;
use io::{self, BufReader, LineWriter};
use libc;
use sync::{Arc, Mutex, MutexGuard};
use sys::stdio;
use sys_common::io::{read_to_end_uninitialized};
use sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard};
use libc;
use thread::LocalKeyState;

/// Stdout used by print! and println! macros
thread_local! {
Expand Down Expand Up @@ -576,14 +577,31 @@ pub fn set_print(sink: Box<Write + Send>) -> Option<Box<Write + Send>> {
issue = "0")]
#[doc(hidden)]
pub fn _print(args: fmt::Arguments) {
let result = LOCAL_STDOUT.with(|s| {
if s.borrow_state() == BorrowState::Unused {
if let Some(w) = s.borrow_mut().as_mut() {
return w.write_fmt(args);
}
// As an implementation of the `println!` macro, we want to try our best to
// not panic wherever possible and get the output somewhere. There are
// currently two possible vectors for panics we take care of here:
//
// 1. If the TLS key for the local stdout has been destroyed, accessing it
// would cause a panic. Note that we just lump in the uninitialized case
// here for convenience, we're not trying to avoid a panic.
// 2. If the local stdout is currently in use (e.g. we're in the middle of
// already printing) then accessing again would cause a panic.
//
// If, however, the actual I/O causes an error, we do indeed panic.
let result = match LOCAL_STDOUT.state() {
LocalKeyState::Uninitialized |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOCAL_STDOUT.with would initialize data, so I think this branch should go together with the next arm.

EDIT: this is also fine, because if stdout ends up actually changed from the global default, it would also get initialised.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this branch could actually go either way, but I figured that we may as well avoid initializing TLS if we're not gonna use it anyway (e.g. as you also mentioned it'd just go to the same place regardless)

LocalKeyState::Destroyed => stdout().write_fmt(args),
LocalKeyState::Valid => {
LOCAL_STDOUT.with(|s| {
if s.borrow_state() == BorrowState::Unused {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this should block waiting (using e.g. a spinlock) rather than falling back to global stdout.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, never mind me, this is fine, since there’s rarely a case where LOCAL_STDOUT happens to not be global stdout and two levels of locks isn’t really good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah and currently the only way that this can be in use is if the same thread is already printing (e.g. somewhere up on the stack someone called println!), so we can't actually wait for the resource to become available here because it never will.

This is akin to calling println! in a Display implementation.

if let Some(w) = s.borrow_mut().as_mut() {
return w.write_fmt(args);
}
}
stdout().write_fmt(args)
})
}
stdout().write_fmt(args)
});
};
if let Err(e) = result {
panic!("failed printing to stdout: {}", e);
}
Expand Down
30 changes: 30 additions & 0 deletions src/test/run-pass/issue-29488.rs
@@ -0,0 +1,30 @@
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use std::thread;

struct Foo;

impl Drop for Foo {
fn drop(&mut self) {
println!("test2");
}
}

thread_local!(static FOO: Foo = Foo);

fn main() {
// Off the main thread due to #28129, be sure to initialize FOO first before
// calling `println!`
thread::spawn(|| {
FOO.with(|_| {});
println!("test1");
}).join().unwrap();
}