Skip to contentSkip to conversation

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

PATCH In review Jesper Juhl 3 participants 6 messages Aug 12, 2006 – Aug 16, 2006 XFS filesystem

lore
Share & tools
lore

Jesper Juhl

author
#0 lore raw

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

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 <[email protected]>

---

 fs/xfs/xfs_da_btree.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)
fs/xfs/xfs_da_btree.c +2 -1
@@ -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;
#1 lore raw
Quoted 4 lines · Jesper Juhl

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:

xfs_da_btree.c +4 -4
Index: 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;
}

#2 lore raw

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())

Quoted 4 lines · Eric Sandeen

...
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

xfs_da_btree.c +18 -15
Index: xfs-linux/xfs_da_btree.c
===================================================================
@@ -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);
 			}
#3 lore raw

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())

Quoted 5 lines · Nathan Scott
Quoted 2 lines · Eric Sandeen

...
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

author
#4 lore raw

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())

In reply to Nathan Scott
Quoted 8 lines · Nathan Scott
Quoted 4 lines · Eric Sandeen

...
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 ;-)

Signature · 3 lines
Jesper Juhl <[email protected]>
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
#5 lore raw
In reply to Jesper Juhl
Quoted 4 lines · 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).

It looks like a false positive to me.

Quoted 3 lines

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.

blk->magic is guaranteed to be one of those two things by the first loop.

Quoted 2 lines

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

Reading closely through the 1st loop we can see that there's no way to get to the second loop without having one of the LEAF variants in the magic#. The NODE variant indicates its an internal btree node, and we move down the btree toward the leaves, at which point we jump out into the second loop.

Quoted 4 lines

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 :-)

Its not correct. What I think we should do is add a third branch into that second loop which just ASSERTs (to trip up a debug build) and set retval to EFSCORRUPTED (to shut these tools up).

cheers.

Signature · 1 line
Nathan