search

open-power / petitboot

GNU General Public License v2.0
212 stars 56 forks source link

discover: Fix use-after-free error in DHCP handling #93

Closed rarbab closed 2 years ago

rarbab commented 2 years ago

Fixes open-power/petitboot#92.

jk-ozlabs commented 2 years ago

Thanks for the patch! This looks good to me, but I think we need some additional cleanup too, if we're switching from a talloc_steal to a talloc_reference.

The original allocate/free path in user_event_handle_message originally had some handling for user_event_dhcp performing the steal (see a50d5fe279db71cf85fabeb675c99b167ec63dcb, subsequently modified by 9e869ebe3a5127575105d82c4d289d95cbed2db9).

Switching to the reference approach means that the talloc_free(event) should no longer be conditional, otherwise we may have a leak there.

[it looks like user_event_url doesn't have the same semantics, despite the comment in user_event_handle_message...]

rarbab commented 2 years ago

Sure, that makes sense. I've modified the patch so it also removes the conditional from talloc_free(event). 9e869eb says that was originally added to make scan-build happy, and there is no difference in the scan-build results before and after my change, so that seems good.

Also added a superfluous patch to correct that comment in user_event_handle_message() beforehand because I think it makes more clear that it's okay to remove the if clause.

jk-ozlabs commented 2 years ago

Super, LGTM. I'll do some testing here and get this merged soon.

jk-ozlabs commented 2 years ago

Merged, thank you for the contribution!