| Message ID | 53F10BF1.6070602@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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> --- -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html