Bug 83340
| Summary: | [patch] setnetgrent() and supporting functions don't check malloc for failures | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Dan Lukes <dan> | ||||
| Component: | bin | Assignee: | freebsd-bugs (Nobody) <bugs> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | ||||||
| Priority: | Normal | ||||||
| Version: | 5.4-STABLE | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
|
Description
Dan Lukes
2005-07-12 18:30:15 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.
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 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" State Changed From-To: open->closed Patch applied, thanks. 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" |
