-
Notifications
You must be signed in to change notification settings - Fork 13.3k
std: Prevent print panics when using TLS #29491
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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! { | ||
|
@@ -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 | | ||
LocalKeyState::Destroyed => stdout().write_fmt(args), | ||
LocalKeyState::Valid => { | ||
LOCAL_STDOUT.with(|s| { | ||
if s.borrow_state() == BorrowState::Unused { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 This is akin to calling |
||
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); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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(); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)