xfs
[Top] [All Lists]

[PATCH 06/14] xfs_quota: fix memory leak in quota_group_type() error pat

To: xfs@xxxxxxxxxxx
Subject: [PATCH 06/14] xfs_quota: fix memory leak in quota_group_type() error path
From: Eric Sandeen <sandeen@xxxxxxxxxx>
Date: Tue, 8 Apr 2014 18:24:56 -0500
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1396999504-13769-1-git-send-email-sandeen@xxxxxxxxxx>
References: <1396999504-13769-1-git-send-email-sandeen@xxxxxxxxxx>
quota_group_type has some rather contorted logic that's
been around since 2005.

In the (!name) case, if any of the 3 calls setting up ngroups fails,
we fall back to using just one group.

However, if it's the getgroups() call that fails, we overwrite
the allocated gid ptr with &gid, thus leaking that allocated
memory.  Worse, we set "dofree" to 1, so will free non-allocated
local var gid.  And that last else case is redundant; if we get there,
gids is guaranteed to be non-null.

Refactor it a bit to be more clear (I hope) and correct.

Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
---
 quota/quota.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/quota/quota.c b/quota/quota.c
index 7e52ad2..367da8c 100644
--- a/quota/quota.c
+++ b/quota/quota.c
@@ -289,15 +289,19 @@ quota_group_type(
                }
                gids = &gid;
                ngroups = 1;
-       } else if ( ((ngroups = sysconf(_SC_NGROUPS_MAX)) < 0) ||
-                   ((gids = malloc(ngroups * sizeof(gid_t))) == NULL) ||
-                   ((ngroups = getgroups(ngroups, gids)) < 0)) {
-               dofree = (gids != NULL);
-               gid = getgid();
-               gids = &gid;
-               ngroups = 1;
        } else {
-               dofree = (gids != NULL);
+               if ( ((ngroups = sysconf(_SC_NGROUPS_MAX)) < 0) ||
+                    ((gids = malloc(ngroups * sizeof(gid_t))) == NULL) ||
+                    ((ngroups = getgroups(ngroups, gids)) < 0)) {
+                       /* something failed.  Fall back to 1 group */
+                       free(gids);
+                       gid = getgid();
+                       gids = &gid;
+                       ngroups = 1;
+               } else {
+                       /* It all worked, and we allocated memory */
+                       dofree = 1;
+               }
        }
 
        for (i = 0; i < ngroups; i++, name = NULL) {
-- 
1.7.1

<Prev in Thread] Current Thread [Next in Thread>