git: e87848a8150e - main - mountd(8): Allow to pass {NGROUPS_MAX} + 1 groups

From: Olivier Certner <olce_at_FreeBSD.org>
Date: Mon, 16 Dec 2024 14:45:31 UTC
The branch main has been updated by olce:

URL: https://cgit.FreeBSD.org/src/commit/?id=e87848a8150ed75da29d99a7d0c0bba6cc5129b8

commit e87848a8150ed75da29d99a7d0c0bba6cc5129b8
Author:     Olivier Certner <olce@FreeBSD.org>
AuthorDate: 2024-10-08 12:30:03 +0000
Commit:     Olivier Certner <olce@FreeBSD.org>
CommitDate: 2024-12-16 14:42:31 +0000

    mountd(8): Allow to pass {NGROUPS_MAX} + 1 groups
    
    NGROUPS_MAX is just the minimum maximum of the number of allowed
    supplementary groups.  The actual runtime value may be greater.  Allow
    more groups to be specified accordingly (now that, a few commits ago,
    nmount(2) has been changed similarly).
    
    To this end, we just allocate once and for all a static array called
    'tmp_groups' big enough to hold {NGROUPS_MAX} + 1 groups (the maximum
    number of supplementary groups plus the effective GID, which we store in
    a variable named 'tngroups_max' to avoid confusion with the kernel
    variable 'ngroups_max' holding only the maximum number of
    *supplementary* groups) in main() and use this temporary space in
    get_exportlist_one(), do_opt() and parsecred().  Doing so in passing
    fixes a (benign) memory leak in case "-maproot" and/or "-mapall" were
    specified multiple times and the first option comprised more than
    SMALLNGROUPS.
    
    parsecred() does not use 'cr_smallgrps' anymore, but we have kept
    'cr_smallgrps'/SMALLNGROUPS as 'struct expcred' is also included in
    'struct exportlist' and 'struct grouplist', and thus this preallocated
    field still results in an optimization for the common case of small
    number of groups (although its real impact is probably negligible and
    arguably was not worth the trouble).
    
    While here, in do_mount(), remove some unnecessary groups array
    allocation and copying.
    
    Reviewed by:    rmacklem (older version)
    Approved by:    markj (mentor)
    MFC after:      2 weeks
    Relnotes:       yes
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D47016
---
 usr.sbin/mountd/mountd.c | 70 +++++++++++++++++++++++++-----------------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/usr.sbin/mountd/mountd.c b/usr.sbin/mountd/mountd.c
index 27d22ba06fa4..cb87535f6c3b 100644
--- a/usr.sbin/mountd/mountd.c
+++ b/usr.sbin/mountd/mountd.c
@@ -57,6 +57,7 @@
 
 #include <arpa/inet.h>
 
+#include <assert.h>
 #include <ctype.h>
 #include <err.h>
 #include <errno.h>
@@ -245,7 +246,7 @@ static void	huphandler(int sig);
 static int	makemask(struct sockaddr_storage *ssp, int bitlen);
 static void	mntsrv(struct svc_req *, SVCXPRT *);
 static void	nextfield(char **, char **);
-static void	out_of_mem(void);
+static void	out_of_mem(void) __dead2;
 static void	parsecred(char *, struct expcred *);
 static int	parsesec(char *, struct exportlist *);
 static int	put_exlist(struct dirlist *, XDR *, struct dirlist *,
@@ -302,6 +303,11 @@ static int has_publicfh = 0;
 static int has_set_publicfh = 0;
 
 static struct pidfh *pfh = NULL;
+
+/* Temporary storage for credentials' groups. */
+static long tngroups_max;
+static gid_t *tmp_groups = NULL;
+
 /* Bits for opt_flags above */
 #define	OP_MAPROOT	0x01
 #define	OP_MAPALL	0x02
@@ -434,6 +440,18 @@ main(int argc, char **argv)
 		warn("cannot open or create pidfile");
 	}
 
+	openlog("mountd", LOG_PID, LOG_DAEMON);
+
+	/* How many groups do we support? */
+	tngroups_max = sysconf(_SC_NGROUPS_MAX);
+	if (tngroups_max == -1)
+		tngroups_max = NGROUPS_MAX;
+	/* Add space for the effective GID. */
+	++tngroups_max;
+	tmp_groups = malloc(tngroups_max);
+	if (tmp_groups == NULL)
+		out_of_mem();
+
 	s = socket(AF_INET6, SOCK_DGRAM, IPPROTO_UDP);
 	if (s < 0)
 		have_v6 = 0;
@@ -539,7 +557,6 @@ main(int argc, char **argv)
 		exnames = argv;
 	else
 		exnames = exnames_default;
-	openlog("mountd", LOG_PID, LOG_DAEMON);
 	if (debug)
 		warnx("getting export list");
 	get_exportlist(0);
@@ -1571,9 +1588,9 @@ get_exportlist_one(int passno)
 	int unvis_len;
 
 	v4root_phase = 0;
-	anon.cr_groups = NULL;
 	dirhead = (struct dirlist *)NULL;
 	unvis_dir[0] = '\0';
