diff mbox

[v2,1/2] nfs-utils: mountd: Use a dynamic buffer for storing lists of gid's

Message ID 1302160948-5628-1-git-send-email-sean.finney@sonyericsson.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sean Finney April 7, 2011, 7:22 a.m. UTC
Previously, in auth_unix_gid, group lists were stored in an array of
hard-coded length 100, and in the situation that the group lists for a
particular call were too large, the array was swapped with a dynamically
allocated/freed buffer.  For environments where users are commonly in
a large number of groups, this isn't an ideal approach.

Instead, make the group list static scoped to the function, and
use malloc/realloc to grow it on an as-needed basis.

Signed-off-by: Sean Finney <sean.finney@sonyericsson.com>
---
 utils/mountd/cache.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)

Comments

Jim Rees April 7, 2011, 12:22 p.m. UTC | #1
Sean Finney wrote:

  Previously, in auth_unix_gid, group lists were stored in an array of
  hard-coded length 100, and in the situation that the group lists for a
  particular call were too large, the array was swapped with a dynamically
  allocated/freed buffer.  For environments where users are commonly in
  a large number of groups, this isn't an ideal approach.
  
  Instead, make the group list static scoped to the function, and
  use malloc/realloc to grow it on an as-needed basis.

I would re-word this.  The list isn't static, the list pointer is.  I don't
think you need to mention that, it's just confusing.  "Use malloc/realloc to
grow the list on an as-needed basis."

  +		groups = malloc(sizeof(gid_t)*INITIAL_MANAGED_GROUPS);

Style nit:  Put blanks around the "*".

I was going to suggest you first call getgrouplist with groups set to 0 so
you can always alloc() the correct size list, but then I found this in the
man page:

BUGS
       In  glibc  versions  before  2.3.3, the implementation of this function
       contains a buffer-overrun bug: it returns the complete list  of  groups
       for  user  in  the array groups, even when the number of groups exceeds
       *ngroups.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chuck Lever III April 7, 2011, 2:29 p.m. UTC | #2
On Apr 7, 2011, at 10:15 AM, Finney, Sean wrote:

> Hi Jim,
> 
> On Thu, 2011-04-07 at 08:22 -0400, Jim Rees wrote:
> 
>> I was going to suggest you first call getgrouplist with groups set to 0 so
>> you can always alloc() the correct size list, but then I found this in the
>> man page:
>> 
>> BUGS
>>       In  glibc  versions  before  2.3.3, the implementation of this function
>>       contains a buffer-overrun bug: it returns the complete list  of  groups
>>       for  user  in  the array groups, even when the number of groups exceeds
>>       *ngroups.
> 
> Yuck.  The function is also not in POSIX or any other standards, so
> maybe it's worth discussing at a later point whether getgroups(2) would
> be a better interface to use, even if it means an extra step or two.

The standard RPC library function authunix_create_default() uses getgroups(2).  Take a look at the glibc version of this function to see the preferred method for using getgroups(2).
Jim Rees April 7, 2011, 3:01 p.m. UTC | #3
Finney, Sean wrote:

  > I would re-word this.  The list isn't static, the list pointer is.  I don't
  > think you need to mention that, it's just confusing.  "Use malloc/realloc to
  > grow the list on an as-needed basis."
  
  Okay.  I was trying to just put a hint that there was a persistant
  buffer that would hang around through multiple invocations, though
  perhaps that's a little too much 'implementation details'.  Totally fine
  with modifying the description either way :)

If it were me, I would alloc and free the list each time rather than keeping
it around.  It's not like malloc is super expensive.  But that's a matter of
taste.

  > BUGS
  >        In  glibc  versions  before  2.3.3, the implementation of this function
  >        contains a buffer-overrun bug: it returns the complete list  of  groups
  >        for  user  in  the array groups, even when the number of groups exceeds
  >        *ngroups.
  
  Yuck.  The function is also not in POSIX or any other standards, so
  maybe it's worth discussing at a later point whether getgroups(2) would
  be a better interface to use, even if it means an extra step or two.

There's no sense in making nfs-utils portable to non-linux, so depending on
glibc is probably ok.  2.3.3 was released in December 2003 so I would argue
we don't care about the buffer-overrun bug.

  Anyway, I will re-spin this patch with the two changes that you've
  suggested and submit it tomorrow.   Care to take a look at patch
  2/2?  :)

It looked fine to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 9bbbfb3..7deb050 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -37,6 +37,7 @@ 
 #include "blkid/blkid.h"
 #endif
 
+#define INITIAL_MANAGED_GROUPS 100
 
 enum nfsd_fsid {
 	FSID_DEV = 0,
@@ -127,11 +128,21 @@  void auth_unix_gid(FILE *f)
 	 */
 	int uid;
 	struct passwd *pw;
-	gid_t glist[100], *groups = glist;
-	int ngroups = 100;
+	static gid_t *groups = NULL;
+	static int groups_len = 0;
+	gid_t *more_groups;
+	int ngroups = 0;
 	int rv, i;
 	char *cp;
 
+	if (groups_len == 0) {
+		groups = malloc(sizeof(gid_t)*INITIAL_MANAGED_GROUPS);
+		if (!groups)
+			return;
+
+		groups_len = ngroups = INITIAL_MANAGED_GROUPS;
+	}
+
 	if (readline(fileno(f), &lbuf, &lbuflen) != 1)
 		return;
 
@@ -144,13 +155,16 @@  void auth_unix_gid(FILE *f)
 		rv = -1;
 	else {
 		rv = getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups);
-		if (rv == -1 && ngroups >= 100) {
-			groups = malloc(sizeof(gid_t)*ngroups);
-			if (!groups)
+		if (rv == -1 && ngroups >= groups_len) {
+			more_groups = realloc(groups, sizeof(gid_t)*ngroups);
+			if (!more_groups)
 				rv = -1;
-			else
+			else {
+				groups = more_groups;
+				groups_len = ngroups;
 				rv = getgrouplist(pw->pw_name, pw->pw_gid,
 						  groups, &ngroups);
+			}
 		}
 	}
 	qword_printint(f, uid);
@@ -162,9 +176,6 @@  void auth_unix_gid(FILE *f)
 	} else
 		qword_printint(f, 0);
 	qword_eol(f);
-
-	if (groups != glist)
-		free(groups);
 }
 
 #if USE_BLKID