On 8/29/24 3:34 PM, Bill O'Donnell wrote:
> On Thu, Aug 29, 2024 at 02:47:24PM -0500, Bill O'Donnell wrote:
...
>>>> + free(list_cpy);
>>>
>>> and then this would double-free that same memory address.
>>
>> I see that now. This code is indeed difficult to grok.
>>
>> Perhaps (if this a legitimate finding of use after free), instead of having the memory
>> freed in invidx_commit(), it should instead be freed once in fstab_commit() after the iterations
>> of the for-loops in that function. I'll have a look at that possibility.
>
> i.e., Removing what Coverity tags as the culprit (node_free(list_del(dst_n)) from
> invidx_commit(), and adding free(list) following the for-loop iteration in fstab_commit() may be
> a better solution.
I don't think that's the right approach.
invidx_commit() has this while loop, which is where coverity thinks the passed-in "list"
might get freed, before the caller uses it again:
/* Clean up the mess we just created */
/* find node for dst_fileidx */
dst_n = find_invidx_node(list, dst_fileidx);
tmp_parent = ((data_t *)(dst_n->data))->parent;
while(dst_n != NULL) {
node_t *tmp_n1;
dst_d = dst_n->data;
/* close affected invidx file and stobj files */
for(i = 0; i < dst_d->nbr_children; i++) {
close_stobj_file(((data_t *)(dst_d->children[i]->data))->file_idx, BOOL_FALSE);
}
/* free_all_children on that node */
free_all_children(dst_n);
tmp_n1 = find_invidx_node(dst_n->next, dst_fileidx);
node_free(list_del(dst_n));
dst_n = tmp_n1;
}
"list" is presumably the head of a list of items, and this is cleaning up / freeing items
within that list. Coverity seems to think that the while loop can end up getting back to
the head and freeing it, which the caller then uses again in a loop.
My guess is that coverity is wrong, but I don't think you're going to be able to prove that
(or fix this) without at least getting a sense of what this code is actually doing, and
how this list is shaped and managed...
-Eric
