[PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path

[PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path

[Eric Sandeen]

Coverity pointed this out; in the newly added
qgroup_subtree_accounting(), if btrfs_find_all_roots()
returns an error, we leak at least the parents pointer,
and possibly the roots pointer, depending on what failure
occurs.

If btrfs_find_all_roots() returns an error, we need to
free up all allocations before we return.  "roots" is
initialized to NULL, so it should be safe to free
it unconditionally (ulist_free() handles that case).

Cc: Mark Fasheh <mfasheh@suse.de>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index b497498..8abe455 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1973,7 +1973,7 @@ static int qgroup_subtree_accounting(struct btrfs_trans_handle *trans,
 				   elem.seq, &roots);
 	btrfs_put_tree_mod_seq(fs_info, &elem);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	if (roots->nnodes != 1)
 		goto out;

Re: [PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path

[Eric Sandeen]

On 8/17/14, 3:09 PM, Eric Sandeen wrote:
> Coverity pointed this out; in the newly added
> qgroup_subtree_accounting(), if btrfs_find_all_roots()
> returns an error, we leak at least the parents pointer,
> and possibly the roots pointer, depending on what failure
> occurs.

FWIW, Coverity also doesn't like this line:

        unode = ulist_next(roots, &uiter); /* Only want 1 so no need to loop */

it thinks that unode should be checked for NULL, but it seems
like that can't fail, especially since we already checked that
roots->nnodes == 1...

So maybe that should just be marked & ignored.
Or it could be added as a defensive check, I suppose...

-Eric

Re: [PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path

[Mark Fasheh]

On Sun, Aug 17, 2014 at 03:09:21PM -0500, Eric Sandeen wrote:
> Coverity pointed this out; in the newly added
> qgroup_subtree_accounting(), if btrfs_find_all_roots()
> returns an error, we leak at least the parents pointer,
> and possibly the roots pointer, depending on what failure
> occurs.
> 
> If btrfs_find_all_roots() returns an error, we need to
> free up all allocations before we return.  "roots" is
> initialized to NULL, so it should be safe to free
> it unconditionally (ulist_free() handles that case).

Great, thanks for this Eric.

Reviewed-by: Mark Fasheh <mfasheh@suse.de>

--
Mark Fasheh

Re: [PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path

[Chris Mason]

On 08/18/2014 05:42 PM, Mark Fasheh wrote:
> On Sun, Aug 17, 2014 at 03:09:21PM -0500, Eric Sandeen wrote:
>> Coverity pointed this out; in the newly added
>> qgroup_subtree_accounting(), if btrfs_find_all_roots()
>> returns an error, we leak at least the parents pointer,
>> and possibly the roots pointer, depending on what failure
>> occurs.
>>
>> If btrfs_find_all_roots() returns an error, we need to
>> free up all allocations before we return.  "roots" is
>> initialized to NULL, so it should be safe to free
>> it unconditionally (ulist_free() handles that case).
> 
> Great, thanks for this Eric.
> 
> Reviewed-by: Mark Fasheh <mfasheh@suse.de>
> 

Thanks guys, this is queued.

-chris