Bug 83340 - [patch] setnetgrent() and supporting functions don't check malloc for failures
Summary: [patch] setnetgrent() and supporting functions don't check malloc for failures
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 5.4-STABLE
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-07-12 18:30 UTC by Dan Lukes
Modified: 2015-02-20 01:42 UTC (History)
0 users

See Also:


Attachments
patch (2.07 KB, patch)
2005-07-12 18:30 UTC, Dan Lukes
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Lukes 2005-07-12 18:30:15 UTC
	setnetgrent(), parse_netgrp() called from it, read_for_group()
called from parse_netgrp() don't check malloc for failures
Comment 1 ghelmer 2012-01-04 21:17:13 UTC
I have updated the patch a bit to resolve the possibility of a memory =
leak in parse_netgrp() if an ng_str[] element allocation fails, and to =
prevent corrupting the grouphead.gr chain in the event any allocation =
fails. However, I don't have an environment handy to test this so if you =
could check this before I commit it, I would appreciate it.

Index: getnetgrent.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- getnetgrent.c       (revision 229512)
+++ getnetgrent.c       (working copy)
@@ -203,9 +203,7 @@
                        if (parse_netgrp(group))
                                endnetgrent();
                        else {
-                               grouphead.grname =3D (char *)
-                                       malloc(strlen(group) + 1);
-                               strcpy(grouphead.grname, group);
+                               grouphead.grname =3D strdup(group);
                        }
                        if (netf)
                                fclose(netf);
@@ -417,7 +415,7 @@
 parse_netgrp(const char *group)
 {
        char *spos, *epos;
-       int len, strpos;
+       int len, strpos, freepos;
 #ifdef DEBUG
        int fields;
 #endif
@@ -454,9 +452,9 @@
        while (pos !=3D NULL && *pos !=3D '\0') {
                if (*pos =3D=3D '(') {
                        grp =3D (struct netgrp *)malloc(sizeof (struct =
netgrp));
+                       if (grp =3D=3D NULL)
+                               return(1);
                        bzero((char *)grp, sizeof (struct netgrp));
-                       grp->ng_next =3D grouphead.gr;
-                       grouphead.gr =3D grp;
                        pos++;
                        gpos =3D strsep(&pos, ")");
 #ifdef DEBUG
@@ -477,6 +475,13 @@
                                        if (len > 0) {
                                                grp->ng_str[strpos] =3D  =
(char *)
                                                        malloc(len + 1);
+                                               if (grp->ng_str[strpos] =
=3D=3D NULL) {
+                                                       for (freepos =3D =
0; freepos < strpos; freepos++)
+                                                               if =
(grp->ng_str[freepos] !=3D NULL)
+                                                                       =
free(grp->ng_str[freepos]);
+                                                       free(grp);
+                                                       return(1);
+                                               }
                                                bcopy(spos, =
grp->ng_str[strpos],
                                                        len + 1);
                                        }
@@ -490,6 +495,8 @@
                                        grp->ng_str[strpos] =3D NULL;
                                }
                        }
+                       grp->ng_next =3D grouphead.gr;
+                       grouphead.gr =3D grp;
 #ifdef DEBUG
                        /*
                         * Note: on other platforms, malformed netgroup
@@ -526,7 +533,7 @@
 static struct linelist *
 read_for_group(const char *group)
 {
-       char *pos, *spos, *linep, *olinep;
+       char *pos, *spos, *linep;
        int len, olen;
        int cont;
        struct linelist *lp;
@@ -534,6 +541,7 @@
 #ifdef YP
        char *result;
        int resultlen;
+       linep =3D NULL;
=20
        while (_netgr_yp_enabled || fgets(line, LINSIZ, netf) !=3D NULL) =
{
                if (_netgr_yp_enabled) {
@@ -554,6 +562,7 @@
                        free(result);
                }
 #else
+       linep =3D NULL;
        while (fgets(line, LINSIZ, netf) !=3D NULL) {
 #endif
                pos =3D (char *)&line;
@@ -576,8 +585,14 @@
                        pos++;
                if (*pos !=3D '\n' && *pos !=3D '\0') {
                        lp =3D (struct linelist *)malloc(sizeof (*lp));
+                       if (lp =3D=3D NULL)=20
+                               return(NULL);
                        lp->l_parsed =3D 0;
                        lp->l_groupname =3D (char *)malloc(len + 1);
+                       if (lp->l_groupname =3D=3D NULL) {
+                               free(lp);
+                               return(NULL);
+                       }
                        bcopy(spos, lp->l_groupname, len);
                        *(lp->l_groupname + len) =3D '\0';
                        len =3D strlen(pos);
@@ -595,15 +610,15 @@
                                } else
                                        cont =3D 0;
                                if (len > 0) {
-                                       linep =3D (char *)malloc(olen + =
len + 1);
-                                       if (olen > 0) {
-                                               bcopy(olinep, linep, =
olen);
-                                               free(olinep);
+                                       linep =3D (char =
*)reallocf(linep, olen + len + 1);
+                                       if (linep =3D=3D NULL) {
+                                               free(lp->l_groupname);
+                                               free(lp);
+                                               return(NULL);
                                        }
                                        bcopy(pos, linep + olen, len);
                                        olen +=3D len;
                                        *(linep + olen) =3D '\0';
-                                       olinep =3D linep;
                                }
                                if (cont) {
                                        if (fgets(line, LINSIZ, netf)) {
@@ -634,5 +649,5 @@
         */
        rewind(netf);
 #endif
-       return ((struct linelist *)0);
+       return (NULL);
 }


