On Thu, Aug 29, 2024 at 02:35:27PM -0500, Eric Sandeen wrote:
> On 8/29/24 12:59 PM, Bill O'Donnell wrote:
> > Fix potential use-after-free of list pointer in fstab_commit().
> > Use a copy of the pointer when calling invidx_commit().
>
> I'm not sure how (or even if) the use after free happens -xfsdump is so hard
> to read - but ...
>
> > Coverity CID 1498039.
> >
> > Signed-off-by: Bill O'Donnell <bodonnel@xxxxxxxxxx>
> > ---
> > invutil/fstab.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/invutil/fstab.c b/invutil/fstab.c
> > index 88d849e..fe2b1f9 100644
> > --- a/invutil/fstab.c
> > +++ b/invutil/fstab.c
> > @@ -66,6 +66,7 @@ fstab_commit(WINDOW *win, node_t *current, node_t *list)
> > data_t *d;
> > invt_fstab_t *fstabentry;
> > int fstabentry_idx;
> > + node_t *list_cpy = list;
> >
> > n = current;
> > if(n == NULL || n->data == NULL)
> > @@ -77,8 +78,10 @@ fstab_commit(WINDOW *win, node_t *current, node_t *list)
> >
> > if(d->deleted == BOOL_TRUE && d->imported == BOOL_FALSE) {
> > for(i = 0; i < d->nbr_children; i++) {
> > - invidx_commit(win, d->children[i], list);
> > + list_cpy = list;
>
> this copies the memory address stored in "list" into your new pointer var "list_cpy"
>
> > + invidx_commit(win, d->children[i], list_cpy);
>
> If invidx_commit() frees the 2nd argument, it frees the memory address pointed
> to by both list and list_cpy.
>
> Storing the same memory address in 2 pointer variables does not prevent that memory
> from being freed.
>
> > }
> > + 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.
Thanks-
Bill
>
>
> > mark_all_children_commited(current);
> >
> > fstabentry_idx = (int)(((long)fstabentry - (long)fstab_file[fidx].mapaddr - sizeof(invt_counter_t)) / sizeof(invt_fstab_t));
> > @@ -101,8 +104,10 @@ fstab_commit(WINDOW *win, node_t *current, node_t *list)
> > invt_fstab_t *dest;
> >
> > for(i = 0; i < d->nbr_children; i++) {
> > - invidx_commit(win, d->children[i], list);
> > + list_cpy = list;
> > + invidx_commit(win, d->children[i], list_cpy);
> > }
> > + free(list_cpy);
> > mark_all_children_commited(current);
> >
> > if(find_matching_fstab(0, fstabentry) >= 0) {
>
