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@xxxxxxxxxx>
> > >
> > > Add proper exit error paths to avoid checking all pointers at the current path
> > >
> > > Fixes-coverity-id: 1512651
> > >
> > > Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> > > ---
> > > 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
