Repository: theseus-os/Theseus (language: rust)

Issue:
Undefined behaviour in aarch64 `logger`
## 1
```rust
pub static mut LOGGER: Logger = unsafe { MaybeUninit::uninit().assume_init() };
```
From the [`assume_init` docs](https://docs.rs/maybe-uninit/2.0.0/maybe_uninit/union.MaybeUninit.html#method.assume_init):
> It is up to the caller to guarantee that the MaybeUninit<T> really is in an initialized state. Calling this when the content is not yet fully initialized causes immediate undefined behavior.

We should either `assume_init` in `log` or wrap the logger in an `Option`.

## 2
```rust
let mutable = unsafe { (self as *const Self).cast_mut().as_mut().unwrap() };
```
From the [Nomicon](https://doc.rust-lang.org/nomicon/transmutes.html):
> - Transmuting an & to &mut is Undefined Behavior. While certain usages may appear safe, note that the Rust optimizer is free to assume that a shared reference won't change through its lifetime and thus such transmutation will run afoul of those assumptions. So:
>     - Transmuting an & to &mut is always Undefined Behavior.
>     - No you can't do it.
>     - No you're not special.

We should use a mutex or unsafe cell.


Discussion / hints:
@NathanRoyer can chime in here.

I think this will likely be removed once we have UEFI support for aarch64, since we plan to merge the code in `/aarch64` into the main repo.

Fix patch:
diff --git a/aarch64/Cargo.lock b/aarch64/Cargo.lock
index 408f945343..10a7d382fd 100644
--- a/aarch64/Cargo.lock
+++ b/aarch64/Cargo.lock
@@ -170,6 +170,7 @@ dependencies = [
 name = "logger"
 version = "0.1.0"
 dependencies = [
+ "irq_safety",
  "log",
  "pl011_qemu",
 ]
@@ -300,7 +301,6 @@ dependencies = [
 name = "page_table_entry"
 version = "0.1.0"
 dependencies = [
- "bit_field 0.7.0",
  "frame_allocator",
  "kernel_config",
  "memory_structs",
diff --git a/aarch64/kernel/logger/Cargo.toml b/aarch64/kernel/logger/Cargo.toml
index 492a2009a6..9425c8f39e 100644
--- a/aarch64/kernel/logger/Cargo.toml
+++ b/aarch64/kernel/logger/Cargo.toml
@@ -8,4 +8,5 @@ path = "lib.rs"
 
 [dependencies]
 log = "0.4.17"
+irq_safety = { git = "https://github.com/theseus-os/irq_safety" }
 pl011_qemu = { git = "https://github.com/theseus-os/pl011/", branch = "aarch64-qemu-virt-test" }
diff --git a/aarch64/kernel/logger/lib.rs b/aarch64/kernel/logger/lib.rs
index fdb80f5f60..7d889e26b2 100644
--- a/aarch64/kernel/logger/lib.rs
+++ b/aarch64/kernel/logger/lib.rs
@@ -2,36 +2,58 @@
 
 use pl011_qemu::{PL011, UART1};
 use log::{Record, Metadata, Log, set_logger, set_max_level, STATIC_MAX_LEVEL};
-use core::{fmt::Write, mem::MaybeUninit};
+use core::fmt::Write;
+use irq_safety::MutexIrqSafe;
 
 type QemuVirtUart = PL011<UART1>;
 
-pub struct Logger {
-    pub pl011: PL011<UART1>,
+/// This wraps a UART channel handle.
+pub struct QemuVirtUartLogger {
+    pub(crate) uart: MutexIrqSafe<QemuVirtUart>,
 }
 
-impl Log for Logger {
+impl Log for QemuVirtUartLogger {
     fn enabled(&self, _metadata: &Metadata) -> bool {
+        // allow all messages
         true
     }
 
     fn log(&self, record: &Record) {
-        let mutable = unsafe { (self as *const Self).cast_mut().as_mut().unwrap() };
+        let mut mutable_uart = self.uart.lock();
 
         if self.enabled(record.metadata()) {
-            let _ = write!(&mut mutable.pl011, "{} - {}\r\n", record.level(), record.args());
+            // result is discarded because we
+            // have no alternative way to signal
+            // an issue to the user
+            let _ = write!(&mut mutable_uart, "{} - {}\r\n", record.level(), record.args());
         }
     }
 
     fn flush(&self) {}
 }
 
-pub static mut LOGGER: Logger = unsafe { MaybeUninit::uninit().assume_init() };
+// Global logger Singleton
+static mut LOGGER: Option<QemuVirtUartLogger> = None;
 
+/// Initialize the internal global "LOGGER" singleton
+/// and sets it as the system-wide logger for the `log`
+/// crate.
+///
+/// Bootstrapping code must call this as early
+/// as possible for all log messages to show up
+/// on the UART output of Qemu.
 pub fn init() -> Result<(), &'static str> {
     set_max_level(STATIC_MAX_LEVEL);
-    unsafe {
-        LOGGER = Logger { pl011: QemuVirtUart::new(UART1::take().unwrap()) };
-        set_logger(&LOGGER).map_err(|_| "logger::init - couldn't set logger")
-    }
+
+    let uart1 = UART1::take().unwrap();
+    let logger = QemuVirtUartLogger {
+        uart: MutexIrqSafe::new(QemuVirtUart::new(uart1)),
+    };
+
+    let logger_static = unsafe {
+        LOGGER = Some(logger);
+        LOGGER.as_ref().unwrap()
+    };
+
+    set_logger(logger_static).map_err(|_| "logger::init - couldn't set logger")
 }


Test patch: