On Thu, Aug 29, 2024 at 02:47:24PM -0500, Bill O'Donnell wrote:
> 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.
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.
>
>
> >
> >
> > > 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) {
> >
>
