Skip to contentSkip to conversation

btrfs: fix leak in qgroup_subtree_accounting() error path

PATCH In review Eric Sandeen 3 participants 4 messages Aug 17, 2014 – Aug 19, 2014 Btrfs file system

lore
Share & tools
lore

Eric Sandeen

author
#0 lore raw

PATCH 1/1 btrfs: fix leak in qgroup_subtree_accounting() error path

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 <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>

---

fs/btrfs/qgroup.c +1 -1
index b497498..8abe455 100644
@@ -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;
author
#1 lore raw
Quoted 5 lines · 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.

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

Reviewed-by on v1 1/1
#2 lore raw
In reply to Eric Sandeen
Quoted 10 lines · 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).

Great, thanks for this Eric.

Reviewed-by Mark Fasheh <[email protected]>

--
Mark Fasheh

#3 lore raw
Quoted 7 lines · Mark Fasheh
Quoted 6 lines · 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.

Quoted 9 lines
Quoted 4 lines

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 <[email protected]>

Thanks guys, this is queued.

-chris