Skip to content

dhcpv4: fix do_page_fault() when interface lost carrier#372

Closed
updatede wants to merge 2 commits intoopenwrt:masterfrom
updatede:patch-1
Closed

dhcpv4: fix do_page_fault() when interface lost carrier#372
updatede wants to merge 2 commits intoopenwrt:masterfrom
updatede:patch-1

Conversation

@updatede
Copy link

This fix issue #371 . According to avl.h, av_delete can’t be called in remove all elements loop.

This fix issue openwrt#371 . According to avl.h, av_delete can’t be called in remove all elements loop. 

Signed-off-by: updatede <updatede@gmail.com>
@systemcrash
Copy link
Contributor

ping @Alphix and @Noltari. WDYT?

@Noltari
Copy link
Member

Noltari commented Jan 19, 2026

@updatede please fix the commit title
As reported at https://github.com/openwrt/odhcpd/actions/runs/21016348202/job/60422128645?pr=372, it should be something like:
dhcpv4: fix do_page_fault() when interface lost carrier

@updatede updatede changed the title Fix do_page_fault() when interface lost carrier dhcpv4: fix do_page_fault() when interface lost carrier Jan 19, 2026
@systemcrash
Copy link
Contributor

@updatede commit title, not PR title (needs force push) :)

@systemcrash
Copy link
Contributor

merge commits are not accepted... just rebase on master if you must

@updatede updatede closed this Jan 20, 2026
@updatede updatede deleted the patch-1 branch January 20, 2026 01:10
@Alphix
Copy link
Contributor

Alphix commented Jan 20, 2026

Hm, I could just do a quick check on my phone (not very ergonomic), I assume you want to fix the issue with:

bool dhcpv4_setup_interface()
   ...
   avl_remove_all_elements(...)
       dhcpv4_free_lease(...)

But dhcpv4_free_lease() is called from a bunch of other places, have you checked all callers? It seems likely that we'd end up free'ing but not removing leases in some cases. Wouldn't changing avl_remove_all_elements() to avl_for_each_element_safe() be a better option?

@updatede
Copy link
Author

no, I haven’t check any other place where free something twice with av_delete, maybe triple , quadruple :) no I haven’t check it. Feel free to provide better solution.

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.

4 participants