[patch] samba-3.0.2: sys_getgrouplist_internals(..., gid, groups, ...) returns multiple instances of gid in groups

[Buck Huppmann]

on Solaris 9, anyway

before patch:
[2004/01/23 11:18:18, 5] auth/auth_util.c:debug_unix_user_token(505)
  UNIX token of user 40535
  Primary group is 118 and contains 4 supplementary groups
  Group[  0]: 118
  Group[  1]: 118
  Group[  2]: 18
  Group[  3]: 70

after:
[2004/02/11 17:55:57, 5] auth/auth_util.c:debug_unix_user_token(505)
  UNIX token of user 40535
  Primary group is 118 and contains 3 supplementary groups
  Group[  0]: 118
  Group[  1]: 18
  Group[  2]: 70

which is more than a cosmetic problem, if you're in 16 groups, 'cause
then calls to setgroups() fail in set_sec_ctx(), pop_sec_ctx() etc.
(and those functions don't report the error, leaving you scratching
your head when permissions problems bite)

but the patch is included for illustrative purposes only; please
rewrite, especially since this has only been tested on one system

nevertheless, samba 3 is off to a great start. thanks, you all

--buck, maryland, usa

--- samba-3.0.2/source/lib/system_smbd.c.orig	Fri Oct 10 14:08:35 2003
+++ samba-3.0.2/source/lib/system_smbd.c	Wed Feb 11 17:18:37 2004
@@ -38,7 +38,7 @@
   */
 static int getgrouplist_internals(const char *user, gid_t gid, gid_t *groups, int *grpcnt)
 {
-	gid_t *gids_saved;
+	gid_t *gids_saved, *groups_tmp;
 	int ret, ngrp_saved, num_gids;
 
 	if (non_root_mode()) {
@@ -79,16 +79,82 @@
 	setgid(gid);
 
 	num_gids = getgroups(0, NULL);
-	if (num_gids + 1 > *grpcnt) {
-		*grpcnt = num_gids + 1;
-		ret = -1;
-	} else {
-		ret = getgroups(*grpcnt - 1, &groups[1]);
-		if (ret >= 0) {
-			groups[0] = gid;
-			*grpcnt = ret + 1;
+	
+	/* see if we can use the buffer in place */
+	if (num_gids <= *grpcnt) {
+		groups_tmp = groups;
+	}
+	/* otherwise, allocate a buffer big enough so we can getgroups()
+	 * and see whether or not gid is among them, so we can return the
+	 * correct grpcnt (num_gids or num_gids + 1) */
+	else {
+		groups_tmp = (gid_t *)malloc(sizeof(gid_t) * (num_gids));
+		if (!groups_tmp) {
+			errno = ENOMEM;
+			ret = -1;
+			goto restore_gid;
+		}
+	}
+	ret = getgroups(num_gids, groups_tmp);
+	if (ret >= 0) {
+		int i;
+		int saw_gid = False;
+		/* just in case this has changed in the interim; can only
+		 * down-size, though */
+		num_gids = ret < num_gids ? ret : num_gids;
+		for (i = 0; i < num_gids; i++) {
+			if (groups_tmp[i] == gid) {
+				saw_gid = True;
+				break;
+			}
 		}
+		/* if the buffer wasn't big enough and we were just
+		 * counting groups, return the number we found we
+		 * needed */
+		if (groups_tmp != groups) {
+			if (!saw_gid) {
+				/* one more for the gid arg, since it wasn't
+				 * in the list */
+				num_gids++;
+			}
+			/* unlikely: if (num_gids now <= *grpcnt) forge-ahead; */
+			free(groups_tmp);
+			/* cse: *grpcnt = num_gids; is below */
+			ret = -1;
+		}
+		else {
+			if (saw_gid) {
+				if (i != 0) {
+					/* pull gid arg up to front */
+					memmove(&groups[1], &groups[0],
+						sizeof(gid_t) * i);
+					groups[0] = gid;
+				}
+				/* cse: *grpcnt = num_gids; is below */
+			}
+			else {
+				num_gids++;
+				ret++;
+				/* cse: *grpcnt = num_gids; is below */
+				if (num_gids > *grpcnt) {
+					ret = -1;
+				}
+				else {
+					/* avoid 0-length memmove() */
+					if (num_gids > 1) {
+						memmove(&groups[1],
+							&groups[0],
+							sizeof(gid_t)
+							* (num_gids - 1));
+					}
+					groups[0] = gid;
+				}
+			}
+		}
+		*grpcnt = num_gids;
 	}
+
+restore_gid:
 
 	restore_re_gid();