On Thu, Aug 29, 2024 at 04:56:01PM -0500, Eric Sandeen wrote:
> 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...
That's my take on it as well. I'm leaning towards a false positive. I'll have another look.
Thanks for reviewing.
-Bill
