[PATCH] XFS: possibly uninitialized variable use in fs/xfs/xfs_da_btree.c::xfs_da_node_lookup_int()

[PATCH] XFS: possibly uninitialized variable use in fs/xfs/xfs_da_btree.c::xfs_da_node_lookup_int()

[Jesper Juhl]

Ok, I'll honestly admit that I'm in over my head here. But, coverity found
a potential use of an uninitialized variable in 
fs/xfs/xfs_da_btree.c::xfs_da_node_lookup_int() and I think it might be 
correct (coverity bug #900).

The problem spot is this bit of code : 

...
 			if (blk->magic == XFS_DIR2_LEAFN_MAGIC) {
 				retval = xfs_dir2_leafn_lookup_int(blk->bp, args,
 								&blk->index, state);
 			}
 			else if (blk->magic == XFS_ATTR_LEAF_MAGIC) {
 				retval = xfs_attr_leaf_lookup_int(blk->bp, args);
 				blk->index = args->index;
 				args->blkno = blk->blkno;
 			}
 			if (((retval == ENOENT) || (retval == ENOATTR)) &&

                              ^^^--- 'retval' possibly used uninitialized here...

 			    (blk->hashval == args->hashval)) {
...

Nothing initializes 'retval' before this bit of code, so if 'blk->magic' is 
neither == XFS_DIR2_LEAFN_MAGIC or XFS_ATTR_LEAF_MAGIC then it'll be in an 
uninitialized state when we get to the "if (((retval == ENOENT) ..." bit.

Now we get to the part where I'm in over my head. 

Since there is code above that check 'blk->magic' against 
being == XFS_DA_NODE_MAGIC and I don't see how we would be skipping the 
code quoted above in that case, it looks to me like this could happen and 
lead to the uninitialized use of retval.
It also seems to me, from reading fs/xfs/xfs_da_btree.h, that 'blk->magic' 
might in some cases be == XFS_DIR2_LEAF1_MAGIC in which case we'd also end 
up using retval uninitialized.

Now, what to do about it. Well, if I'm reading the function correctly the 
safest thing would be to assume a 'retval' of ENOENT if the above 
"if/else if" didn't trigger, so below is a patch that initializes 'retval' 
to that value so that if we do hit this corner case we'll just act as if 
we couldn't find what we were looking for in the Btree.

I suspect I may be completely wrong, and if that's the case I'd sure like 
an explanation of where I went wrong along with the NACK for the patch.
In case my understanding is in fact correct and the patch below makes sense,
then kindly apply :-)



Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 fs/xfs/xfs_da_btree.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.18-rc4-orig/fs/xfs/xfs_da_btree.c	2006-08-11 00:11:13.000000000 +0200
+++ linux-2.6.18-rc4/fs/xfs/xfs_da_btree.c	2006-08-12 23:18:23.000000000 +0200
@@ -1053,7 +1053,8 @@ xfs_da_node_lookup_int(xfs_da_state_t *s
 	xfs_da_intnode_t *node;
 	xfs_da_node_entry_t *btree;
 	xfs_dablk_t blkno;
-	int probe, span, max, error, retval;
+	int probe, span, max, error;
+	int retval = ENOENT;
 	xfs_dahash_t hashval;
 	xfs_da_args_t *args;
 

Re: [PATCH] XFS: possibly uninitialized variable use in fs/xfs/xfs_da_btree.c::xfs_da_node_lookup_int()

[Eric Sandeen]

Jesper Juhl wrote:
> I suspect I may be completely wrong, and if that's the case I'd sure like 
> an explanation of where I went wrong along with the NACK for the patch.
> In case my understanding is in fact correct and the patch below makes sense,
> then kindly apply :-)
> 

Seems reasonable to me; I also can't see how it avoids using an uninitialized 
retval in all cases.  But I'm in over my head too, so don't take my word for it 
:).  I'll let the folks @sgi chime in...

FWIW seems like there's a lot of unnecessary endian flipping in there too; I 
haven't tested this but since it endian-flips the magic into blk->magic seems 
like it may as well use it:

Index: xfs-linux/xfs_da_btree.c
===================================================================
--- xfs-linux.orig/xfs_da_btree.c
+++ xfs-linux/xfs_da_btree.c
@@ -1079,14 +1079,14 @@ xfs_da_node_lookup_int(xfs_da_state_t *s
  			return(error);
  		}
  		curr = blk->bp->data;
