Possible unsoundness on architectures without atomics

[ROMemories]

My understanding is that log relies on atomics to implement some of its functionality, especially set_logger() and set_max_level().

It seems there is an internal "fake" AtomicUsize on platforms that do not provide atomics, with the assumption that no such architectures are multicore:

log/src/lib.rs

Lines 451 to 454 in 5359483

	// Any platform without atomics is unlikely to have multiple cores, so
	// writing via Cell will not be a race condition.
	#[cfg(not(target_has_atomic = "ptr"))]
	unsafe impl Sync for AtomicUsize {}

However this assumption does not hold on the RP2040 MCU, which features two Cortex-M0+ (ARMv6-M) cores.

I am not sure this actually currently results in unsoundness because it is not yet clear to me whether the instances of AtomicUsize are actually used in ways that could result in race conditions on these architectures.

I suppose leveraging portable-atomic on architectures that do not provide atomics could help fix this, if this is an acceptable dependency for log, which would only be required on such architectures.

---

For context, we at Ariel OS are building an embedded OS which, among many other things, abstracts over log and makes it easily usable on many processor architectures, making this easy to test.

[Thomasdezeeuw]

Note that set_logger has the cfg(target_has_atomic = "ptr") attribute, i.e. it shouldn't be available if the atomic are not present

log/src/lib.rs

Lines 1402 to 1405 in 5359483

	#[cfg(target_has_atomic = "ptr")]
	pub fn set_logger(logger: &'static dyn Log) -> Result<(), SetLoggerError> {
	set_logger_inner(|| logger)
	}

In that case the unsafe set_logger_racy has to be used:

log/src/lib.rs

Lines 1454 to 1467 in 5359483

	pub unsafe fn set_logger_racy(logger: &'static dyn Log) -> Result<(), SetLoggerError> {
	match STATE.load(Ordering::Acquire) {
	UNINITIALIZED => {
	LOGGER = logger;
	STATE.store(INITIALIZED, Ordering::Release);
	Ok(())
	}
	INITIALIZING => {
	// This is just plain UB, since we were racing another initialization function
	unreachable!("set_logger_racy must not be used with other initialization functions")
	}
	_ => Err(SetLoggerError(())),
	}
	}

That doesn't make things any safer, but it means that log isn't unsound.

[ROMemories]

Pondering on this some more, it seems to be me that the platform being multicore or not is actually irrelevant, it just has to support multi-threading for this to be an issue.

In that case the unsafe set_logger_racy has to be used

I am aware of this function, but then the logger() getter function actually relies on the happens-before relationship between LOGGER and STATE:

log/src/lib.rs

Lines 1506 to 1521 in 5359483

	pub fn logger() -> &'static dyn Log {
	// Acquire memory ordering guarantees that current thread would see any
	// memory writes that happened before store of the value
	// into `STATE` with memory ordering `Release` or stronger.
	//
	// Since the value `INITIALIZED` is written only after `LOGGER` was
	// initialized, observing it after `Acquire` load here makes both
	// write to the `LOGGER` static and initialization of the logger
	// internal state synchronized with current thread.
	if STATE.load(Ordering::Acquire) != INITIALIZED {
	static NOP: NopLogger = NopLogger;
	&NOP
	} else {
	unsafe { LOGGER }
	}
	}

However, on architectures without atomics, STATE really is not an atomic, and so this happens-before relationship is not guaranteed.

[Thomasdezeeuw]

However, on architectures without atomics, STATE really is not an atomic, and so this happens-before relationship is not guaranteed.

Agreed. But then you'll most likely also not have threads. Personally I don't see how threads can do any cross-thread communication safely without atomics.

[ROMemories]

But then you'll most likely also not have threads.

These MCUs—the RP2040 and the ESP32-C3 (RISC-V without the (A) extension) for instance—do support threading however, and we do support it in Ariel OS (even multicore).

Personally I don't see how threads can do any cross-thread communication safely without atomics.

This is done through critical sections on bare metal, which involve temporarily disabling interrupts (see the critical-section crate and the implementation in the cortex-m crate for instance). These can be used to emulate atomics on architectures that do not support them, which is what portable-atomic does (when needed).

[Thomasdezeeuw]

This is done through critical sections on bare metal, which involve temporarily disabling interrupts (see the critical-section crate and the implementation in the cortex-m crate for instance). These can be used to emulate atomics on architectures that do not support them, which is what portable-atomic does (when needed).

Interesting, I didn't know about that -- I don't do a lot of embedded work myself.

Would calling set_logger_racy in such a critical section be sufficient? (it's something to docs already call out: https://docs.rs/log/latest/log/fn.set_logger_racy.html#safety)

[ROMemories]

(it's something to docs already call out: https://docs.rs/log/latest/log/fn.set_logger_racy.html#safety)

The docs mention that:

It is safe to use other logging functions while this function runs (including all logging macros).

So I think this is not exactly complete, as it would not be safe to call logger() while set_logger_racy() runs.

Would calling set_logger_racy in such a critical section be sufficient?

Wrapping these calls in a critical section should indeed enforce that.