+
 	while (get_line()) {
 		if (debug)
 			warnx("got line %s", line);
@@ -1586,9 +1603,9 @@ get_exportlist_one(int passno)
 		 * Set defaults.
 		 */
 		has_host = FALSE;
-		anon.cr_groups = anon.cr_smallgrps;
 		anon.cr_uid = UID_NOBODY;
 		anon.cr_ngroups = 1;
+		anon.cr_groups = tmp_groups;
 		anon.cr_groups[0] = nogroup();
 		exflags = MNT_EXPORTED;
 		got_nondir = 0;
@@ -1918,10 +1935,6 @@ nextline:
 			free_dir(dirhead);
 			dirhead = (struct dirlist *)NULL;
 		}
-		if (anon.cr_groups != anon.cr_smallgrps) {
-			free(anon.cr_groups);
-			anon.cr_groups = NULL;
-		}
 	}
 }
 
@@ -3187,11 +3200,11 @@ do_mount(struct exportlist *ep, struct grouplist *grp, uint64_t exflags,
 	eap->ex_flags = exflags;
 	eap->ex_uid = anoncrp->cr_uid;
 	eap->ex_ngroups = anoncrp->cr_ngroups;
-	if (eap->ex_ngroups > 0) {
-		eap->ex_groups = malloc(eap->ex_ngroups * sizeof(gid_t));
-		memcpy(eap->ex_groups, anoncrp->cr_groups, eap->ex_ngroups *
-		    sizeof(gid_t));
-	}
+	/*
+	 * Use the memory pointed to by 'anoncrp', as it outlives 'eap' which is
+	 * local to this function.
+	 */
+	eap->ex_groups = anoncrp->cr_groups;
 	LOGDEBUG("do_mount exflags=0x%jx", (uintmax_t)exflags);
 	eap->ex_indexfile = ep->ex_indexfile;
 	if (grp->gr_type == GT_HOST)
@@ -3381,7 +3394,6 @@ skip:
 	if (cp)
 		*cp = savedc;
 error_exit:
-	free(eap->ex_groups);
 	/* free strings allocated by strdup() in getmntopts.c */
 	if (iov != NULL) {
 		free(iov[0].iov_base); /* fstype */
@@ -3609,21 +3621,19 @@ get_line(void)
  * Parse a description of a credential.
  */
 static void
-parsecred(char *namelist, struct expcred *cr)
+parsecred(char *names, struct expcred *cr)
 {
 	char *name;
-	char *names;
 	struct passwd *pw;
-	gid_t groups[NGROUPS_MAX + 1];
-	int ngroups;
 	unsigned long name_ul;
 	char *end = NULL;
 
+	assert(cr->cr_groups == tmp_groups);
+
 	/*
 	 * Parse the user and if possible get its password table entry.
 	 * 'cr_uid' is filled when exiting this block.
 	 */
-	names = namelist;
 	name = strsep_quote(&names, ":");
 	name_ul = strtoul(name, &end, 10);
 	if (*end != '\0' || end == name)
@@ -3650,17 +3660,13 @@ parsecred(char *namelist, struct expcred *cr)
 			    "can't determine groups", name);
 			goto nogroup;
 		}
-		cr->cr_uid = pw->pw_uid;
-		ngroups = NGROUPS_MAX + 1;
-		if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups)) {
+
+		cr->cr_ngroups = tngroups_max;
+		if (getgrouplist(pw->pw_name, pw->pw_gid,
+		    cr->cr_groups, &cr->cr_ngroups) != 0) {
 			syslog(LOG_ERR, "too many groups");
-			ngroups = NGROUPS_MAX + 1;
+			cr->cr_ngroups = tngroups_max;
 		}
-
-		if (ngroups > SMALLNGROUPS)
-			cr->cr_groups = malloc(ngroups * sizeof(gid_t));
-		cr->cr_ngroups = ngroups;
-		memcpy(cr->cr_groups, groups, ngroups * sizeof(gid_t));
 		return;
 	}
 
@@ -3684,17 +3690,14 @@ parsecred(char *namelist, struct expcred *cr)
 		} else {
 			group = name_ul;
 		}
-		if (cr->cr_ngroups == NGROUPS_MAX + 1) {
+		if (cr->cr_ngroups == tngroups_max) {
 			syslog(LOG_ERR, "too many groups");
 			break;
 		}
-		groups[cr->cr_ngroups++] = group;
+		cr->cr_groups[cr->cr_ngroups++] = group;
 	}
 	if (cr->cr_ngroups == 0)
 		goto nogroup;
-	if (cr->cr_ngroups > SMALLNGROUPS)
-		cr->cr_groups = malloc(cr->cr_ngroups * sizeof(gid_t));
-	memcpy(cr->cr_groups, groups, cr->cr_ngroups * sizeof(gid_t));
 	return;
 
 nogroup:
@@ -4063,6 +4066,7 @@ huphandler(int sig __unused)
 static void
 terminate(int sig __unused)
 {
+	free(tmp_groups);
 	pidfile_remove(pfh);
 	rpcb_unset(MOUNTPROG, MOUNTVERS, NULL);
 	rpcb_unset(MOUNTPROG, MOUNTVERS3, NULL);