fix `static mut` warnings in `control-plane-agent` & `host-sp-comms` #1740

hawkw · 2024-04-08T19:04:27Z

Currently, the control-plane-agent task uses static muts to declare
static buffers. These are guarded by an AtomicBool that ensures that
only a single mutable reference to these buffers is ever created.
However, they are still declared as static muts and referenced by
creating an &mut reference to those static muts, which now generates
a warning as it may be unsound in future Rust editions:

Compiler warnings:

warning: creating a mutable reference to mutable static is discouraged
     --> task/control-plane-agent/src/mgs_gimlet/host_phase2.rs:334:14
     |
334 |     unsafe { &mut PHASE2_BUF }
     |              ^^^^^^^^^^^^^^^ mutable reference to mutable static
     |
     = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
     = note: this will be a hard error in the 2024 edition
     = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior
     = note: `#[warn(static_mut_refs)]` on by default
help: use `addr_of_mut!` instead to create a raw pointer
     |
334 |     unsafe { addr_of_mut!(PHASE2_BUF) }
     |              ~~~~~~~~~~~~~~~~~~~~~~~~

warning: creating a mutable reference to mutable static is discouraged
     --> task/control-plane-agent/src/mgs_gimlet.rs:1431:14
     |
1431 |     unsafe { &mut UART_TX_BUF }
     |              ^^^^^^^^^^^^^^^^ mutable reference to mutable static
     |
     = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
     = note: this will be a hard error in the 2024 edition
     = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior
help: use `addr_of_mut!` instead to create a raw pointer
     |
1431 |     unsafe { addr_of_mut!(UART_TX_BUF) }
     |              ~~~~~~~~~~~~~~~~~~~~~~~~~

warning: creating a mutable reference to mutable static is discouraged
     --> task/control-plane-agent/src/mgs_gimlet.rs:1448:14
     |
1448 |     unsafe { &mut UART_RX_BUF }
     |              ^^^^^^^^^^^^^^^^ mutable reference to mutable static
     |
     = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
     = note: this will be a hard error in the 2024 edition
     = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior
help: use `addr_of_mut!` instead to create a raw pointer
     |
1448 |     unsafe { addr_of_mut!(UART_RX_BUF) }
     |              ~~~~~~~~~~~~~~~~~~~~~~~~~

warning: creating a mutable reference to mutable static is discouraged
     --> task/control-plane-agent/src/mgs_gimlet.rs:1464:14
     |
1464 |     unsafe { &mut INSTALLINATOR_IMAGE_ID_BUF }
     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable reference to mutable static
     |
     = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
     = note: this will be a hard error in the 2024 edition
     = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior
help: use `addr_of_mut!` instead to create a raw pointer
     |
1464 |     unsafe { addr_of_mut!(INSTALLINATOR_IMAGE_ID_BUF) }
     |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

warning: `task-control-plane-agent` (bin "task-control-plane-agent") generated 4 warnings

These warnings suggest that we use addr_of_mut instead, but rather
than doing that, I've wrapped these buffers in UnsafeCells as part of
a new wrapper type for resource that can only be claimed once,
unimaginatively named ClaimOnce.

UnsafeCell expresses to the compiler that we intend to mutate this
data but allows us to store it in a regular (non mut) static
variable, which feels much less sketchy --- in general, the use of
static mut should be discouraged.

After making this change in control-plane-agent, I've also gone and
factored out the ClaimOnce type into the static-cell crate, so that it
can be used to remove one last use of static mut in host-sp-comms.

I haven't touched the static mut buffers in Hiffy, as I think that's a more
complex use-case that will require more thought.

cbiffle · 2024-04-11T20:24:29Z

task/attest/src/main.rs

