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

Jump to bottom

Improve panic handling code and communication #569

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Improve panic handling code and communication #569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7dd9fa5
Add panic handler function
webmaster128 Oct 30, 2024
b785826
Move called function and error into handle_vm_panic
webmaster128 Oct 30, 2024
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
19 changes: 10 additions & 9 deletions libwasmvm/src/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use serde::Serialize;
use crate::api::GoApi;
use crate::args::{CACHE_ARG, CHECKSUM_ARG, CONFIG_ARG, WASM_ARG};
use crate::error::{handle_c_error_binary, handle_c_error_default, handle_c_error_ptr, Error};
use crate::handle_vm_panic::handle_vm_panic;
use crate::memory::{ByteSliceView, UnmanagedVector};
use crate::querier::GoQuerier;
use crate::storage::GoStorage;
Expand All @@ -31,7 +32,7 @@ pub extern "C" fn init_cache(
error_msg: Option<&mut UnmanagedVector>,
) -> *mut cache_t {
let r = catch_unwind(|| do_init_cache(config)).unwrap_or_else(|err| {
eprintln!("Panic in do_init_cache: {err:?}");
handle_vm_panic("do_init_cache", err);
Err(Error::panic())
});
handle_c_error_ptr(r, error_msg) as *mut cache_t
Expand All @@ -56,7 +57,7 @@ pub extern "C" fn save_wasm(
let r = match to_cache(cache) {
Some(c) => catch_unwind(AssertUnwindSafe(move || do_save_wasm(c, wasm, unchecked)))
.unwrap_or_else(|err| {
eprintln!("Panic in do_save_wasm: {err:?}");
handle_vm_panic("do_save_wasm", err);
Err(Error::panic())
}),
None => Err(Error::unset_arg(CACHE_ARG)),
Expand Down Expand Up @@ -88,7 +89,7 @@ pub extern "C" fn remove_wasm(
let r = match to_cache(cache) {
Some(c) => catch_unwind(AssertUnwindSafe(move || do_remove_wasm(c, checksum)))
.unwrap_or_else(|err| {
eprintln!("Panic in do_remove_wasm: {err:?}");
handle_vm_panic("do_remove_wasm", err);
Err(Error::panic())
}),
None => Err(Error::unset_arg(CACHE_ARG)),
Expand Down Expand Up @@ -117,7 +118,7 @@ pub extern "C" fn load_wasm(
let r = match to_cache(cache) {
Some(c) => catch_unwind(AssertUnwindSafe(move || do_load_wasm(c, checksum)))
.unwrap_or_else(|err| {
eprintln!("Panic in do_load_wasm: {err:?}");
handle_vm_panic("do_load_wasm", err);
Err(Error::panic())
}),
None => Err(Error::unset_arg(CACHE_ARG)),
Expand Down Expand Up @@ -147,7 +148,7 @@ pub extern "C" fn pin(
let r = match to_cache(cache) {
Some(c) => {
catch_unwind(AssertUnwindSafe(move || do_pin(c, checksum))).unwrap_or_else(|err| {
eprintln!("Panic in do_pin: {err:?}");
handle_vm_panic("do_pin", err);
Err(Error::panic())
})
}
Expand Down Expand Up @@ -177,7 +178,7 @@ pub extern "C" fn unpin(
let r = match to_cache(cache) {
Some(c) => {
catch_unwind(AssertUnwindSafe(move || do_unpin(c, checksum))).unwrap_or_else(|err| {
eprintln!("Panic in do_unpin: {err:?}");
handle_vm_panic("do_unpin", err);
Err(Error::panic())
})
}
Expand Down Expand Up @@ -285,7 +286,7 @@ pub extern "C" fn analyze_code(
let r = match to_cache(cache) {
Some(c) => catch_unwind(AssertUnwindSafe(move || do_analyze_code(c, checksum)))
.unwrap_or_else(|err| {
eprintln!("Panic in do_analyze_code: {err:?}");
handle_vm_panic("do_analyze_code", err);
Err(Error::panic())
}),
None => Err(Error::unset_arg(CACHE_ARG)),
Expand Down Expand Up @@ -363,7 +364,7 @@ pub extern "C" fn get_metrics(
let r = match to_cache(cache) {
Some(c) => {
catch_unwind(AssertUnwindSafe(move || do_get_metrics(c))).unwrap_or_else(|err| {
eprintln!("Panic in do_get_metrics: {err:?}");
handle_vm_panic("do_get_metrics", err);
Err(Error::panic())
})
}
Expand Down Expand Up @@ -418,7 +419,7 @@ pub extern "C" fn get_pinned_metrics(
let r = match to_cache(cache) {
Some(c) => {
catch_unwind(AssertUnwindSafe(move || do_get_pinned_metrics(c))).unwrap_or_else(|err| {
eprintln!("Panic in do_get_pinned_metrics: {err:?}");
handle_vm_panic("do_get_pinned_metrics", err);
Err(Error::panic())
})
}
Expand Down
5 changes: 3 additions & 2 deletions libwasmvm/src/calls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::args::{ARG1, ARG2, ARG3, CACHE_ARG, CHECKSUM_ARG, GAS_REPORT_ARG};
use crate::cache::{cache_t, to_cache};
use crate::db::Db;
use crate::error::{handle_c_error_binary, Error};
use crate::handle_vm_panic::handle_vm_panic;
use crate::memory::{ByteSliceView, UnmanagedVector};
use crate::querier::GoQuerier;
use crate::storage::GoStorage;
Expand Down Expand Up @@ -528,7 +529,7 @@ fn call_2_args(
)
}))
.unwrap_or_else(|err| {
eprintln!("Panic in do_call_2_args: {err:?}");
handle_vm_panic("do_call_2_args", err);
Err(Error::panic())
}),
None => Err(Error::unset_arg(CACHE_ARG)),
Expand Down Expand Up @@ -622,7 +623,7 @@ fn call_3_args(
)
}))
.unwrap_or_else(|err| {
eprintln!("Panic in do_call_3_args: {err:?}");
handle_vm_panic("do_call_3_args", err);
Err(Error::panic())
}),
None => Err(Error::unset_arg(CACHE_ARG)),
Expand Down
34 changes: 34 additions & 0 deletions libwasmvm/src/handle_vm_panic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use std::any::Any;

/// A function to process cases in which the VM panics.
///
/// We want to provide as much debug information as possible
/// as those cases are not expated to happen during healthy operations.
pub fn handle_vm_panic(what: &str, err: Box<dyn Any + Send + 'static>) {
eprintln!("Panic in {what}:");
eprintln!("{err:?}"); // Does not show useful information, see https://users.rust-lang.org/t/return-value-from-catch-unwind-is-a-useless-any/89134/6
eprintln!(
"This indicates a panic in during the operations of libwasmvm/cosmwasm-vm.
Such panics must not happen and are considered bugs. If you see this in any real-world or
close-to-real-world usage of wasmvm, please consider filing a security report,
no matter if it can be abused or not:
(https://github.com/CosmWasm/advisories/blob/main/SECURITY.md#reporting-a-vulnerability).
Thank you for your help keeping CosmWasm safe and secure 💚"
);
}

#[cfg(test)]
mod tests {
use super::*;

use std::panic::catch_unwind;

#[test]
fn handle_vm_panic_works() {
fn nice_try() {
panic!("oh no!");
}
let err = catch_unwind(nice_try).unwrap_err();
handle_vm_panic("nice_try", err);
}
Comment on lines +27 to +33
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a bit of a nitpick, but I'm not sure how I feel about this test. It's really testing catch_unwind rather than handle_vm_panic. Basically the only thing this ensures about our code is that handle_vm_panic doesn't panic itself.

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 agree for the automated case and it might not be a text book style test. However, it is very handy for manual inspection of the console output (which revealed the Any debug problem) as well as ensuring the function can be used in isolation with the weird input type. Do you have an idea how to improve the test?

Copy link
Member

Choose a reason for hiding this comment

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

The guard pattern maybe? A bit verbose but no parameter needed?

static VM_WRITER: RefCell<Option<Box<dyn Writer>>> = RefCell::new(None);

fn with_writer<W: Write + 'static, F: Fn(&dyn Writer)>(w: W, func: F) {
    let prev = VM_WRITER.get();
    VM_WRITER.set(Some(Box::new(w)));
    access_writer(func);
    VM_WRITER.set(prev);
}

fn access_writer<F: Fn(&dyn Writer)>(func: F) {
    VM_WRITER.with_borrow(func);
}

[...]

access_writer(|w| writeln!(w, "hello world!"));

Copy link
Member

Choose a reason for hiding this comment

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

Well, that code itself won't work immediately because .with_borrow isn't enough and it needs a fallback case to io::stdout() but close enough to illustrate what I mean.

Copy link
Member

Choose a reason for hiding this comment

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

And the overhead is alright since it's a rare case to occur anyway (hopefully never)

Copy link
Member Author

Choose a reason for hiding this comment

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

No strong opinion on if/how to improve this. But if there are no easy wins, I'd merge this as is and leave it up to you to improve it later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, then let's merge as is for now.

}
1 change: 1 addition & 0 deletions libwasmvm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ mod db;
mod error;
mod gas_meter;
mod gas_report;
mod handle_vm_panic;
mod iterator;
mod memory;
mod querier;
Expand Down
Loading