Skip to content

Commit

Permalink
Use a static buffer for panic messages
Browse files Browse the repository at this point in the history
Previously panic messages were using `to_string()` which required
allocating a string. Now a static buffer is used for building up the
panic message to prevent possibly panicking allocating the message
`String`.
  • Loading branch information
nick-mobilecoin committed Jan 23, 2023
1 parent 08c5aef commit 3444cd5
Show file tree
Hide file tree
Showing 5 changed files with 195 additions and 7 deletions.
1 change: 1 addition & 0 deletions io/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ mc-sgx-util = "0.4.0"
[dev-dependencies]
once_cell = "1.16.0"
serial_test = "0.9.0"
yare = "1.0.1"
5 changes: 4 additions & 1 deletion io/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
// Copyright (c) 2022 The MobileCoin Foundation
// Copyright (c) 2022-2023 The MobileCoin Foundation

#![doc = include_str!("../README.md")]
#![deny(missing_docs, missing_debug_implementations)]
#![no_std]

mod write_buffer;

use core::ffi::c_void;
use mc_sgx_core_sys_types::sgx_status_t;
use mc_sgx_core_types::Error;
use mc_sgx_util::ResultInto;
pub use write_buffer::WriteBuffer;

/// Write the entire `buffer` into the hosts stderr sink.
///
Expand Down
157 changes: 157 additions & 0 deletions io/src/write_buffer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
// Copyright (c) 2023 The MobileCoin Foundation

//! Provides a sized buffer which implements the [`fmt::Write`] trait.
use core::fmt;

/// Byte size of [`WriteBuffer`].
///
/// Attempting to write more than this many bytes to the [`WriteBuffer`] will
/// result in an error.
pub const BUFFER_SIZE: usize = 4096;

/// A buffer which implements the [`fmt::Write`] trait.
#[derive(Debug)]
pub struct WriteBuffer {
buf: [u8; BUFFER_SIZE],
pos: usize,
}

impl WriteBuffer {
/// Create a new empty [`WriteBuffer`]
pub const fn new() -> Self {
WriteBuffer {
buf: [0; BUFFER_SIZE],
pos: 0,
}
}

/// Clear the contents in the [`WriteBuffer`]
pub fn clear(&mut self) {
self.pos = 0;
}
}

impl AsRef<str> for WriteBuffer {
fn as_ref(&self) -> &str {
// Shouldn't fail because [`Write::write_str()`] is the only public way
// to add content. [`Write::write_str()`] takes a `&str` so for this to
// fail someone must have coerced invalid UTF-8 to a string prior to
// this method invocation.
core::str::from_utf8(&self.buf[..self.pos])
.expect("`WriteBuffer` is not valid UTF-8. It should have only been given `&str`s")
}
}

impl AsRef<[u8]> for WriteBuffer {
fn as_ref(&self) -> &[u8] {
&self.buf[..self.pos]
}
}

impl fmt::Write for WriteBuffer {
fn write_str(&mut self, string: &str) -> fmt::Result {
let bytes = string.as_bytes();

let remaining = &mut self.buf[self.pos..];
if remaining.len() < bytes.len() {
return Err(fmt::Error);
}

let new_contents = &mut remaining[..bytes.len()];
new_contents.copy_from_slice(bytes);

self.pos += bytes.len();

Ok(())
}
}

#[cfg(test)]
mod test {
extern crate std;

use super::*;
use core::fmt::Write;
use std::vec;
use yare::parameterized;

#[test]
fn new_write_buffer_is_empty() {
let buffer = WriteBuffer::new();

let contents: &str = buffer.as_ref();
assert_eq!(contents, "");
}

#[parameterized(
what = {&["what"], "what"},
hello = {&["hello"], "hello"},
good_bye = {&["good", " ", "bye"], "good bye"},
i_like_long_strings = {&["i", " ", "like", " ", "long", " ", "strings"], "i like long strings"},
no_spaces = {&["no", "spaces"], "nospaces"},
)]
fn write_buffer_contains_the_written_contents(messages: &[&str], expected: &str) {
let mut buffer = WriteBuffer::new();

for message in messages {
buffer
.write_str(message)
.expect("Shouldn't fail to write message");
}

let contents: &str = buffer.as_ref();
assert_eq!(contents, expected);
}

