* [PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path
@ 2014-08-17 20:09 Eric Sandeen
2014-08-17 20:25 ` Eric Sandeen
2014-08-18 21:42 ` Mark Fasheh
0 siblings, 2 replies; 4+ messages in thread
From: Eric Sandeen @ 2014-08-17 20:09 UTC (permalink / raw
To: linux-btrfs; +Cc: Mark Fasheh
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;
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path
2014-08-17 20:09 [PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path Eric Sandeen
@ 2014-08-17 20:25 ` Eric Sandeen
2014-08-18 21:42 ` Mark Fasheh
1 sibling, 0 replies; 4+ messages in thread
From: Eric Sandeen @ 2014-08-17 20:25 UTC (permalink / raw
To: linux-btrfs; +Cc: Mark Fasheh
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path
2014-08-17 20:09 [PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path Eric Sandeen
2014-08-17 20:25 ` Eric Sandeen
@ 2014-08-18 21:42 ` Mark Fasheh
2014-08-19 15:27 ` Chris Mason
1 sibling, 1 reply; 4+ messages in thread
From: Mark Fasheh @ 2014-08-18 21:42 UTC (permalink / raw
To: Eric Sandeen; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path
2014-08-18 21:42 ` Mark Fasheh
@ 2014-08-19 15:27 ` Chris Mason
0 siblings, 0 replies; 4+ messages in thread
From: Chris Mason @ 2014-08-19 15:27 UTC (permalink / raw
To: Mark Fasheh, Eric Sandeen; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-08-19 15:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-17 20:09 [PATCH] btrfs: fix leak in qgroup_subtree_accounting() error path Eric Sandeen
2014-08-17 20:25 ` Eric Sandeen
2014-08-18 21:42 ` Mark Fasheh
2014-08-19 15:27 ` Chris Mason
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.