--------
This message has been scanned by ComplianceSafe, powered by Palisade's PacketSure.
Comment 2 Dan Lukes 2012-01-08 09:39:27 UTC
Unfortunately I have no appropriate environment also (nor I had it in 
2005 year - such report has been created because of semi-automatic code 
analysis I did).

But I reviewed your patch (I analyzed it from scratch - 7 years is soo 
long to remember details of my original proposal) and it should work 
good, YMHO.

Dan
Comment 3 dfilter service freebsd_committer 2012-05-21 22:10:55 UTC
Author: ghelmer
Date: Mon May 21 21:10:00 2012
New Revision: 235740
URL: http://svn.freebsd.org/changeset/base/235740

Log:
  Add checks for memory allocation failures in appropriate places, and
  avoid creating bad entries in the grp list as a result of memory allocation
  failures while building new entries.
  
  PR:		bin/83340
  Reviewed by:	delphij (prior version of patch)

Modified:
  head/lib/libc/gen/getnetgrent.c

Modified: head/lib/libc/gen/getnetgrent.c
==============================================================================
--- head/lib/libc/gen/getnetgrent.c	Mon May 21 21:04:29 2012	(r235739)
+++ head/lib/libc/gen/getnetgrent.c	Mon May 21 21:10:00 2012	(r235740)
@@ -203,9 +203,7 @@ setnetgrent(const char *group)
 			if (parse_netgrp(group))
 				endnetgrent();
 			else {
-				grouphead.grname = (char *)
-					malloc(strlen(group) + 1);
-				strcpy(grouphead.grname, group);
+				grouphead.grname = strdup(group);
 			}
 			if (netf)
 				fclose(netf);
@@ -457,9 +455,9 @@ parse_netgrp(const char *group)
 	while (pos != NULL && *pos != '\0') {
 		if (*pos == '(') {
 			grp = (struct netgrp *)malloc(sizeof (struct netgrp));
+			if (grp == NULL)
+				return (1);
 			bzero((char *)grp, sizeof (struct netgrp));
-			grp->ng_next = grouphead.gr;
-			grouphead.gr = grp;
 			pos++;
 			gpos = strsep(&pos, ")");
 #ifdef DEBUG
@@ -480,6 +478,13 @@ parse_netgrp(const char *group)
 					if (len > 0) {
 						grp->ng_str[strpos] =  (char *)
 							malloc(len + 1);
+						if (grp->ng_str[strpos] == NULL) {
+							int freepos;
+							for (freepos = 0; freepos < strpos; freepos++)
+								free(grp->ng_str[freepos]);
+							free(grp);
+							return (1);
+						}
 						bcopy(spos, grp->ng_str[strpos],
 							len + 1);
 					}
@@ -493,6 +498,8 @@ parse_netgrp(const char *group)
 					grp->ng_str[strpos] = NULL;
 				}
 			}
