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

esp-storage: Fix incorrect usage of MaybeUninit

Open DBLouis opened this issue 1 year ago • 10 comments
trafficstars

Submission Checklist 📝

  • [x] I have updated existing examples or added new ones (if applicable).
  • [x] I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • [x] My changes were added to the CHANGELOG.md in the proper section.
  • [x] 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 avatar Aug 03 '24 12:08 DBLouis

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

DBLouis avatar Aug 03 '24 13:08 DBLouis

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

Dominaezzz avatar Aug 03 '24 14:08 Dominaezzz

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

Do I need to bump the version then?

DBLouis avatar Aug 03 '24 15:08 DBLouis

No, just add an entry for your PR

Dominaezzz avatar Aug 03 '24 15:08 Dominaezzz

I just renamed the commit

DBLouis avatar Aug 03 '24 17:08 DBLouis

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

ProfFan avatar Sep 04 '24 14:09 ProfFan

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

I've been busy, I finish this soon 🙂

DBLouis avatar Sep 05 '24 06:09 DBLouis

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

bjoernQ avatar Sep 12 '24 06:09 bjoernQ

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 :)

jessebraham avatar Oct 21 '24 12:10 jessebraham

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 :)

DBLouis avatar Oct 23 '24 14:10 DBLouis

I think the easier solution to get the perf required and to avoid unsafe is to store the flash buffer inside FlashStorage itself? This way it's only initialized once, and this will probably reduce stack usage too.

@bjoernQ are you aware of any reason not to do it like this instead?

MabezDev avatar Nov 14 '24 11:11 MabezDev

I think the easier solution to get the perf required and to avoid unsafe is to store the flash buffer inside FlashStorage itself? This way it's only initialized once, and this will probably reduce stack usage too.

@bjoernQ are you aware of any reason not to do it like this instead?

I think so, yes

bjoernQ avatar Nov 15 '24 07:11 bjoernQ

Thanks for making us aware of this, I've opened https://github.com/esp-rs/esp-hal/issues/3009 so we don't forget about this.

I'm closing this PR now, please feel free to open a new one if you want to tackle this issue again.

MabezDev avatar Jan 21 '25 14:01 MabezDev