@@ -93,9 +92,8 @@ struct AttestServer {

 impl Default for AttestServer {
    fn default() -> Self {


This isn't new in your change, but a Default impl that can only ever be called once seems sketchy.

yeah, i also noticed and didn't love that; I wasn't sure if it was worth changing as part of this branch or not, but i may go ahead and do it here.

cbiffle · 2024-04-11T20:28:52Z

task/thermal/src/control.rs

+
+        static ERR_BLACKBOX: ClaimOnceCell<ThermalSensorErrors> =
+            ClaimOnceCell::new(ThermalSensorErrors::new());
+        static PREV_ERR_BLACKBOX: ClaimOnceCell<ThermalSensorErrors> =


Nit: this increases the RAM usage by one word compared to the original. Shrug.

hm, they could be stuck in one ClaimOnceCell as a tuple or something, i guess...lemme do that.

In the most recent stack of commits this looks like the only one that didn't get put in a tuple 🤣

cbiffle

Looks generally good, the things I notice here aren't things you did.

Currently, the `control-plane-agent` task uses `static mut`s to declare static buffers. These are guarded by an `AtomicBool` that ensures that only a single mutable reference to these buffers is ever created. However, they are still declared as `static mut`s and referenced by creating an `&mut` reference to those `static mut`s, which now generates a warning as it may be unsound in future Rust editions: <details> <summary>Compiler warnings:</summary> ``` warning: creating a mutable reference to mutable static is discouraged --> task/control-plane-agent/src/mgs_gimlet/host_phase2.rs:334:14 | 334 | unsafe { &mut PHASE2_BUF } | ^^^^^^^^^^^^^^^ mutable reference to mutable static | = note: for more information, see issue #114447 <rust-lang/rust#114447> = note: this will be a hard error in the 2024 edition = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior = note: `#[warn(static_mut_refs)]` on by default help: use `addr_of_mut!` instead to create a raw pointer | 334 | unsafe { addr_of_mut!(PHASE2_BUF) } | ~~~~~~~~~~~~~~~~~~~~~~~~ warning: creating a mutable reference to mutable static is discouraged --> task/control-plane-agent/src/mgs_gimlet.rs:1431:14 | 1431 | unsafe { &mut UART_TX_BUF } | ^^^^^^^^^^^^^^^^ mutable reference to mutable static | = note: for more information, see issue #114447 <rust-lang/rust#114447> = note: this will be a hard error in the 2024 edition = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior help: use `addr_of_mut!` instead to create a raw pointer | 1431 | unsafe { addr_of_mut!(UART_TX_BUF) } | ~~~~~~~~~~~~~~~~~~~~~~~~~ warning: creating a mutable reference to mutable static is discouraged --> task/control-plane-agent/src/mgs_gimlet.rs:1448:14 | 1448 | unsafe { &mut UART_RX_BUF } | ^^^^^^^^^^^^^^^^ mutable reference to mutable static | = note: for more information, see issue #114447 <rust-lang/rust#114447> = note: this will be a hard error in the 2024 edition = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior help: use `addr_of_mut!` instead to create a raw pointer | 1448 | unsafe { addr_of_mut!(UART_RX_BUF) } | ~~~~~~~~~~~~~~~~~~~~~~~~~ warning: creating a mutable reference to mutable static is discouraged --> task/control-plane-agent/src/mgs_gimlet.rs:1464:14 | 1464 | unsafe { &mut INSTALLINATOR_IMAGE_ID_BUF } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable reference to mutable static | = note: for more information, see issue #114447 <rust-lang/rust#114447> = note: this will be a hard error in the 2024 edition = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior help: use `addr_of_mut!` instead to create a raw pointer | 1464 | unsafe { addr_of_mut!(INSTALLINATOR_IMAGE_ID_BUF) } | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ warning: `task-control-plane-agent` (bin "task-control-plane-agent") generated 4 warnings ``` </details> These warnings suggest that we use `addr_of_mut` instead, but rather than doing that, I've wrapped these buffers in `UnsafeCell`s as part of a new wrapper type for resource that can only be claimed once, unimaginatively named `ClaimOnce`. `UnsafeCell` expresses to the compiler that we intend to mutate this data but allows us to store it in a regular (non `mut`) `static` variable, which feels much less sketchy --- in general, the use of `static mut` should be discouraged. We could, alternatively, use `StaticCell` for this, but this solution is simpler: since the claim is never *released*, the `ClaimOnce::claim` method can return an `&'static mut T` rather than a guard type (as in `StaticCell`'s `StaticRef<'a, T>`). This means we don't need to change any of the types in `control-plane-agent` which reference these buffers as `&'static mut T`s to use that guard type.

There's also a `static mut` used for a claim-once buffer in `host-sp-comms`. This commit factors out the `ClaimOnce` type from `control-plane-agent` and moves it to the `static-cell` crate, so that we can re-use it in `host-sp-comms`. I also renamed it to `ClaimOnceCell` for consistency.

This commit refactors existing uses of the `mutable_statics!` macro for all singleton `static`s that can be const initialized. Because `ClaimOnceCell` constructs the initial value in const-eval at compile time, rather than by initializing a `MaybeUninit` at runtime, this means that we never need to reserve stack space for the initializer. Also, because we don't need to generate code for initializing these values at runtime, this might scrape a few bytes off binary size.

In `task-net`, using const initialization actually makes things bigger instead of smaller, because `smoltcp::SocketStorage::EMPTY` is apparently not actually all zeroes? Thus, the socket array has to actually live in the binary, instead of being initialized one-by-one at runtime. "Something something Kolmogorov complexity", I guess. Anyway, this puts it back the way it was before.

hawkw · 2024-04-11T22:03:25Z

final size diff for app/sidecar/rev-d.toml:

Comparing against the previously saved sizes
Checking for differences in -caboose-
Checking for differences in auxflash
Checking for differences in control_plane_agent
	flash: -72
	ram: -4
Checking for differences in dump_agent
	flash: -88
	ram: 4
Checking for differences in ecp5_front_io
Checking for differences in ecp5_mainboard
Checking for differences in hiffy
Checking for differences in i2c_driver
Checking for differences in idle
Checking for differences in ignition
Checking for differences in jefe
Checking for differences in kernel
Checking for differences in monorail
Checking for differences in net
	flash: 2148
	ram: 4
Checking for differences in packrat
	flash: -36
Checking for differences in power
Checking for differences in sensor
Checking for differences in sequencer
Checking for differences in sprot
	flash: -40
	ram: 4
Checking for differences in sys
Checking for differences in thermal
	flash: -56
	ram: 8
Checking for differences in transceivers
	flash: -52
	ram: 4
Checking for differences in udpbroadcast
Checking for differences in udpecho
Checking for differences in update_server
Checking for differences in validate
Checking for differences in vpd

and for app/gimlet/rev-f.toml:

Comparing against the previously saved sizes
Checking for differences in -caboose-
Checking for differences in control_plane_agent
	flash: -240
Checking for differences in dump_agent
	flash: -88
	ram: 4
Checking for differences in gimlet_inspector
Checking for differences in gimlet_seq
Checking for differences in hash_driver
Checking for differences in hf
Checking for differences in hiffy
Checking for differences in host_sp_comms
	flash: -128
Checking for differences in i2c_driver
Checking for differences in idle
Checking for differences in jefe
Checking for differences in kernel
Checking for differences in net
Checking for differences in packrat
	flash: -52
Checking for differences in power
Checking for differences in sbrmi
Checking for differences in sensor
Checking for differences in spd
Checking for differences in spi2_driver
Checking for differences in sprot
	flash: -40
	ram: 4
Checking for differences in sys
Checking for differences in thermal
	flash: -68
	ram: 8
Checking for differences in udpbroadcast
Checking for differences in udpecho
Checking for differences in update_server
Checking for differences in user_leds
Checking for differences in validate
Checking for differences in vpd

Clippy (quite rightly!) [complains] about code which "[c]reat[es] a mutable reference which can be repeatedly derived from an immutable reference", as this allows mutable aliasing. But, what Clippy *doesn't* realize, because it's just a dumb machine that can only look at the function's signature and apply simple rules to it, is that the entire point of this thing is that the mutable reference can only be ever created once. So, let's silence Clippy's criticism here. [complains]: https://rust-lang.github.io/rust-clippy/master/index.html#/mut_from_ref