+			grp->ng_next = grouphead.gr;
+			grouphead.gr = grp;
 #ifdef DEBUG
 			/*
 			 * Note: on other platforms, malformed netgroup
@@ -529,7 +536,7 @@ parse_netgrp(const char *group)
 static struct linelist *
 read_for_group(const char *group)
 {
-	char *pos, *spos, *linep, *olinep;
+	char *pos, *spos, *linep;
 	int len, olen;
 	int cont;
 	struct linelist *lp;
@@ -537,6 +544,7 @@ read_for_group(const char *group)
 #ifdef YP
 	char *result;
 	int resultlen;
+	linep = NULL;
 
 	while (_netgr_yp_enabled || fgets(line, LINSIZ, netf) != NULL) {
 		if (_netgr_yp_enabled) {
@@ -557,6 +565,7 @@ read_for_group(const char *group)
 			free(result);
 		}
 #else
+	linep = NULL;
 	while (fgets(line, LINSIZ, netf) != NULL) {
 #endif
 		pos = (char *)&line;
@@ -579,8 +588,14 @@ read_for_group(const char *group)
 			pos++;
 		if (*pos != '\n' && *pos != '\0') {
 			lp = (struct linelist *)malloc(sizeof (*lp));
+			if (lp == NULL) 
+				return (NULL);
 			lp->l_parsed = 0;
 			lp->l_groupname = (char *)malloc(len + 1);
+			if (lp->l_groupname == NULL) {
+				free(lp);
+				return (NULL);
+			}
 			bcopy(spos, lp->l_groupname, len);
 			*(lp->l_groupname + len) = '\0';
 			len = strlen(pos);
@@ -598,15 +613,15 @@ read_for_group(const char *group)
 				} else
 					cont = 0;
 				if (len > 0) {
-					linep = (char *)malloc(olen + len + 1);
-					if (olen > 0) {
-						bcopy(olinep, linep, olen);
-						free(olinep);
+					linep = (char *)reallocf(linep, olen + len + 1);
+					if (linep == NULL) {
+						free(lp->l_groupname);
+						free(lp);
+						return (NULL);
 					}
 					bcopy(pos, linep + olen, len);
 					olen += len;
 					*(linep + olen) = '\0';
-					olinep = linep;
 				}
 				if (cont) {
 					if (fgets(line, LINSIZ, netf)) {
@@ -637,5 +652,5 @@ read_for_group(const char *group)
 	 */
 	rewind(netf);
 #endif
-	return ((struct linelist *)0);
+	return (NULL);
 }
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 4 Guy Helmer freebsd_committer 2012-06-06 18:23:23 UTC
State Changed
From-To: open->closed

Patch applied, thanks.
Comment 5 dfilter service freebsd_committer 2012-06-14 22:48:34 UTC
Author: ghelmer
Date: Thu Jun 14 21:48:14 2012
New Revision: 237106
URL: http://svn.freebsd.org/changeset/base/237106

Log:
  MFC 235739-235740,236402:
  Apply style(9) to return and switch/case statements.
  
  Add checks for memory allocation failures in appropriate places, and
  avoid creating bad entries in the grp list as a result of memory allocation
  failures while building new entries.
  
  Style(9) improvements: remove unnecessary parenthesis, improve order
  of local variable declarations, remove bogus casts, and resolve long
  lines.
  
  PR:		bin/83340

Modified:
  stable/9/lib/libc/gen/getnetgrent.c
Directory Properties:
  stable/9/lib/libc/   (props changed)

Modified: stable/9/lib/libc/gen/getnetgrent.c
==============================================================================
--- stable/9/lib/libc/gen/getnetgrent.c	Thu Jun 14 21:40:14 2012	(r237105)
+++ stable/9/lib/libc/gen/getnetgrent.c	Thu Jun 14 21:48:14 2012	(r237106)
@@ -203,9 +203,7 @@ setnetgrent(const char *group)
 			if (parse_netgrp(group))
 				endnetgrent();
 			else {
-				grouphead.grname = (char *)
-					malloc(strlen(group) + 1);
-				strcpy(grouphead.grname, group);
+				grouphead.grname = strdup(group);
 			}
 			if (netf)
 				fclose(netf);
@@ -292,12 +290,12 @@ _listmatch(const char *list, const char 
 		while(*ptr != ','  && *ptr != '\0' && !isspace((unsigned char)*ptr))
 			ptr++;
 		if (strncmp(cptr, group, glen) == 0 && glen == (ptr - cptr))
-			return(1);
+			return (1);
 		while(*ptr == ','  || isspace((unsigned char)*ptr))
 			ptr++;
 	}
 
-	return(0);
+	return (0);
 }
 
 static int
