From 3444cd520f59d4c2098b0a4880637e1368f25adc Mon Sep 17 00:00:00 2001 From: Nick Santana Date: Mon, 23 Jan 2023 13:33:23 -0800 Subject: [PATCH] Use a static buffer for panic messages 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`. --- io/Cargo.toml | 1 + io/src/lib.rs | 5 +- io/src/write_buffer.rs | 157 +++++++++++++++++++++++++++++++++++++++++ panic/log/Cargo.toml | 1 + panic/log/src/lib.rs | 38 ++++++++-- 5 files changed, 195 insertions(+), 7 deletions(-) create mode 100644 io/src/write_buffer.rs diff --git a/io/Cargo.toml b/io/Cargo.toml index f13d36b..090cd68 100644 --- a/io/Cargo.toml +++ b/io/Cargo.toml @@ -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" diff --git a/io/src/lib.rs b/io/src/lib.rs index 8eda10f..6379ac1 100644 --- a/io/src/lib.rs +++ b/io/src/lib.rs @@ -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. /// diff --git a/io/src/write_buffer.rs b/io/src/write_buffer.rs new file mode 100644 index 0000000..adfdb74 --- /dev/null +++ b/io/src/write_buffer.rs @@ -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 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()); + } +} diff --git a/panic/log/Cargo.toml b/panic/log/Cargo.toml index 307f957..bbfa2e7 100644 --- a/panic/log/Cargo.toml +++ b/panic/log/Cargo.toml @@ -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 diff --git a/panic/log/src/lib.rs b/panic/log/src/lib.rs index 2debb43..d743a69 100644 --- a/panic/log/src/lib.rs +++ b/panic/log/src/lib.rs @@ -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 = 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."); + } +}