-		ASSERT(be16_to_cpu(curr->magic) == XFS_DA_NODE_MAGIC ||
-		       be16_to_cpu(curr->magic) == XFS_DIR2_LEAFN_MAGIC ||
-		       be16_to_cpu(curr->magic) == XFS_ATTR_LEAF_MAGIC);
+		blk->magic = be16_to_cpu(curr->magic);
+		ASSERT(blk->magic == XFS_DA_NODE_MAGIC ||
+		       blk->magic == XFS_DIR2_LEAFN_MAGIC ||
+		       blk->magic == XFS_ATTR_LEAF_MAGIC);

  		/*
  		 * Search an intermediate node for a match.
  		 */
-		blk->magic = be16_to_cpu(curr->magic);
  		if (blk->magic == XFS_DA_NODE_MAGIC) {
  			node = blk->bp->data;
  			blk->hashval = 
be32_to_cpu(node->btree[be16_to_cpu(node->hdr.count)-1].hashval);
@@ -1133,10 +1133,10 @@ xfs_da_node_lookup_int(xfs_da_state_t *s
  				blk->index = probe;
  				blkno = be32_to_cpu(btree->before);
  			}
-		} else if (be16_to_cpu(curr->magic) == XFS_ATTR_LEAF_MAGIC) {
+		} else if (blk->magic == XFS_ATTR_LEAF_MAGIC) {
  			blk->hashval = xfs_attr_leaf_lasthash(blk->bp, NULL);
  			break;
-		} else if (be16_to_cpu(curr->magic) == XFS_DIR2_LEAFN_MAGIC) {
+		} else if (blk->magic == XFS_DIR2_LEAFN_MAGIC) {
  			blk->hashval = xfs_dir2_leafn_lasthash(blk->bp, NULL);
  			break;
  		}

review: cleanup xfs_da_node_lookup_int (was Re: [PATCH] XFS: possibly uninitialized variable use in fs/xfs/xfs_da_btree.c::xfs_da_node_lookup_int())

[Nathan Scott]

On Sat, Aug 12, 2006 at 10:24:54PM -0500, Eric Sandeen wrote:
> ...
> FWIW seems like there's a lot of unnecessary endian flipping in there too; I 
> haven't tested this but since it endian-flips the magic into blk->magic seems 
> like it may as well use it:

How's this look?

thanks.

--
Nathan