#[parameterized(
why_not = {b"why not"},
you_bet = {b"you_bet"},
)]
fn write_buffer_as_bytes(message: &[u8]) {
let mut buffer = WriteBuffer::new();
let message_str = core::str::from_utf8(message).expect("Message should be valid UTF-8");

buffer
.write_str(message_str)
.expect("Shouldn't fail to write message");

let contents: &[u8] = buffer.as_ref();
assert_eq!(contents, message);
}

#[test]
fn write_buffer_can_hold_4096_bytes() {
let mut buffer = WriteBuffer::new();
// 66 == 'B'
let mut message = vec![66u8; BUFFER_SIZE - 1];
let message_str = core::str::from_utf8(&message).expect("Message should be valid UTF-8");

buffer
.write_str(message_str)
.expect("Shouldn't fail to write message");
buffer
.write_str("C")
.expect("Shouldn't fail to write last byte");

let contents: &[u8] = buffer.as_ref();
message.push(67);
assert_eq!(contents, message);
}

#[test]
fn write_buffer_errors_at_4097_bytes() {
let mut buffer = WriteBuffer::new();
let message = [66u8; BUFFER_SIZE - 1];
let message_str = core::str::from_utf8(&message).expect("Message should be valid UTF-8");

buffer
.write_str(message_str)
.expect("Shouldn't fail to write message");
buffer
.write_str("C")
.expect("Shouldn't fail to write last byte");

assert!(buffer.write_str("D").is_err());
}
}
1 change: 1 addition & 0 deletions panic/log/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ keywords = ["sgx", "no-std", "panic"]

[dependencies]
mc-sgx-io = { path = "../../io", version = "0.1.0" }
mc-sgx-sync = { path = "../../sync", version = "0.1.0" }

# This is a crate that can only be built for an SGX target, so it's not part of
# the root workspace. Because of this limitation we must re-iterate the
Expand Down
38 changes: 32 additions & 6 deletions panic/log/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,41 @@
#![deny(missing_docs, missing_debug_implementations)]
#![no_std]

extern crate alloc;
use alloc::string::ToString;
use core::fmt::Write;
use core::panic::PanicInfo;
use mc_sgx_io::WriteBuffer;
use mc_sgx_sync::Mutex;

/// A buffer for building up the panic message.
/// We avoid allocating when handling the panic as failure to allocate could be
/// the cause of the panic.
static MESSAGE_BUFFER: Mutex<WriteBuffer> = Mutex::new(WriteBuffer::new());

#[panic_handler]
fn panic(info: &PanicInfo) -> ! {
// Ignore the result, we're already panicking we can't really do much if
// `stderr_write_all()` fails
let _ = mc_sgx_io::stderr_write_all(info.to_string().as_bytes());

log_panic_info(info);
loop {}
}

/// Log information during a panic
///
/// If for some reason the `info` exceeds the size of the [`MESSAGE_BUFFER`]
/// then this will log a default message.
///
/// # Arguments:
/// * `info` - The panic information to log
fn log_panic_info(info: &PanicInfo) {
if let Ok(mut buffer) = MESSAGE_BUFFER.lock() {
buffer.clear();
let message = match write!(buffer, "{info}") {
Ok(()) => buffer.as_ref(),
_ => "Failed to format panic log info.",
};

// Ignore the result, we're already panicking we can't really do much if
// `stderr_write_all()` fails
let _ = mc_sgx_io::stderr_write_all(message.as_bytes());
} else {
let _ = mc_sgx_io::stderr_write_all(b"Mutex for panic logging has been poisoned.");
}
}

0 comments on commit 3444cd5

Please sign in to comment.