Fixes for NFS in environments with large group memberships

Fixes for NFS in environments with large group memberships

[Finney, Sean]

SGkgVGhlcmUhCgpMYXN0IHdlZWsgSSBmaXJlZCBvZmYgYSBjb3VwbGUgcGF0Y2hlcyB0byB0aGlz
IGxpc3QgYW5kIGluIGhpbmRzaWdodCBJCnJlYWxpemUgdGhhdCBJIG1pZ2h0IG5vdCBoYXZlIGJl
ZW4gdG90YWxseSBjbGVhciBmb3IgdGhlIG1vdGl2YXRpb24KYmVoaW5kIHRoZSBwYXRjaGVzOgoK
W1BBVENIXSBVc2UgYSBuYW1lZCBjb25zdGFudCBmb3IgbWF4IG51bWJlciBvZiBtYW5hZ2VkIGdy
b3VwcywgYW5kIGluYwpbUEFUQ0hdIEluY3JlYXNlIHRoZSBkZWZhdWx0IGJ1ZmZlciBzaXplIGZv
ciBjYWNoZWxpc3QgY2hhbm5lbCBmaWxlcy4KClNpbmNlIG5vYm9keSBoYXMgcmV2aWV3ZWQgdGhl
bSB5ZXQsIEkgdGhvdWdodCBJJ2QgbWFrZSBpdCBjbGVhciB0aGF0CnRoZXNlIGNoYW5nZXMgKG9y
IHNvbWV0aGluZyBhbG9uZyBzdWNoIGxpbmVzKSBhcmUgcmVxdWlyZWQgZm9yIE5GUwphY2Nlc3Mg
dG8gbW91bnRwb2ludHMgd2hlbiB1c2VycyBhcmUgaW4gbGFyZ2Vpc2ggKD4xMDApIG51bWJlcnMg
b2YKZ3JvdXBzLiAgVGhlcmUgYXJlIGFjdHVhbGx5IHR3byBwcm9ibGVtczoKCiogSGFyZC1jb2Rl
ZCBsaW1pdCBmb3IgZ3JvdXAgbWVtYmVyc2hpcCBhdCAxMDAgZ3JvdXBzIGluIG1vdW50ZCBzb3Vy
Y2UKKiBMYXJnZSBzZXJ2ZXItc2lkZSBwcm9jZnMgd3JpdGVzIGFyZSBzcGxpdCBpbnRvIG11bGl0
cGxlIHdyaXRlcyBkdWUgdG8Kc3RkaW8gYnVmZmVyaW5nIGluIG1vdW50ZCBhbmQgc3ZjZ3NzZCBz
b3VyY2VzLCByZXN1bHRpbmcgaW4gY2xpZW50CmhhbmdzLgoKVGhlIHBhdGNoZXMgaW4gcXVlc3Rp
b24gc29sdmUgdGhpcyBpbiBhcyBub24taW50cnVzaXZlIGEgbWFubmVyIGFzCnBvc3NpYmxlLCB0
aG91Z2ggSSdtIG9mIGNvdXJzZSBvcGVuIHRvIGNoYW5naW5nIHRoaW5ncyB1cCBpZiB5b3UgZmVl
bCBpdApzaG91bGQgYmUgZG9uZSBvdGhlcndpc2UuICBTbyBwbGVhc2UgbGV0IG1lIGtub3cgd2hh
dCB5b3UgdGhpbmssIG9yIGlmCml0IHdvdWxkIGJlIGJldHRlciBmb3IgbWUgdG8gZmlyc3QgZ28g
YW5kIG9wZW4gYSBidWcsIHN0YXJ0IGEKZGlzY3Vzc2lvbiwgZXRjLgoKClRoYW5rcywKU2VhbiAK
Tu+/ve+/ve+/ve+/ve+/vXLvv73vv71577+977+977+9Yu+/vVjvv73vv73Hp3bvv71e77+9Kd66
ey5u77+9K++/ve+/ve+/ve+/vXvvv73vv73vv70i77+977+9Xm7vv71y77+977+977+9eu+/vRrv
v73vv71o77+977+977+977+9Ju+/ve+/vR7vv71H77+977+977+9aO+/vQMo77+96ZqO77+93aJq
Iu+/ve+/vRrvv70bbe+/ve+/ve+/ve+/ve+/vXrvv73elu+/ve+/ve+/vWbvv73vv73vv71o77+9
77+977+9fu+/vW3vv70=

