search

flipperzero-rs / flipperzero

Rust on the Flipper Zero
MIT License
517 stars 34 forks source link

Use ptr::NonNull instead of raw pointers #67

Closed kitterion closed 1 year ago

kitterion commented 1 year ago

This allows rust's null pointer optimization to kick in. For example, with this change the size of Option<FuriString> becomes 8 -> 4. As far as I can tell, flipperzero's malloc doesn't ever return null but crashes the app instead. See https://github.com/flipperdevices/flipperzero-firmware/blob/0.82.3/furi/core/memmgr_heap.c#L484 That's the function that malloc calls directly, it has the only return statement at the very end, and furi_check just crashes the app if the condition is false, i.e. the pointer is null.

I think I caught all the structs, I'm happy to add more if I didn't.

dcoles commented 1 year ago

Thanks for the PR. Yes, from what I've seen malloc will never fail and the memory is always zeroed regardless of whether you use malloc or calloc.

dcoles commented 1 year ago

Post-hoc utACK.

I'm not familiar with utACK. Is that "untested approval"?

I also took a look through each of the functions to verify that it either calls furi_check to ensure the values are non-null or called malloc (which internally calls furi_check as well as zeros out the allocated memory)

I think NonNull is the right choice for these structures, since they can't possibly work with a null value (and the furi_checks allow us to bypass the need for using the checked NonNull::New).

(I think you are agreeing, it's just not clear given the unresolved comments / unapproved review.)

str4d commented 1 year ago

I'm not familiar with utACK. Is that "untested approval"?

Yep. Just an indication that it was a visual review only, and I did not e.g. run the code myself to check how it behaves.

I think you are agreeing,

Yes, I am. I just wanted to check myself that the changes made sense (because NonNull has more stringent requirements), and leave a record of why I was satisfied.

it's just not clear given the unresolved comments / unapproved review.

After a PR is merged, GitHub doesn't let anyone submit either "Approved" or "Changes Requested" PR reviews, so the above was submitted as a "Comments" review. But it would have been "Approved", hence (ut)ACK.