mirror of
https://git.haproxy.org/git/haproxy.git/
synced 2025-08-06 23:27:04 +02:00
BUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free() v2
ckch_inst_free() unlink the ckch_inst_link structure but never free it. It can't be fixed simply because cli_io_handler_commit_cafile_crlfile() is using a cafile_entry list to iterate a list of ckch_inst entries to free. So both cli_io_handler_commit_cafile_crlfile() and ckch_inst_free() would modify the list at the same time. In order to let the caller manipulate the ckch_inst_link, ckch_inst_free() now checks if the element is still attached before trying to detach and free it. For this trick to work, the caller need to do a LIST_DEL_INIT() during the iteration over the ckch_inst_link. list_for_each_entry was also replace by a while (!LIST_ISEMPTY()) on the head list in cli_io_handler_commit_cafile_crlfile() so the iteration works correctly, because it could have been stuck on the first detached element. list_for_each_entry_safe() is not enough to fix the issue since multiple element could have been removed. Must be backported as far as 2.5.
This commit is contained in:
parent
bccbad2654
commit
e0fa91ffe1
@ -986,8 +986,15 @@ void ckch_inst_free(struct ckch_inst *inst)
|
||||
LIST_DELETE(&inst->by_ckchs);
|
||||
LIST_DELETE(&inst->by_crtlist_entry);
|
||||
|
||||
/* Free the cafile_link_refs list */
|
||||
list_for_each_entry_safe(link_ref, link_ref_s, &inst->cafile_link_refs, list) {
|
||||
LIST_DELETE(&link_ref->link->list);
|
||||
if (link_ref->link && LIST_INLIST(&link_ref->link->list)) {
|
||||
/* Try to detach and free the ckch_inst_link only if it
|
||||
* was attached, this way it can be used to loop from
|
||||
* the caller */
|
||||
LIST_DEL_INIT(&link_ref->link->list);
|
||||
ha_free(&link_ref->link);
|
||||
}
|
||||
LIST_DELETE(&link_ref->list);
|
||||
free(link_ref);
|
||||
}
|
||||
@ -2908,11 +2915,17 @@ static int cli_io_handler_commit_cafile_crlfile(struct appctx *appctx)
|
||||
__ssl_sock_load_new_ckch_instance(ckchi_link->ckch_inst);
|
||||
}
|
||||
|
||||
/* delete the old sni_ctx, the old ckch_insts and the ckch_store */
|
||||
list_for_each_entry(ckchi_link, &old_cafile_entry->ckch_inst_link, list) {
|
||||
__ckch_inst_free_locked(ckchi_link->ckch_inst);
|
||||
}
|
||||
/* delete the old sni_ctx, the old ckch_insts
|
||||
* and the ckch_store. ckch_inst_free() also
|
||||
* manipulates the list so it's cleaner to loop
|
||||
* until it's empty */
|
||||
while (!LIST_ISEMPTY(&old_cafile_entry->ckch_inst_link)) {
|
||||
ckchi_link = LIST_ELEM(old_cafile_entry->ckch_inst_link.n, typeof(ckchi_link), list);
|
||||
|
||||
LIST_DEL_INIT(&ckchi_link->list); /* must reinit because ckch_inst checks the list */
|
||||
__ckch_inst_free_locked(ckchi_link->ckch_inst);
|
||||
free(ckchi_link);
|
||||
}
|
||||
|
||||
/* Remove the old cafile entry from the tree */
|
||||
ebmb_delete(&old_cafile_entry->node);
|
||||
|
Loading…
Reference in New Issue
Block a user