`rtt_init*` macros are unsound

[japaric]

rtt_init!

The rtt_init! macro lets you effectively alias a mutable reference to memory. Example below:

#[entry]
fn main() -> ! {
    let c0 = alias();

    // use `c0`
}

#[exception]
fn SysTick() {
    let c0 = alias();
    // use `c0`
}

fn alias() -> UpChannel {
    let cs = rtt_init! {
        up: {
            0: {
                size: 1024
                mode: NoBlockSkip
            }
        }
    };
    cs.up.0
}

Both c0 variables have mutable access to the same chunk of memory and each variable can be accessed from a different priority level (SysTick can preempt main): this alone is a data race and UB in paper (in practice, you may need to call write to make the code explode)

Suggested fix: either make rtt_init! unsafe to call OR make rtt_init! return Some only once. The latter is the owned singleton pattern (described in its zero-sized reference form in this blog post). Basically you want something like this:

fn once() -> Option<Something> {
    static ONCE: AtomicBool = AtomicBool::new(false);
    
    if ONCE
        .compare_exchange(false, true, Ordering::Relaxed, Ordering::Relaxed)
        .is_ok() {
        // this branch will run at most *once*
        // ..
        Some(something)
    } else {
        None
    }
}

rtt_init_default!

This is unsound for the same reason rtt_init! is unsound: you should not be able to alias Channels in safe code.

Fix: fixing rtt_init! fixes this -- just don't make rtt_init unsafe to call but rtt_init_default safe to call; both should be unsafe OR both should return an Option

rtt_init_print!

This one also lets you run into data races in safe code. See below:

#[entry]
fn main() -> ! {
    init();

    loop {
        rprintln!("Hello, world!");
    }
}

#[exception]
fn SysTick() {
    init();
}

fn init() {
    rtt_init_print!();
}

Here the exception (SysTick) can preempt rprintln! and re-initialize the channel (overwrite static variables) used by rprintln!. The main issue here is the unsafe code in rtt_init! running more than once and/or running in parallel / concurrently to the execution of rprintln!.

Suggested fix: this macro should panic if called more than once OR should be no-op if called more than once

cc @yatekii

[Yatekii]

Great catch, thanks for reporting :)

Should be easy enough to fix thanks to your suggestions.

[mvirkkunen]

This one is a bit annoying to fix due to the fact we can't rely on the availability of atomics on all platforms. I suppose I could live with atomics by default and a platform feature flag (is it just me or does like every crate have a "cortex-m" flag for instance?) like "no-atomics" that disables atomics and makes everything unsafe instead.

I'd rather not have a requirement for everybody to use "unsafe".

[Yatekii]

Yeah, this won't work on M0 targets indeed. But I think having a sane default is a good trade-off with a flag to disable it :)

[xobs]

As an update, Rust now has #[cfg(target_has_atomic = 32)] which can be used to omit such code on those platforms.

[tgross35]

Somewhat tangential, but using the OnceLock pattern would probably also allow panic-rtt-target to work without first needing to call rtt_init_default, which would be convenient.