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.

Sign up for GitHub

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

panic-in-panic-hook: formatting a message that's just a string is risk-free #122984

Merged
merged 1 commit into from
Mar 24, 2024
Merged

panic-in-panic-hook: formatting a message that's just a string is risk-free #122984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions library/std/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@
#![feature(float_gamma)]
#![feature(float_minimum_maximum)]
#![feature(float_next_up_down)]
#![feature(fmt_internals)]
#![feature(generic_nonzero)]
#![feature(hasher_prefixfree_extras)]
#![feature(hashmap_internals)]
Expand Down
14 changes: 9 additions & 5 deletions library/std/src/panicking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,7 @@ pub mod panic_count {
pub fn increase(run_panic_hook: bool) -> Option<MustAbort> {
let global_count = GLOBAL_PANIC_COUNT.fetch_add(1, Ordering::Relaxed);
if global_count & ALWAYS_ABORT_FLAG != 0 {
// Do *not* access thread-local state, we might be after a `fork`.
return Some(MustAbort::AlwaysAbort);
}

Expand Down Expand Up @@ -744,19 +745,22 @@ fn rust_panic_with_hook(
if let Some(must_abort) = must_abort {
match must_abort {
panic_count::MustAbort::PanicInHook => {
// Don't try to print the message in this case
// - perhaps that is causing the recursive panics.
// Don't try to format the message in this case, perhaps that is causing the
// recursive panics. However if the message is just a string, no user-defined
// code is involved in printing it, so that is risk-free.
let msg_str = message.and_then(|m| m.as_str()).map(|m| [m]);
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
let message = msg_str.as_ref().map(|m| fmt::Arguments::new_const(m));
let panicinfo = PanicInfo::internal_constructor(
None, // no message
location, // but we want to show the location!
message.as_ref(),
location,
can_unwind,
force_no_backtrace,
);
rtprintpanic!("{panicinfo}\nthread panicked while processing panic. aborting.\n");
Comment on lines +748 to 759
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason I thought that this would show the message as unformatted, not just when it contains no formatting args.
ie. I thought it shows both cases:

  • panic!("some unformatted string"); would show as some unformatted string
  • panic!("some formatted string i={} j={}", i, j); would show as some formatted string i={} j={} (literally)

but the latter doesn't show at all(which is indeed intended to be so by this PR, no problem),
therefore for my local rust copy that I'm gonna use, I had to change it and make it do that second case too(well, almost that: the {} won't be visible, it's acceptable compromise - I don't really need to see those, given that it would only complicate the code used to show them) by replacing lines 751 and 752 from above with:

let message=message.map(|m| m.unformat_it());
Index: /var/tmp/portage/dev-lang/rust-1.75.0-r1/work/rustc-1.75.0-src/library/core/src/fmt/mod.rs
===================================================================
--- .orig/var/tmp/portage/dev-lang/rust-1.75.0-r1/work/rustc-1.75.0-src/library/core/src/fmt/mod.rs
+++ /var/tmp/portage/dev-lang/rust-1.75.0-r1/work/rustc-1.75.0-src/library/core/src/fmt/mod.rs
@@ -352,6 +352,20 @@ impl<'a> Arguments<'a> {
         Arguments { pieces, fmt: Some(fmt), args }
     }
 
+    /// just return the unformatted string
+    /// so this:
+    /// "some formatted string i={} j={}"
+    /// would be seen as:
+    /// "some formatted string i= j="
+    /// because the {}s get removed and transformed into the args array.
+    /// this assumes `pub fn write` was already patched to handle the extra pieces,
+    /// else you only see the first piece like:
+    /// "some formatted string i="
+    #[inline]
+    pub fn unformat_it(&self) -> Arguments<'a> {
+        Arguments { pieces:self.pieces, fmt: None, args: &[] }
+    }
+
     /// Estimates the length of the formatted text.
     ///
     /// This is intended to be used for setting initial `String` capacity
@@ -1140,10 +1154,23 @@ pub fn write(output: &mut dyn Write, arg
     }
 
     // There can be only one trailing string piece left.
-    if let Some(piece) = args.pieces.get(idx) {
-        formatter.buf.write_str(*piece)?;
+    //if let Some(piece) = args.pieces.get(idx) {
+    //    formatter.buf.write_str(*piece)?;
+    //}
+    //Maybe there's a ton of pieces*, that had fmt: None and args: &[]
+    //and we wanna print them all, yes they'll be unformatted like due to .unformat_it() function
+    //from Arguments struct.
+    // * not just the one trailing string piece left.
+    if idx < args.pieces.len() {
+        // Iterate over the remaining string pieces starting from idx
+        for piece in &args.pieces[idx..] {
+            // Write each piece to the formatter buffer
+            formatter.buf.write_str(piece)?;
+        }
     }
 
+
+
     Ok(())
 }

I'm just saying this in case it may be useful to anyone for any reason. Certainly I don't expect any such change(s) to be incorporated into rust as it seems hacky or unnecessary.

Doing this however has uncovered an issue(playground link) whereby the following message from within .expect isn't shown (even before this PR, ie. rust stable):

use std::fmt::{Display, self};

struct MyStruct;

impl Display for MyStruct {
    fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result {
        None::<i32>.expect("oh snap");
        todo!();//ignore this, it's for return
    }
}

fn main() {
    
        let instance = MyStruct;

        assert!(false, "oh no, '{}' was unexpected", instance);
   
}
   Compiling playground v0.0.1 (/playground)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.66s
     Running `target/debug/playground`
panicked at src/main.rs:7:21:
thread panicked while processing panic. aborting.

feel free(anyone) to make an issue out of it, I'm just "busy" exploring how to fix it locally. Note that in this PR, message is seen as being None after the two lines that transform it, this is why it's not shown, however as to why it's None after when it's clearly shown initially to be there when displayed (as {:?}(Debug is same as Display) shows as Some(oh snap)) I'm uncertain yet (in progress, might update this later)

Copy link
Member Author

@RalfJung RalfJung Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Doing this however has uncovered an issue(playground link) whereby the following message from within .expect isn't shown (even before this PR, ie. rust stable):

That's expected, the panic here uses formatting. It goes via this code path.

My PR is only expected to show the panic message for literal panic!("text here"), not for anything that involves {} in any way.

It will show in some extra cases due to this optimization, but that's not guaranteed.

This comment was marked as resolved.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean that None::.expect("oh snap"); uses formatting internally?

Yes, it does.

}
panic_count::MustAbort::AlwaysAbort => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I could use the same logic as above in this branch as well, which would fix #122940 but degrade panic printing for post-fork panics that do need formatting. Let me know what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Amanieu any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

I think #122940 is better fixed with an additional flag on the global count.

// Unfortunately, this does not print a backtrace, because creating
// a `Backtrace` will allocate, which we must to avoid here.
// a `Backtrace` will allocate, which we must avoid here.
let panicinfo = PanicInfo::internal_constructor(
message,
location,
Expand Down
1 change: 1 addition & 0 deletions tests/ui/panics/panic-in-message-fmt.run.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
panicked at $DIR/panic-in-message-fmt.rs:18:9:
not yet implemented
thread panicked while processing panic. aborting.
Loading