lkml.org 
[lkml]   [2006]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ocfs2: dereference before NULL check in ocfs2_direct_IO_get_blocks()
On Sat, Jun 03, 2006 at 11:34:39AM -0400, Florin Malita wrote:
> 'bh_result' & 'inode' are explicitly checked against NULL so they
> shouldn't be dereferenced prior to that.
>
> Coverity ID: 1273, 1274.

> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -559,13 +559,14 @@ static int ocfs2_direct_IO_get_blocks(st
> u64 p_blkno;
> int contig_blocks;
> unsigned char blocksize_bits;
> - unsigned long max_blocks = bh_result->b_size >> inode->i_blkbits;
> + unsigned long max_blocks;
>
> if (!inode || !bh_result) {
> mlog(ML_ERROR, "inode or bh_result is null\n");
> return -EIO;
> }
>
> + max_blocks = bh_result->b_size >> inode->i_blkbits;
> blocksize_bits = inode->i_sb->s_blocksize_bits;

AFAICS, the patch is BS, as usual with this type of patches.

Can "inode" and "bh_result" be NULL here? I bet they can't.

1. The sole purpose of ocfs2_direct_IO_get_blocks() is to be given to
blockdev_direct_IO_no_locking() at fs/ocfs2/aops.c:662

Can blockdev_direct_IO_no_locking() call it with first or third
arg being NULL?

2. blockdev_direct_IO_no_locking() is static inline wrapper
include/linux/fs.h:1687

Can __blockdev_direct_IO() call it's "get_block" arg with first
or third arg being NULL?

3. Aiee, __blockdev_direct_IO() is normal function (fs/direct-io.c:1179)
and it gives damn "get_block" arg to direct_io_worker() at
fs/direct-io.c:1276

4. direct_io_worker() initializes ->get_block method of "dio" structure
and then does something my tiny mind fails to understand. All I can
see it doesn't call nor change method later.

Where is ->get_block called?

5. That what grep(1) is for. One place

fs/direct-io.c:553: ret = (*dio->get_block)(dio->inode, fs_startblk,
fs/direct-io.c-554- map_bh, create);

Now time to press PageUp. We're seeking for dereferences of
"dio->inode" and "map_bh" above.

6. Both are immediately out of question:

536 map_bh->b_size = fs_count << dio->inode->i_blkbits;
^^^^^^ ^^^^^^^^^^
so it can't be NULL ditto
537
538 create = dio->rw == WRITE;
539 if (dio->lock_type == DIO_LOCKING) {
540 if (dio->block_in_file < (i_size_read(dio->inode) >>
541 dio->blkbits))
542 create = 0;
543 } else if (dio->lock_type == DIO_NO_LOCKING) {
544 create = 0;
545 }
546
547 /*
548 * For writes inside i_size we forbid block creations: only
549 * overwrites are permitted. We fall back to buffered writes
550 * at a higher level for inside-i_size block-instantiating
551 * writes.
552 */
553 ret = (*dio->get_block)(dio->inode, fs_startblk,
^^^^^^^^^^^^^^ ^^^^^^^^^^
offending call offending arg
554 map_bh, create);
^^^^^^
offending arg
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-06-03 21:17    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site