Code review comment for ~paride/ubuntu/+source/samba:lp1892145-focal

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

It is somewhat puzzling to read the explanation that the destructor will make this a double free as you'd expect the destructor to also check the pointers validity right?
Like the removed
- if (gse_ctx->k5ctx) {
- krb5_free_context(gse_ctx->k5ctx);

Naively I'd expect the same check to prevent a double free in the dtor in TALLOC_FREE(gse_ctx);

Checking the destructor (same file gse_context_destructor) shows that it indeed checks for gse_ctx->k5ctx, but other than the pure free which the patch removes it does check and close potentially existing caches and keytabs.
And it does an explicit set to NULL after the free.
If the set to NULL is needed then the dtor will have run into the crash while trying to clean the rest - and the extra checks and cleanups on caches and keytabs seem right.

So TL;DR: this looks good but not to be a "fix double free" instead it seems to be more like "clean sub-elements before free and mark NULL afterwards". But at this point it becomes bike-shedding, my initial concerns are resolved and I think this is ok.

Changelog issues raised by Utkarsh are resolved as well by now.
The SRU test plan is slightly weak missing an explicit testcase, but that is for the SRU Team to discuss and the fix seems fine.

+1 and sponsoring

review: Approve

« Back to merge proposal