Re: Fixes for NFS in environments with large group memberships

[J. Bruce Fields]

On Mon, Apr 04, 2011 at 10:02:59AM +0200, Finney, Sean wrote:
> Hi There!
>=20
> Last week I fired off a couple patches to this list and in hindsight =
I
> realize that I might not have been totally clear for the motivation
> behind the patches:
>=20
> [PATCH] Use a named constant for max number of managed groups, and in=
c
> [PATCH] Increase the default buffer size for cachelist channel files.

I can't find those; could you resend?

--b.

>=20
> Since nobody has reviewed them yet, I thought I'd make it clear that
> these changes (or something along such lines) are required for NFS
> access to mountpoints when users are in largeish (>100) numbers of
> groups.  There are actually two problems:
>=20
> * Hard-coded limit for group membership at 100 groups in mountd sourc=
e
> * Large server-side procfs writes are split into mulitple writes due =
to
> stdio buffering in mountd and svcgssd sources, resulting in client
> hangs.
>=20
> The patches in question solve this in as non-intrusive a manner as
> possible, though I'm of course open to changing things up if you feel=
 it
> should be done otherwise.  So please let me know what you think, or i=
f
> it would be better for me to first go and open a bug, start a
> discussion, etc.
>=20
>=20
> Thanks,
> Sean=20
> N?????r??y????b?X??=C7=A7v?^?)=DE=BA{.n?+????{???"??^n?r???z?=1A??h??=
???&??=1E?G???h?=03(?=E9=9A=8E?=DD=A2j"??=1A?=1Bm??????z?=DE=96???f???h=
???~?m

Re: Fixes for NFS in environments with large group memberships

[Finney, Sean]

SGkgQnJ1Y2UsCgpPbiBUdWUsIDIwMTEtMDQtMDUgYXQgMTk6MzUgLTA0MDAsIEouIEJydWNlIEZp
ZWxkcyB3cm90ZToKCj4gPiAKPiA+IFtQQVRDSF0gVXNlIGEgbmFtZWQgY29uc3RhbnQgZm9yIG1h
eCBudW1iZXIgb2YgbWFuYWdlZCBncm91cHMsIGFuZCBpbmMKPiA+IFtQQVRDSF0gSW5jcmVhc2Ug
dGhlIGRlZmF1bHQgYnVmZmVyIHNpemUgZm9yIGNhY2hlbGlzdCBjaGFubmVsIGZpbGVzLgo+IAo+
IEkgY2FuJ3QgZmluZCB0aG9zZTsgY291bGQgeW91IHJlc2VuZD8KClN1cmUgdGhpbmcuICBPbiBh
IHNlY29uZCByZXZpZXcsIGl0IHNlZW1zIHRoYXQgdGhlICJuYW1lZCBjb25zdGFudCIKcGF0Y2gg
bWF5IG5vdCBiZSBzdHJpY3RseSBuZWNlc3NhcnksIHRob3VnaCBidW1waW5nIHRoZSB2YWx1ZSBh
dm9pZHMKbmVlZGluZyB0byBkbyBhIG1hbGxvYyBldmVyeSBvcGVyYXRpb24uICBJIHRoaW5rIEkn
bGwgdGFrZSBhIHF1aWNrIHNob3QKYXQgcmUtc3Bpbm5pbmcgdGhhdCBwYXRjaCB0byB1c2UgYSBk
eW5hbWljYWxseSBncm93aW5nIHN0YXRpYyBidWZmZXIKaW5zdGVhZFsxXSwgYW5kIHJlLXNlbmQg
dGhlIHBhdGNoIGFmdGVyIHRoYXQuCgoJU2VhbgoKWzFdIEkgd2FzIGhlc2l0YW50IHRvIGRvIHRo
aXMgYXQgZmlyc3QgZHVlIHRvIGFsbCB0aGUgcmVmZXJlbmNlcyB0bwoidGhyZWFkcyIsIGJ1dCBm
cm9tIHdoYXQgSSBjYW4gdGVsbCAoYSkgbmZzLXV0aWxzIGRvZXNuJ3QgdXNlIHRocmVhZHMKYW5k
IGluc3RlYWQgZm9yaygpLCBhbmQgKGIpIHRoZSBzYW1lIHRlY2huaXF1ZSBpcyBhbHJlYWR5IHVz
ZWQgaW4gYXQKbGVhc3Qgb25lIG90aGVyIHBsYWNlIGluIHRoZSBjb2RlLgoT77+977+97Lm7HO+/
vSbvv71+77+9Ju+/vRjvv73vv70rLe+/ve+/vd22F++/ve+/vXfvv73vv73Lm++/ve+/ve+/vW3v
v71i77+977+9Z37Ip++/vRfvv73vv73cqH3vv73vv73vv73GoHrvv70majordu+/ve+/ve+/vQfv
v73vv73vv73vv716Wivvv73vv70rembvv73vv73vv71o77+977+977+9fu+/ve+/ve+/ve+/vWnv
v73vv73vv71677+9Hu+/vXfvv73vv73vv70/77+977+977+977+9Ju+/vSnfohtm

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

[Sean Finney]

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(-)

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
-- 
1.7.4.1

[PATCH v2 2/2] nfs-utils: Increase the stdio file buffer size for procfs files

[Sean Finney]

Previously, when writing to /proc/net/rpc/*/channel, if a cache line
were larger than the default buffer size (likely 1024 bytes), mountd
and svcgssd would split writes into a number of buffer-sized writes.
Each of these writes would get an EINVAL error back from the kernel
procfs handle (it expects line-oriented input and does not account for
multiple/split writes), and no cache update would occur.

When such behavior occurs, NFS clients depending on mountd to finish
the cache operation would block/hang, or receive EPERM, depending on
the context of the operation.  This is likely to happen if a user is a
member of a large (~100-200) number of groups.

Instead, every fopen() on the procfs files in question is followed by
a call to setvbuf(), using a per-file dedicated buffer of
RPC_CHAN_BUF_SIZE length.

Really, mountd should not be using stdio-style buffered file operations
on files in /proc to begin with.  A better solution would be to use
internally managed buffers and calls to write() instead of these stdio
calls, but that would be a more extensive change; so this is proposed
as a quick and not-so-dirty fix in the meantime.

Signed-off-by: Sean Finney <sean.finney@sonyericsson.com>
---
 support/include/misc.h    |    3 +++
 utils/gssd/svcgssd_proc.c |    3 +++
 utils/mountd/cache.c      |    2 ++
 3 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/support/include/misc.h b/support/include/misc.h
index 9a1b25d..7e3874e 100644
--- a/support/include/misc.h
+++ b/support/include/misc.h
@@ -24,4 +24,7 @@ struct hostent *get_reliable_hostbyaddr(const char *addr, int len, int type);
 
 extern int is_mountpoint(char *path);
 
+/* size of the file pointer buffers for rpc procfs files */
+#define RPC_CHAN_BUF_SIZE 32768
+
 #endif /* MISC_H */
diff --git a/utils/gssd/svcgssd_proc.c b/utils/gssd/svcgssd_proc.c
index 6f2ba61..8f6548e 100644
--- a/utils/gssd/svcgssd_proc.c
+++ b/utils/gssd/svcgssd_proc.c
@@ -56,6 +56,7 @@
 #include "gss_util.h"
 #include "err_util.h"
 #include "context.h"
+#include "misc.h"
 
 extern char * mech2file(gss_OID mech);
 #define SVCGSSD_CONTEXT_CHANNEL "/proc/net/rpc/auth.rpcsec.context/channel"
@@ -78,6 +79,7 @@ do_svc_downcall(gss_buffer_desc *out_handle, struct svc_cred *cred,
 	FILE *f;
 	int i;
 	char *fname = NULL;
+	char vbuf[RPC_CHAN_BUF_SIZE];
 	int err;
 
 	printerr(1, "doing downcall\n");
@@ -90,6 +92,7 @@ do_svc_downcall(gss_buffer_desc *out_handle, struct svc_cred *cred,
 			     SVCGSSD_CONTEXT_CHANNEL, strerror(errno));
 		goto out_err;
 	}
+	setvbuf(f, vbuf, _IOLBF, RPC_CHAN_BUF_SIZE);
 	qword_printhex(f, out_handle->value, out_handle->length);
 	/* XXX are types OK for the rest of this? */
 	/* For context cache, use the actual context endtime */
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 7deb050..4eded17 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -739,6 +739,7 @@ struct {
 	char *cache_name;
 	void (*cache_handle)(FILE *f);
 	FILE *f;
+	char vbuf[RPC_CHAN_BUF_SIZE];
 } cachelist[] = {
 	{ "auth.unix.ip", auth_unix_ip},
 	{ "auth.unix.gid", auth_unix_gid},
@@ -757,6 +758,7 @@ void cache_open(void)
 			continue;
 		sprintf(path, "/proc/net/rpc/%s/channel", cachelist[i].cache_name);
 		cachelist[i].f = fopen(path, "r+");
+		setvbuf(cachelist[i].f, cachelist[i].vbuf, _IOLBF, RPC_CHAN_BUF_SIZE);
 	}
 }
 
-- 
1.7.4.1

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

[Jim Rees]

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.

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

[Finney, Sean]

SGkgSmltLAoKT24gVGh1LCAyMDExLTA0LTA3IGF0IDA4OjIyIC0wNDAwLCBKaW0gUmVlcyB3cm90
ZToKPiBTZWFuIEZpbm5leSB3cm90ZToKPiAKPiAgIFByZXZpb3VzbHksIGluIGF1dGhfdW5peF9n
aWQsIGdyb3VwIGxpc3RzIHdlcmUgc3RvcmVkIGluIGFuIGFycmF5IG9mCj4gICBoYXJkLWNvZGVk
IGxlbmd0aCAxMDAsIGFuZCBpbiB0aGUgc2l0dWF0aW9uIHRoYXQgdGhlIGdyb3VwIGxpc3RzIGZv
ciBhCj4gICBwYXJ0aWN1bGFyIGNhbGwgd2VyZSB0b28gbGFyZ2UsIHRoZSBhcnJheSB3YXMgc3dh
cHBlZCB3aXRoIGEgZHluYW1pY2FsbHkKPiAgIGFsbG9jYXRlZC9mcmVlZCBidWZmZXIuICBGb3Ig
ZW52aXJvbm1lbnRzIHdoZXJlIHVzZXJzIGFyZSBjb21tb25seSBpbgo+ICAgYSBsYXJnZSBudW1i
ZXIgb2YgZ3JvdXBzLCB0aGlzIGlzbid0IGFuIGlkZWFsIGFwcHJvYWNoLgo+ICAgCj4gICBJbnN0
ZWFkLCBtYWtlIHRoZSBncm91cCBsaXN0IHN0YXRpYyBzY29wZWQgdG8gdGhlIGZ1bmN0aW9uLCBh
bmQKPiAgIHVzZSBtYWxsb2MvcmVhbGxvYyB0byBncm93IGl0IG9uIGFuIGFzLW5lZWRlZCBiYXNp
cy4KPiAKPiBJIHdvdWxkIHJlLXdvcmQgdGhpcy4gIFRoZSBsaXN0IGlzbid0IHN0YXRpYywgdGhl
IGxpc3QgcG9pbnRlciBpcy4gIEkgZG9uJ3QKPiB0aGluayB5b3UgbmVlZCB0byBtZW50aW9uIHRo
YXQsIGl0J3MganVzdCBjb25mdXNpbmcuICAiVXNlIG1hbGxvYy9yZWFsbG9jIHRvCj4gZ3JvdyB0
aGUgbGlzdCBvbiBhbiBhcy1uZWVkZWQgYmFzaXMuIgoKT2theS4gIEkgd2FzIHRyeWluZyB0byBq
dXN0IHB1dCBhIGhpbnQgdGhhdCB0aGVyZSB3YXMgYSBwZXJzaXN0YW50CmJ1ZmZlciB0aGF0IHdv
dWxkIGhhbmcgYXJvdW5kIHRocm91Z2ggbXVsdGlwbGUgaW52b2NhdGlvbnMsIHRob3VnaApwZXJo
YXBzIHRoYXQncyBhIGxpdHRsZSB0b28gbXVjaCAnaW1wbGVtZW50YXRpb24gZGV0YWlscycuICBU
b3RhbGx5IGZpbmUKd2l0aCBtb2RpZnlpbmcgdGhlIGRlc2NyaXB0aW9uIGVpdGhlciB3YXkgOikK
Cgo+IAo+ICAgKwkJZ3JvdXBzID0gbWFsbG9jKHNpemVvZihnaWRfdCkqSU5JVElBTF9NQU5BR0VE
X0dST1VQUyk7Cj4gCj4gU3R5bGUgbml0OiAgUHV0IGJsYW5rcyBhcm91bmQgdGhlICIqIi4KCldp
bGwgZG8uCgo+IEkgd2FzIGdvaW5nIHRvIHN1Z2dlc3QgeW91IGZpcnN0IGNhbGwgZ2V0Z3JvdXBs
aXN0IHdpdGggZ3JvdXBzIHNldCB0byAwIHNvCj4geW91IGNhbiBhbHdheXMgYWxsb2MoKSB0aGUg
Y29ycmVjdCBzaXplIGxpc3QsIGJ1dCB0aGVuIEkgZm91bmQgdGhpcyBpbiB0aGUKPiBtYW4gcGFn
ZToKPiAKPiBCVUdTCj4gICAgICAgIEluICBnbGliYyAgdmVyc2lvbnMgIGJlZm9yZSAgMi4zLjMs
IHRoZSBpbXBsZW1lbnRhdGlvbiBvZiB0aGlzIGZ1bmN0aW9uCj4gICAgICAgIGNvbnRhaW5zIGEg
YnVmZmVyLW92ZXJydW4gYnVnOiBpdCByZXR1cm5zIHRoZSBjb21wbGV0ZSBsaXN0ICBvZiAgZ3Jv
dXBzCj4gICAgICAgIGZvciAgdXNlciAgaW4gIHRoZSBhcnJheSBncm91cHMsIGV2ZW4gd2hlbiB0
aGUgbnVtYmVyIG9mIGdyb3VwcyBleGNlZWRzCj4gICAgICAgICpuZ3JvdXBzLgoKWXVjay4gIFRo
ZSBmdW5jdGlvbiBpcyBhbHNvIG5vdCBpbiBQT1NJWCBvciBhbnkgb3RoZXIgc3RhbmRhcmRzLCBz
bwptYXliZSBpdCdzIHdvcnRoIGRpc2N1c3NpbmcgYXQgYSBsYXRlciBwb2ludCB3aGV0aGVyIGdl
dGdyb3VwcygyKSB3b3VsZApiZSBhIGJldHRlciBpbnRlcmZhY2UgdG8gdXNlLCBldmVuIGlmIGl0
IG1lYW5zIGFuIGV4dHJhIHN0ZXAgb3IgdHdvLgoKCkFueXdheSwgSSB3aWxsIHJlLXNwaW4gdGhp
cyBwYXRjaCB3aXRoIHRoZSB0d28gY2hhbmdlcyB0aGF0IHlvdSd2ZQpzdWdnZXN0ZWQgYW5kIHN1
Ym1pdCBpdCB0b21vcnJvdy4gICBDYXJlIHRvIHRha2UgYSBsb29rIGF0IHBhdGNoCjIvMj8gIDop
CgoKCVNlYW4KTu+/ve+/ve+/ve+/ve+/vXLvv73vv71577+977+977+9Yu+/vVjvv73vv73Hp3bv
v71e77+9Kd66ey5u77+9K++/ve+/ve+/ve+/vXvvv73vv73vv70i77+977+9Xm7vv71y77+977+9
77+9eu+/vRrvv73vv71o77+977+977+977+9Ju+/ve+/vR7vv71H77+977+977+9aO+/vQMo77+9
6ZqO77+93aJqIu+/ve+/vRrvv70bbe+/ve+/ve+/ve+/ve+/vXrvv73elu+/ve+/ve+/vWbvv73v
v73vv71o77+977+977+9fu+/vW3vv70=

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

[Chuck Lever]

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).

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

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

[Jim Rees]

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.