@@ -311,32 +309,37 @@ _revnetgr_lookup(char* lookupdom, char* 
 
 	for (rot = 0; ; rot++) {
 		switch (rot) {
-			case(0): snprintf(key, MAXHOSTNAMELEN, "%s.%s",
-					  str, dom?dom:lookupdom);
-				 break;
-			case(1): snprintf(key, MAXHOSTNAMELEN, "%s.*",
-					  str);
-				 break;
-			case(2): snprintf(key, MAXHOSTNAMELEN, "*.%s",
-					  dom?dom:lookupdom);
-				 break;
-			case(3): snprintf(key, MAXHOSTNAMELEN, "*.*");
-				 break;
-			default: return(0);
+		case 0:
+			snprintf(key, MAXHOSTNAMELEN, "%s.%s", str,
+			    dom ? dom : lookupdom);
+			break;
+		case 1:
+			snprintf(key, MAXHOSTNAMELEN, "%s.*", str);
+			break;
+		case 2:
+			snprintf(key, MAXHOSTNAMELEN, "*.%s",
+			    dom ? dom : lookupdom);
+			break;
+		case 3:
+			snprintf(key, MAXHOSTNAMELEN, "*.*");
+			break;
+		default:
+			return (0);
 		}
 		y = yp_match(lookupdom, map, key, strlen(key), &result,
-			     &resultlen);
+		    &resultlen);
 		if (y == 0) {
 			rv = _listmatch(result, group, resultlen);
 			free(result);
-			if (rv) return(1);
+			if (rv)
+				return (1);
 		} else if (y != YPERR_KEY) {
 			/*
 			 * If we get an error other than 'no
 			 * such key in map' then something is
 			 * wrong and we should stop the search.
 			 */
-			return(-1);
+			return (-1);
 		}
 	}
 }
