Date: Fri, 15 Jun 2012 17:33:27 -0500 From: Guy Helmer <ghelmer@palisadesystems.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: svn-src-stable@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, svn-src-stable-9@freebsd.org Subject: Re: svn commit: r237106 - stable/9/lib/libc/gen Message-ID: <880C698D-7620-48CB-B0EB-68BBC7FECDBB@palisadesystems.com> In-Reply-To: <20120615212651.GH2337@deviant.kiev.zoral.com.ua> References: <201206142148.q5ELmFul023002@svn.freebsd.org> <20120615212651.GH2337@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Jun 15, 2012, at 4:26 PM, Konstantin Belousov wrote: > On Thu, Jun 14, 2012 at 09:48:15PM +0000, Guy Helmer wrote: >> Author: ghelmer >> Date: Thu Jun 14 21:48:14 2012 >> New Revision: 237106 >> URL: http://svn.freebsd.org/changeset/base/237106 >>=20 >> Log: >> MFC 235739-235740,236402: >> Apply style(9) to return and switch/case statements. >>=20 >> 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. >>=20 >> Style(9) improvements: remove unnecessary parenthesis, improve order >> of local variable declarations, remove bogus casts, and resolve long >> lines. >>=20 >> PR: bin/83340 >>=20 >> Modified: >> stable/9/lib/libc/gen/getnetgrent.c >> Directory Properties: >> stable/9/lib/libc/ (props changed) >>=20 > This broke mountd(8) for me. It dies with SIGSEGV on start. Autopsy = has > shown that this happen due to free(3) on an unallocated pointer. >=20 > I have two netgroups in my /etc/netgroup: > testboxes (solo.home, , ) (x.home, , ) (nv.home, , ) (sandy.home, , ) = (tom.home, ,) > laptops (alf.home, , ) (alf.home-air, , ) (sirion.home, , ) = (sirion.home-air, , ) >=20 > The http://people.freebsd.org/~kib/misc/netgr.c shows the issue. Run = it > as "netgr testboxes laptops". >=20 > Your change below makes lp->l_lines pointer for already processed = groups > invalid because you reallocf(3) the memory pointed by it. Then > 1. accessing the l_lines later causes undefined behaviour; > 2. free(3) call in endnetgrent() dies because pointer is dandling. >=20 >> @@ -595,15 +615,15 @@ read_for_group(const char *group) >> } 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 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)) { >=20 > Below is partial revert of your changes that cures mountd for me. = Also, > the second patch does some further cleanup of the getnetgrent.c. >=20 > Do you agree ? Sorry for the breakage. FWIW, I am having difficulty seeing how: linep =3D malloc(olen + len + = 1); =85 if (olen > 0) { bcopy(olinep, linep, = olen); free(olinep); } is different from: linep =3D reallocf(linep, olen + = len + 1); but if it fixes the issue, it is good. I would beware the disorder in = the sorting of the variables in the line: char *pos, *spos, *linep, *olinep;". Thank you for researching and resolving the issue. Guy >=20 > commit d5cb6720e8d12b281e89f2487561fe95addd0e34 > Author: Konstantin Belousov <kib@freebsd.org> > Date: Sat Jun 16 00:21:11 2012 +0300 >=20 > Partially revert r235740, namely, allocate separate blocks of = memory > for each group' l_line. Using the reallocf(3) invalidates l_line's = of > the previously read groups. >=20 > diff --git a/lib/libc/gen/getnetgrent.c b/lib/libc/gen/getnetgrent.c > index 933a7d3..8cad6c4 100644 > --- a/lib/libc/gen/getnetgrent.c > +++ b/lib/libc/gen/getnetgrent.c > @@ -538,7 +538,7 @@ parse_netgrp(const char *group) > static struct linelist * > read_for_group(const char *group) > { > - char *pos, *spos, *linep; > + char *pos, *spos, *linep, *olinep; > int len, olen; > int cont; > struct linelist *lp; > @@ -615,15 +615,20 @@ read_for_group(const char *group) > } else > cont =3D 0; > if (len > 0) { > - linep =3D reallocf(linep, olen + = len + 1); > + linep =3D malloc(olen + len + = 1); > if (linep =3D=3D NULL) { > free(lp->l_groupname); > free(lp); > return (NULL); > } > + if (olen > 0) { > + bcopy(olinep, linep, = olen); > + free(olinep); > + } > bcopy(pos, linep + olen, len); > olen +=3D len; > *(linep + olen) =3D '\0'; > + olinep =3D linep; > } > if (cont) { > if (fgets(line, LINSIZ, netf)) { >=20 >=20 > commit 5946753b4b094d5f7ac41792df64915434567fd8 > Author: Konstantin Belousov <kib@freebsd.org> > Date: Sat Jun 16 00:23:01 2012 +0300 >=20 > More style. >=20 > diff --git a/lib/libc/gen/getnetgrent.c b/lib/libc/gen/getnetgrent.c > index 8cad6c4..d94fff7 100644 > --- a/lib/libc/gen/getnetgrent.c > +++ b/lib/libc/gen/getnetgrent.c > @@ -161,8 +161,7 @@ setnetgrent(const char *group) > if (group =3D=3D NULL || !strlen(group)) > return; >=20 > - if (grouphead.gr =3D=3D (struct netgrp *)0 || > - strcmp(group, grouphead.grname)) { > + if (grouphead.gr =3D=3D NULL || strcmp(group, grouphead.grname)) = { > endnetgrent(); > #ifdef YP > /* Presumed guilty until proven innocent. */ > @@ -172,7 +171,7 @@ setnetgrent(const char *group) > * use NIS exclusively. > */ > if (((stat(_PATH_NETGROUP, &_yp_statp) < 0) && > - errno =3D=3D ENOENT) || _yp_statp.st_size =3D=3D = 0) > + errno =3D=3D ENOENT) || _yp_statp.st_size =3D=3D 0) > _use_only_yp =3D _netgr_yp_enabled =3D 1; > if ((netf =3D fopen(_PATH_NETGROUP,"r")) !=3D NULL = ||_use_only_yp){ > /* > @@ -247,27 +246,24 @@ endnetgrent(void) > lp =3D lp->l_next; > free(olp->l_groupname); > free(olp->l_line); > - free((char *)olp); > + free(olp); > } > - linehead =3D (struct linelist *)0; > + linehead =3D NULL; > if (grouphead.grname) { > free(grouphead.grname); > - grouphead.grname =3D (char *)0; > + grouphead.grname =3D NULL; > } > gp =3D grouphead.gr; > while (gp) { > ogp =3D gp; > gp =3D gp->ng_next; > - if (ogp->ng_str[NG_HOST]) > - free(ogp->ng_str[NG_HOST]); > - if (ogp->ng_str[NG_USER]) > - free(ogp->ng_str[NG_USER]); > - if (ogp->ng_str[NG_DOM]) > - free(ogp->ng_str[NG_DOM]); > - free((char *)ogp); > + free(ogp->ng_str[NG_HOST]); > + free(ogp->ng_str[NG_USER]); > + free(ogp->ng_str[NG_DOM]); > + free(ogp); > } > - grouphead.gr =3D (struct netgrp *)0; > - nextgrp =3D (struct netgrp *)0; > + grouphead.gr =3D NULL; > + nextgrp =3D NULL; > #ifdef YP > _netgr_yp_enabled =3D 0; > #endif > @@ -282,7 +278,7 @@ _listmatch(const char *list, const char *group, = int len) > int glen =3D strlen(group); >=20 > /* skip possible leading whitespace */ > - while(isspace((unsigned char)*ptr)) > + while (isspace((unsigned char)*ptr)) > ptr++; >=20 > while (ptr < list + len) { > @@ -291,7 +287,7 @@ _listmatch(const char *list, const char *group, = int len) > ptr++; > if (strncmp(cptr, group, glen) =3D=3D 0 && glen =3D=3D = (ptr - cptr)) > return (1); > - while(*ptr =3D=3D ',' || isspace((unsigned char)*ptr)) > + while (*ptr =3D=3D ',' || isspace((unsigned char)*ptr)) > ptr++; > } >=20 > @@ -436,8 +432,7 @@ parse_netgrp(const char *group) > break; > lp =3D lp->l_next; > } > - if (lp =3D=3D (struct linelist *)0 && > - (lp =3D read_for_group(group)) =3D=3D (struct linelist *)0) > + if (lp =3D=3D NULL && (lp =3D read_for_group(group)) =3D=3D = NULL) > return (1); > if (lp->l_parsed) { > #ifdef DEBUG
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?880C698D-7620-48CB-B0EB-68BBC7FECDBB>