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 <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar · Jan 24, 2025 · de941a7 · de941a7
1 parent e048ba4
commit de941a7
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 35 deletions.
diff --git 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
@@ -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]) {