Re: [PATCH] Btrfs: avoid NULL deref after failed allocation

2008-10-02 Thread Andi Kleen
Jim Meyering <[EMAIL PROTECTED]> writes:

> However, in some places, the trend is
> to BUG_ON(!ptr), so I've done that, too.

Even if it's a trend, it's wrong.  Better don't add more. 
And also unnecessary because next reference will obviously oops anyways.

-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] Btrfs: avoid NULL deref after failed allocation

2008-10-02 Thread Jim Meyering
I scanned through the sources looking mostly at kmalloc uses
to see if a NULL result pointer could be dereferenced.
There were a few.  Where it was easy, I adjusted the code
to return -ENOMEM.  However, in some places, the trend is
to BUG_ON(!ptr), so I've done that, too.

There were two cases where nonzero "ret" appears
to have been inadvertently ignored, so I added BUG_ON(ret)
there, too, but it's possible those failures were deliberately ignored.

The second hunk doesn't matter as much: it just adds comments noting:
 - a possible leak
 - an invalid (or at least misleading) return code
 - another possible leak

If you're interested, give a little guidance on what you want,
and I'll be happy to make this more presentable.

---
 fs/btrfs/file.c |6 +-
 fs/btrfs/super.c|6 ++
 fs/btrfs/tree-log.c |6 ++
 3 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 3088a11..9c8a037 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -927,7 +927,11 @@ static ssize_t btrfs_file_write(struct file *file, const 
char __user *buf,
goto out_nolock;
file_update_time(file);
 
-   pages = kmalloc(nrptrs * sizeof(struct page *), GFP_KERNEL);
+   pages = kmalloc(nrptrs * sizeof(*pages), GFP_KERNEL);
+   if (!pages) {
+   err = -ENOMEM;
+   goto out_nolock;
+   }
 
mutex_lock(&inode->i_mutex);
first_index = pos >> PAGE_CACHE_SHIFT;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 2e60398..a158890 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -261,11 +261,15 @@ static int btrfs_parse_early_options(const char *options, 
int flags,
token = match_token(p, tokens, args);
switch (token) {
case Opt_subvol:
+   /* FIXME: multiple Opt_subvol options induce leak */
*subvol_name = match_strdup(&args[0]);
break;
case Opt_device:
error = btrfs_scan_one_device(match_strdup(&args[0]),
flags, holder, fs_devices);
+   /* FIXME: upon match_strdup failure, fail with
+  -ENOMEM rather than lookup_bdev's -EINVAL.
+  Does the match_strdup result need to be freed?  */
if (error)
goto out_free_opts;
break;
@@ -531,6 +535,8 @@ static long btrfs_control_ioctl(struct file *file, unsigned 
int cmd,
int len;
 
vol = kmalloc(sizeof(*vol), GFP_KERNEL);
+   if (!vol)
+   return -ENOMEM;
if (copy_from_user(vol, (void __user *)arg, sizeof(*vol))) {
ret = -EFAULT;
goto out;
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 88bbfd9..912c2ac 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -335,7 +335,9 @@ static noinline int overwrite_item(struct 
btrfs_trans_handle *trans,
return 0;
}
dst_copy = kmalloc(item_size, GFP_NOFS);
+   BUG_ON(!dst_copy);
src_copy = kmalloc(item_size, GFP_NOFS);
+   BUG_ON(!src_copy);
 
read_extent_buffer(eb, src_copy, src_ptr, item_size);
 
@@ -462,6 +464,7 @@ insert:
root->root_key.objectid,
trans->transid,
key->objectid, key->offset);
+   BUG_ON(ret);
} else {
/*
 * insert the extent pointer in the extent
@@ -633,6 +636,7 @@ static noinline int drop_one_dir_item(struct 
btrfs_trans_handle *trans,
btrfs_dir_item_key_to_cpu(leaf, di, &location);
name_len = btrfs_dir_name_len(leaf, di);
name = kmalloc(name_len, GFP_NOFS);
+   BUG_ON(!name);
read_extent_buffer(leaf, name, (unsigned long)(di + 1), name_len);
btrfs_release_path(root, path);
 
@@ -2307,6 +2311,7 @@ static noinline int log_dir_items(struct 
btrfs_trans_handle *trans,
ret = overwrite_item(trans, log, dst_path,
 path->nodes[0], path->slots[0],
 &tmp);
+   BUG_ON(ret);
}
}
btrfs_release_path(root, path);
@@ -2477,6 +2482,7 @@ static noinline int copy_items(struct btrfs_trans_handle 
*trans,
 
ins_data = kmalloc(nr * sizeof(struct btrfs_key) +
   nr * sizeof(u32), GFP_NOFS);
+   BUG_ON(!ins_data);
ins_sizes = (u32 *)ins_data;
ins_keys = (struct btrfs_key *)(ins_data + nr * sizeof(u32));
 
-- 
1.6.0.2.27.gea240
--
To unsubscribe from this list: send the l