diff mbox series

xfs_db: fix inverted logic in error path

Message ID 20230411174644.GI360889@frogsfrogsfrogs (mailing list archive)
State Deferred, archived
Headers show
Series xfs_db: fix inverted logic in error path | expand

Commit Message

Darrick J. Wong April 11, 2023, 5:46 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

smatch complains proceeding into the if body if leaf is a null pointer.
This is backwards, so correct that.

check.c:3455 process_leaf_node_dir_v2_int() warn: variable dereferenced before check 'leaf'

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 db/check.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dave Chinner April 11, 2023, 11:59 p.m. UTC | #1 
On Tue, Apr 11, 2023 at 10:46:44AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> smatch complains proceeding into the if body if leaf is a null pointer.
> This is backwards, so correct that.
> 
> check.c:3455 process_leaf_node_dir_v2_int() warn: variable dereferenced before check 'leaf'
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  db/check.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/db/check.c b/db/check.c
> index 964756d0..d5f3d225 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -3452,7 +3452,7 @@ process_leaf_node_dir_v2_int(
>  				 id->ino, dabno, stale,
>  				 be16_to_cpu(leaf3->hdr.stale));
>  		error++;
> -	} else if (!leaf && stale != be16_to_cpu(leaf->hdr.stale)) {
> +	} else if (leaf && stale != be16_to_cpu(leaf->hdr.stale)) {
>  		if (!sflag || v)
>  			dbprintf(_("dir %lld block %d stale mismatch "
>  				 "%d/%d\n"),

Looks good. I'm surprised the compiler didn't warn about this
obviously broken code...

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Darrick J. Wong April 12, 2023, 9:43 p.m. UTC | #2 
On Wed, Apr 12, 2023 at 09:59:03AM +1000, Dave Chinner wrote:
> On Tue, Apr 11, 2023 at 10:46:44AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > smatch complains proceeding into the if body if leaf is a null pointer.
> > This is backwards, so correct that.
> > 
> > check.c:3455 process_leaf_node_dir_v2_int() warn: variable dereferenced before check 'leaf'
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  db/check.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/db/check.c b/db/check.c
> > index 964756d0..d5f3d225 100644
> > --- a/db/check.c
> > +++ b/db/check.c
> > @@ -3452,7 +3452,7 @@ process_leaf_node_dir_v2_int(
> >  				 id->ino, dabno, stale,
> >  				 be16_to_cpu(leaf3->hdr.stale));
> >  		error++;
> > -	} else if (!leaf && stale != be16_to_cpu(leaf->hdr.stale)) {
> > +	} else if (leaf && stale != be16_to_cpu(leaf->hdr.stale)) {
> >  		if (!sflag || v)
> >  			dbprintf(_("dir %lld block %d stale mismatch "
> >  				 "%d/%d\n"),
> 
> Looks good. I'm surprised the compiler didn't warn about this
> obviously broken code...
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

NAK, this logic is actually trying to switch between the V4 and V5
directory format logic, but it's using the subtlety that @leaf3 is a
null pointer for a V4 fs and non-null for a V5 fs.   Hence this check
really should be:

	} else if (!leaf3 && stale != be16_to_cpu(leaf->hdr.stale)) {

Even though that looks *totally* wrong.  But xfs_check is deprecated, so
fmeh.

So I'll go fix that and rerun through testing.  I had thought this was
fine, but xfs/291 barfed up errors overnight.

--D

> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

diff --git a/db/check.c b/db/check.c
index 964756d0..d5f3d225 100644
--- a/db/check.c
+++ b/db/check.c
@@ -3452,7 +3452,7 @@  process_leaf_node_dir_v2_int(
 				 id->ino, dabno, stale,
 				 be16_to_cpu(leaf3->hdr.stale));
 		error++;
-	} else if (!leaf && stale != be16_to_cpu(leaf->hdr.stale)) {
+	} else if (leaf && stale != be16_to_cpu(leaf->hdr.stale)) {
 		if (!sflag || v)
 			dbprintf(_("dir %lld block %d stale mismatch "
 				 "%d/%d\n"),