Re: [PATCH] xfsdump: prevent use-after-free in fstab_commit()

[Bill O'Donnell <bodonnel@redhat.com>]

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