stm32f0xx-hal icon indicating copy to clipboard operation
stm32f0xx-hal copied to clipboard

Seal the DAC trait to prevent UB

Open jamesmunns opened this issue 5 years ago • 4 comments

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 avatar Feb 04 '20 00:02 jamesmunns

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

jamesmunns avatar Feb 04 '20 00:02 jamesmunns

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

jamesmunns avatar Feb 04 '20 00:02 jamesmunns

@jamesmunns Any followup planned?

therealprof avatar May 02 '20 18:05 therealprof

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

jamesmunns avatar May 02 '20 18:05 jamesmunns