Index: xfs-linux/xfs_da_btree.c
===================================================================
--- xfs-linux.orig/xfs_da_btree.c	2006-08-14 15:34:20.623194000 +1000
+++ xfs-linux/xfs_da_btree.c	2006-08-14 15:42:07.198743750 +1000
@@ -1054,7 +1054,7 @@ xfs_da_node_lookup_int(xfs_da_state_t *s
 	xfs_da_node_entry_t *btree;
 	xfs_dablk_t blkno;
 	int probe, span, max, error, retval;
-	xfs_dahash_t hashval;
+	xfs_dahash_t hashval, btreehashval;
 	xfs_da_args_t *args;
 
 	args = state->args;
@@ -1079,30 +1079,32 @@ xfs_da_node_lookup_int(xfs_da_state_t *s
 			return(error);
 		}
 		curr = blk->bp->data;
-		ASSERT(be16_to_cpu(curr->magic) == XFS_DA_NODE_MAGIC ||
-		       be16_to_cpu(curr->magic) == XFS_DIR2_LEAFN_MAGIC ||
-		       be16_to_cpu(curr->magic) == XFS_ATTR_LEAF_MAGIC);
+		blk->magic = be16_to_cpu(curr->magic);
+		ASSERT(blk->magic == XFS_DA_NODE_MAGIC ||
+		       blk->magic == XFS_DIR2_LEAFN_MAGIC ||
+		       blk->magic == XFS_ATTR_LEAF_MAGIC);
 
 		/*
 		 * Search an intermediate node for a match.
 		 */
-		blk->magic = be16_to_cpu(curr->magic);
 		if (blk->magic == XFS_DA_NODE_MAGIC) {
 			node = blk->bp->data;
-			blk->hashval = be32_to_cpu(node->btree[be16_to_cpu(node->hdr.count)-1].hashval);
+			max = be16_to_cpu(node->hdr.count);
+			btreehashval = node->btree[max-1].hashval;
+			blk->hashval = be32_to_cpu(btreehashval);
 
 			/*
 			 * Binary search.  (note: small blocks will skip loop)
 			 */
-			max = be16_to_cpu(node->hdr.count);
 			probe = span = max / 2;
 			hashval = args->hashval;
 			for (btree = &node->btree[probe]; span > 4;
 				   btree = &node->btree[probe]) {
 				span /= 2;
-				if (be32_to_cpu(btree->hashval) < hashval)
+				btreehashval = be32_to_cpu(btree->hashval);
+				if (btreehashval < hashval)
 					probe += span;
-				else if (be32_to_cpu(btree->hashval) > hashval)
+				else if (btreehashval > hashval)
 					probe -= span;
 				else
 					break;
@@ -1133,10 +1135,10 @@ xfs_da_node_lookup_int(xfs_da_state_t *s
 				blk->index = probe;
 				blkno = be32_to_cpu(btree->before);
 			}
-		} else if (be16_to_cpu(curr->magic) == XFS_ATTR_LEAF_MAGIC) {
+		} else if (blk->magic == XFS_ATTR_LEAF_MAGIC) {
 			blk->hashval = xfs_attr_leaf_lasthash(blk->bp, NULL);
 			break;
-		} else if (be16_to_cpu(curr->magic) == XFS_DIR2_LEAFN_MAGIC) {
+		} else if (blk->magic == XFS_DIR2_LEAFN_MAGIC) {
 			blk->hashval = xfs_dir2_leafn_lasthash(blk->bp, NULL);
 			break;
 		}
@@ -1152,11 +1154,13 @@ xfs_da_node_lookup_int(xfs_da_state_t *s
 		if (blk->magic == XFS_DIR2_LEAFN_MAGIC) {
 			retval = xfs_dir2_leafn_lookup_int(blk->bp, args,
 							&blk->index, state);
-		}
-		else if (blk->magic == XFS_ATTR_LEAF_MAGIC) {
+		} else if (blk->magic == XFS_ATTR_LEAF_MAGIC) {
 			retval = xfs_attr_leaf_lookup_int(blk->bp, args);
 			blk->index = args->index;
 			args->blkno = blk->blkno;
+		} else {
+			ASSERT(0);
+			return XFS_ERROR(EFSCORRUPTED);
 		}
 		if (((retval == ENOENT) || (retval == ENOATTR)) &&
 		    (blk->hashval == args->hashval)) {
@@ -1166,8 +1170,7 @@ xfs_da_node_lookup_int(xfs_da_state_t *s
 				return(error);
 			if (retval == 0) {
 				continue;
-			}
-			else if (blk->magic == XFS_ATTR_LEAF_MAGIC) {
+			} else if (blk->magic == XFS_ATTR_LEAF_MAGIC) {
 				/* path_shift() gives ENOENT */
 				retval = XFS_ERROR(ENOATTR);
 			}

Re: review: cleanup xfs_da_node_lookup_int (was Re: [PATCH] XFS: possibly uninitialized variable use in fs/xfs/xfs_da_btree.c::xfs_da_node_lookup_int())

[Eric Sandeen]

Nathan Scott wrote:
> On Sat, Aug 12, 2006 at 10:24:54PM -0500, Eric Sandeen wrote:
>> ...
>> FWIW seems like there's a lot of unnecessary endian flipping in there too; I 
>> haven't tested this but since it endian-flips the magic into blk->magic seems 
>> like it may as well use it:
> 
> How's this look?

Looks good to me.

(now that I look closer & grok that there are 2 loops in that function 
and the break is in the nested one... oops!)

-Eric

Re: review: cleanup xfs_da_node_lookup_int (was Re: [PATCH] XFS: possibly uninitialized variable use in fs/xfs/xfs_da_btree.c::xfs_da_node_lookup_int())

[Jesper Juhl]

On 14/08/06, Nathan Scott <nathans@sgi.com> wrote:
> On Sat, Aug 12, 2006 at 10:24:54PM -0500, Eric Sandeen wrote:
> > ...
> > FWIW seems like there's a lot of unnecessary endian flipping in there too; I
> > haven't tested this but since it endian-flips the magic into blk->magic seems
> > like it may as well use it:
>
> How's this look?
>
As far as I can see it looks good, but I'm still just starting to read
the XFS code, so I'm far from reliable yet  ;-)

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html