stm32-rs / stm32f0xx-hal

A Rust `embedded-hal` implementation for all MCUs in the STM32 F0 family
BSD Zero Clause License
134 stars 59 forks source link

Seal the DAC trait to prevent UB #94

Open jamesmunns opened 4 years ago

jamesmunns commented 4 years ago

Before this change, the following user code would compile and run without warnings, and would return an uninitialized [u8; 16], which would be UB:

struct UB;
use crate::stm32::DAC;
impl Pins<DAC> for UB {
    type Output = [u8; 16];
}

// ...

let mut rcc = dp.RCC.configure().sysclk(8.mhz()).freeze(&mut dp.FLASH);
let gpioa = dp.GPIOA.split(&mut rcc);
let mut uninit = dac(dp.DAC, UB, &mut rcc);

This change seals the trait and marks it unsafe to prevent outside malicious use, as well as warn maintainers.

jamesmunns commented 4 years ago

Oh, this also removes the deprecation warning around the use of mem::uninitialized().

jamesmunns commented 4 years ago

After much better advice from @jonas-schievink, we might want to just get rid of MaybeUninit in general. From chat:

jschievink: jamesmunns: why not redesign that trait/fn to not require uninitialized?
jamesmunns: I really don't know how to return an arbitrary associated type
I suppose you could also impl the trait in the macro instead, so you have multiple impls with concrete types instead of one blanket impl
I think the blanket impl strays into GAT territory, but I'm not sure
jschievink:
why not give the assoc type a new trait bound like : Channels that is sealed and has a new method or something like that?
unsafe method, that is
therealprof commented 4 years ago

@jamesmunns Any followup planned?

jamesmunns commented 4 years ago

None yet! I'll re add this to my to-do list!