Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seal the DAC trait to prevent UB #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jamesmunns
Copy link
Contributor

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
Copy link
Contributor Author

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

@jamesmunns
Copy link
Contributor Author

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
Copy link
Member

@jamesmunns Any followup planned?

@jamesmunns
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants