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

esp-storage: Fix incorrect usage of MaybeUninit #1902

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

DBLouis
Copy link

@DBLouis DBLouis commented Aug 3, 2024

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

I found incorrect usage of MaybeUninit while browsing the source that might cause UB. Specifically, calling assume_init_mut on a MaybeUninit that has not been initialized ref.

Testing

WIP

@DBLouis
Copy link
Author

DBLouis commented Aug 3, 2024

I am not sure how to run the tests, can someone help please

@Dominaezzz
Copy link
Collaborator

You need to add an entry to the changelog for CI to continue.

@DBLouis
Copy link
Author

DBLouis commented Aug 3, 2024

You need to add an entry to the changelog for CI to continue.

Do I need to bump the version then?

@Dominaezzz
Copy link
Collaborator

No, just add an entry for your PR

@DBLouis DBLouis force-pushed the maybeuninit-ub branch 2 times, most recently from e69eb4e to 5973d8e Compare August 3, 2024 16:59
@DBLouis
Copy link
Author

DBLouis commented Aug 3, 2024

I just renamed the commit

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, thanks for the PR. This looks fine to me but I would like either @bjoernQ or @MabezDev to take a look as well.

#[repr(C)]
pub union FlashBuffer<const N: usize, const M: usize> {
bytes: [MaybeUninit<u8>; M],
words: [MaybeUninit<u32>; N],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something but is the words field ever really used? (Yes, by the accessors but those seem to be unused?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do end up sticking with the union, we also need to make sure its #[repr(C, align(4))]. I checked on the rust playground, and it does currently have an align of 4 anyways, but I think that it's best to be explicit

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think specifying the alignment is needed according to this: https://doc.rust-lang.org/reference/type-layout.html#reprc-unions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something but is the words field ever really used? (Yes, by the accessors but those seem to be unused?)

I have a following PR coming that uses those. I split in two to make more focused PR.

esp-storage/src/buffer.rs Outdated Show resolved Hide resolved
@ProfFan
Copy link
Contributor

ProfFan commented Sep 4, 2024

Wonder what's the status on this? Just checking in :)

@DBLouis
Copy link
Author

DBLouis commented Sep 5, 2024

Wonder what's the status on this? Just checking in :)

I've been busy, I finish this soon 🙂

@bjoernQ bjoernQ marked this pull request as draft September 12, 2024 06:23
@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 12, 2024

Converted to draft for now - set it to Read-for-Review when it's updated

@jessebraham
Copy link
Member

Just checking in here, @DBLouis would you like one of us to take over here to wrap this up? Or do you expect to be able to get back to this in the next couple weeks? No worries either way, would just like to get this across the finish line :)

@DBLouis
Copy link
Author

DBLouis commented Oct 23, 2024

Just checking in here, @DBLouis would you like one of us to take over here to wrap this up? Or do you expect to be able to get back to this in the next couple weeks? No worries either way, would just like to get this across the finish line :)

Yes it is probably better that someone takes over, I will not have time soon, life got in the way :)

@jessebraham jessebraham added the status:needs-attention This should be prioritized label Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:needs-attention This should be prioritized
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants