Re: [PATCH 1/2] xfs_repair: Fix check_refcount() error path

[Carlos Maiolino <cem@kernel.org>]

On Mon, Sep 05, 2022 at 09:05:28AM +0200, Carlos Maiolino wrote:
> On Fri, Sep 02, 2022 at 01:48:21PM -0700, Darrick J. Wong wrote:
> > On Fri, Sep 02, 2022 at 03:43:39PM +0200, Carlos Maiolino wrote:
> > > From: Carlos Maiolino <cmaiolino@redhat.com>
> > >
> > > Add proper exit error paths to avoid checking all pointers at the current path
> > >
> > > Fixes-coverity-id: 1512651
> > >
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > ---
> > >  repair/rmap.c |   23 +++++++++++------------
> > >  1 file changed, 11 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/repair/rmap.c b/repair/rmap.c
> > > index a7c4b25b1..0253c0c36 100644
> > > --- a/repair/rmap.c
> > > +++ b/repair/rmap.c
> > > @@ -1377,7 +1377,7 @@ check_refcounts(
> > >  	if (error) {
> > >  		do_warn(_("Could not read AGF %u to check refcount btree.\n"),
> > >  				agno);
> > > -		goto err;
> > > +		goto err_agf;
> > 
> > Shouldn't this       ^^^^^^^ be err_pag, since we're erroring out and
> > releasing the perag group reference?
> 
> At first I named it err_pag, but pag is used here only to read the agf, and when
> reading agf fail is why we end up reaching this error path, so I thought it
> would be more specific to name it err_agf.
> > 
> > Also ... don't the "if (XXX) free(XXX)" bits take care of all this?
> > 
> 
> Yeah, it does. But that's exactly what coverity is complaining about. We check
> for a NULL pointer 'after' we dereference it earlier, to be more specific:
> 
> ---
> Type: Dereference before NULL check
> Null-checking pag suggests that it may be null, but it has already been
> dereferenced on all paths leading to the check
> ---
> 
> Both patches fix the same issue type.
> 
> > (I can't access Coverity any more, so I don't know what's in the
> > report.)
> > 
> > --D
> > 
> > >  	}

Hi Darrick. Do you have any other opinion at this? Or should I consider it a
no-no and discard those patches?

Cheers.

-- 
Carlos Maiolino