@@ -386,14 +389,14 @@ innetgr(const char *group, const char *h
 	if (_use_only_yp && (host == NULL) != (user == NULL)) {
 		int ret;
 		if(yp_get_default_domain(&_netgr_yp_domain))
-			return(0);
+			return (0);
 		ret = _revnetgr_lookup(_netgr_yp_domain, 
 				      host?"netgroup.byhost":"netgroup.byuser",
 				      host?host:user, dom, group);
 		if (ret == 1)
-			return(1);
+			return (1);
 		else if (ret == 0 && dom != NULL)
-			return(0);
+			return (0);
 	}
 
 	setnetgrent(group);
@@ -416,14 +419,14 @@ innetgr(const char *group, const char *h
 static int
 parse_netgrp(const char *group)
 {
-	char *spos, *epos;
-	int len, strpos;
+	struct netgrp *grp;
+	struct linelist *lp = linehead;
+	char **ng;
+	char *epos, *gpos, *pos, *spos;
+	int freepos, len, strpos;
 #ifdef DEBUG
 	int fields;
 #endif
-	char *pos, *gpos;
-	struct netgrp *grp;
-	struct linelist *lp = linehead;
 
 	/*
 	 * First, see if the line has already been read in.
@@ -453,43 +456,51 @@ parse_netgrp(const char *group)
 	/* Watch for null pointer dereferences, dammit! */
 	while (pos != NULL && *pos != '\0') {
 		if (*pos == '(') {
-			grp = (struct netgrp *)malloc(sizeof (struct netgrp));
-			bzero((char *)grp, sizeof (struct netgrp));
-			grp->ng_next = grouphead.gr;
-			grouphead.gr = grp;
+			grp = malloc(sizeof(*grp));
+			if (grp == NULL)
+				return (1);
+			ng = grp->ng_str;
+			bzero(grp, sizeof(*grp));
 			pos++;
 			gpos = strsep(&pos, ")");
 #ifdef DEBUG
 			fields = 0;
 #endif
 			for (strpos = 0; strpos < 3; strpos++) {
-				if ((spos = strsep(&gpos, ","))) {
-#ifdef DEBUG
-					fields++;
-#endif
-					while (*spos == ' ' || *spos == '\t')
-						spos++;
-					if ((epos = strpbrk(spos, " \t"))) {
-						*epos = '\0';
-						len = epos - spos;
-					} else
-						len = strlen(spos);
-					if (len > 0) {
-						grp->ng_str[strpos] =  (char *)
-							malloc(len + 1);
-						bcopy(spos, grp->ng_str[strpos],
-							len + 1);
-					}
-				} else {
+				if ((spos = strsep(&gpos, ",")) == NULL) {
 					/*
 					 * All other systems I've tested
 					 * return NULL for empty netgroup
 					 * fields. It's up to user programs
 					 * to handle the NULLs appropriately.
 					 */
-					grp->ng_str[strpos] = NULL;
+					ng[strpos] = NULL;
+					continue;
 				}
+#ifdef DEBUG
+				fields++;
+#endif
+				while (*spos == ' ' || *spos == '\t')
+					spos++;
+				if ((epos = strpbrk(spos, " \t"))) {
+					*epos = '\0';
+					len = epos - spos;
+				} else
+					len = strlen(spos);
+				if (len <= 0)
+					continue;
+				ng[strpos] = malloc(len + 1);
+				if (ng[strpos] == NULL) {
+					for (freepos = 0; freepos < strpos;
+					    freepos++)
+						free(ng[freepos]);
+					free(grp);
+					return (1);
+				}
+				bcopy(spos, ng[strpos], len + 1);
 			}
+			grp->ng_next = grouphead.gr;
+			grouphead.gr = grp;
 #ifdef DEBUG
 			/*
 			 * Note: on other platforms, malformed netgroup
@@ -497,14 +508,15 @@ parse_netgrp(const char *group)
 			 * can catch bad entries and report them, we should
 			 * stay silent by default for compatibility's sake.
 			 */
-			if (fields < 3)
-					fprintf(stderr, "Bad entry (%s%s%s%s%s) in netgroup \"%s\"\n",
-						grp->ng_str[NG_HOST] == NULL ? "" : grp->ng_str[NG_HOST],
-						grp->ng_str[NG_USER] == NULL ? "" : ",",
-						grp->ng_str[NG_USER] == NULL ? "" : grp->ng_str[NG_USER],
-						grp->ng_str[NG_DOM] == NULL ? "" : ",",
-						grp->ng_str[NG_DOM] == NULL ? "" : grp->ng_str[NG_DOM],
-						lp->l_groupname);
+			if (fields < 3) {
+				fprintf(stderr,
+				"Bad entry (%s%s%s%s%s) in netgroup \"%s\"\n",
+				    ng[NG_HOST] == NULL ? "" : ng[NG_HOST],
+				    ng[NG_USER] == NULL ? "" : ",",
+				    ng[NG_USER] == NULL ? "" : ng[NG_USER],
+				    ng[NG_DOM] == NULL ? "" : ",",
+				    ng[NG_DOM] == NULL ? "" : ng[NG_DOM],
+				    lp->l_groupname);
 #endif
 		} else {
 			spos = strsep(&pos, ", \t");
@@ -526,7 +538,7 @@ parse_netgrp(const char *group)
 static struct linelist *
 read_for_group(const char *group)
 {
-	char *pos, *spos, *linep, *olinep;
+	char *pos, *spos, *linep;
 	int len, olen;
 	int cont;
 	struct linelist *lp;
@@ -534,6 +546,7 @@ read_for_group(const char *group)
 #ifdef YP
 	char *result;
 	int resultlen;
+	linep = NULL;
 
 	while (_netgr_yp_enabled || fgets(line, LINSIZ, netf) != NULL) {
 		if (_netgr_yp_enabled) {
@@ -541,7 +554,7 @@ read_for_group(const char *group)
 				if(yp_get_default_domain(&_netgr_yp_domain))
 					continue;
 			if (yp_match(_netgr_yp_domain, "netgroup", group,
-					strlen(group), &result, &resultlen)) {
+			    strlen(group), &result, &resultlen)) {
 				free(result);
 				if (_use_only_yp)
 					return ((struct linelist *)0);
@@ -554,6 +567,7 @@ read_for_group(const char *group)
 			free(result);
 		}
 #else
+	linep = NULL;
 	while (fgets(line, LINSIZ, netf) != NULL) {
 #endif
 		pos = (char *)&line;
@@ -576,8 +590,14 @@ read_for_group(const char *group)
 			pos++;
 		if (*pos != '\n' && *pos != '\0') {
 			lp = (struct linelist *)malloc(sizeof (*lp));
+			if (lp == NULL) 
+				return (NULL);
 			lp->l_parsed = 0;
 			lp->l_groupname = (char *)malloc(len + 1);
+			if (lp->l_groupname == NULL) {
+				free(lp);
+				return (NULL);
+			}
 			bcopy(spos, lp->l_groupname, len);
 			*(lp->l_groupname + len) = '\0';
 			len = strlen(pos);
@@ -595,15 +615,15 @@ read_for_group(const char *group)
 				} else
 					cont = 0;
 				if (len > 0) {
-					linep = (char *)malloc(olen + len + 1);
-					if (olen > 0) {
-						bcopy(olinep, linep, olen);
-						free(olinep);
+					linep = reallocf(linep, olen + len + 1);
+					if (linep == NULL) {
+						free(lp->l_groupname);
+						free(lp);
+						return (NULL);
 					}
 					bcopy(pos, linep + olen, len);
 					olen += len;
 					*(linep + olen) = '\0';
-					olinep = linep;
 				}
 				if (cont) {
 					if (fgets(line, LINSIZ, netf)) {
@@ -634,5 +654,5 @@ read_for_group(const char *group)
 	 */
 	rewind(netf);
 #endif
-	return ((struct linelist *)0);
+	return (NULL);
 }
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"