From: Alejandro Colomar <alx@kernel.org>
Date: Thu, 14 Nov 2024 13:39:23 +0000 (+0100)
Subject: lib/, src/: Simplify allocation of buffer
X-Git-Tag: 4.17.3~50
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=de941a7601f8410a1b918c61eec02ab07263eee5;p=thirdparty%2Fshadow.git

lib/, src/: Simplify allocation of buffer

getgroups(0, NULL) returns the number of groups, so that we can allocate
at once.  This might fail if there's a race and the number of users
grows while we're allocating, but if that happens, failing is probably a
good thing to do.

There was some comment saying it doesn't work on some systems, but
according to gnulib, that's only NeXTstep 3.2, which we don't support.

Link: <https://www.gnu.org/software/gnulib/manual/html_node/getgroups.html>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---

diff --git a/lib/addgrps.c b/lib/addgrps.c
index 809b8e702..8a1667c1e 100644
--- a/lib/addgrps.c
+++ b/lib/addgrps.c
@@ -46,21 +46,17 @@ add_groups(const char *list)
 	}
 	strcpy (buf, list);
 
-	for (size_t i = 16; /* void */; i *= 2) {
-		grouplist = MALLOC(i, GETGROUPS_T);
-		if (NULL == grouplist) {
-			return -1;
-		}
-		ngroups = getgroups (i, grouplist);
-		if (ngroups == -1 && errno != EINVAL) {
-			free(grouplist);
-			return -1;
-		}
-		if (i > (size_t)ngroups) {
-			break;
-		}
-		free (grouplist);
-	}
+	ngroups = getgroups(0, NULL);
+	if (ngroups == -1)
+		return -1;
+
+	grouplist = MALLOC(ngroups, GETGROUPS_T);
+	if (grouplist == NULL)
+		return -1;
+
+	ngroups = getgroups(ngroups, grouplist);
+	if (ngroups == -1)
+		goto free_gids;
 
 	added = false;
 	p = buf;
@@ -103,6 +99,10 @@ add_groups(const char *list)
 
 	free (grouplist);
 	return 0;
+
+free_gids:
+	free(grouplist);
+	return -1;
 }
 #else				/* !USE_PAM */
 extern int ISO_C_forbids_an_empty_translation_unit;
diff --git a/src/newgrp.c b/src/newgrp.c
index e5dc2290b..823d2fe0a 100644
--- a/src/newgrp.c
+++ b/src/newgrp.c
@@ -554,28 +554,28 @@ int main (int argc, char **argv)
 	 * nasty message but at least your real and effective group ids are
 	 * set.
 	 */
-	/* don't use getgroups(0, 0) - it doesn't work on some systems */
-	for (int i = 16; /* void */; i *= 2) {
-		grouplist = XMALLOC(i, GETGROUPS_T);
-		ngroups = getgroups (i, grouplist);
-		if (ngroups == -1 && errno != EINVAL) {
-			perror("getgroups");
+
+	ngroups = getgroups(0, NULL);
+	if (ngroups == -1)
+		goto fail_gg;
+
+	grouplist = XMALLOC(ngroups, GETGROUPS_T);
+
+	ngroups = getgroups(ngroups, grouplist);
+	if (ngroups == -1) {
+fail_gg:
+		perror("getgroups");
 #ifdef WITH_AUDIT
-			if (group) {
-				SNPRINTF(audit_buf, "changing new-group=%s", group);
-				audit_logger(AUDIT_CHGRP_ID, Prog,
-					     audit_buf, NULL, getuid(), 0);
-			} else {
-				audit_logger(AUDIT_CHGRP_ID, Prog,
-					     "changing", NULL, getuid(), 0);
-			}
-#endif
-			exit(EXIT_FAILURE);
-		}
-		if (i > (size_t)ngroups) {
-			break;
+		if (group) {
+			SNPRINTF(audit_buf, "changing new-group=%s", group);
+			audit_logger(AUDIT_CHGRP_ID, Prog,
+				     audit_buf, NULL, getuid(), 0);
+		} else {
+			audit_logger(AUDIT_CHGRP_ID, Prog,
+				     "changing", NULL, getuid(), 0);
 		}
-		free (grouplist);
+#endif
+		exit(EXIT_FAILURE);
 	}
 
 	/*
@@ -685,6 +685,8 @@ int main (int argc, char **argv)
 	 * If the group doesn't fit, I'll complain loudly and skip this
 	 * part.
 	 */
+	grouplist = XREALLOC(grouplist, ngroups + 1, GETGROUPS_T);
+
 	int i;
 	for (i = 0; i < ngroups; i++) {
 		if (gid == grouplist[i]) {