Re: [PATCH] ocfs2: dereference before NULL check in ocfs2_direct_IO_get_blocks()

[Florin Malita <fmalita@gmail.com>]

Alexey Dobriyan wrote:
> AFAICS, the patch is BS, as usual with this type of patches.
>   
You missed the full purpose: the patch makes a function consistent about
its own assumptions (but leaves them in place since they're on the
better-safe-than-sorry side). It addresses assumptions inconsistency
(confusing for both human readers and static analysis tools) just as
much as it addresses a possible bug. Regardless of whether "inode" &
"bh_result" can be NULL, I don't think the following is OK (coding-wise):

	unsigned long max_blocks = bh_result->b_size >> inode->i_blkbits;

	if (!inode || !bh_result) {
		mlog(ML_ERROR, "inode or bh_result is null\n");
		return -EIO;
	}


> Can "inode" and "bh_result" be NULL here? I bet they can't
Great, then the NULL branch is dead code and we can fix consistency
differently:

Signed-off-by: Florin Malita <fmalita@gmail.com>
---

 fs/ocfs2/aops.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 47152bf..370c241 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -558,16 +558,9 @@ static int ocfs2_direct_IO_get_blocks(st
 	u64 vbo_max; /* file offset, max_blocks from iblock */
 	u64 p_blkno;
 	int contig_blocks;
-	unsigned char blocksize_bits;
+	unsigned char blocksize_bits = inode->i_sb->s_blocksize_bits;
 	unsigned long max_blocks = bh_result->b_size >> inode->i_blkbits;
 
-	if (!inode || !bh_result) {
-		mlog(ML_ERROR, "inode or bh_result is null\n");
-		return -EIO;
-	}
-
-	blocksize_bits = inode->i_sb->s_blocksize_bits;
-
 	/* This function won't even be called if the request isn't all
 	 * nicely aligned and of the right size, so there's no need
 	 * for us to check any of that. */