From owner-svn-src-stable-9@FreeBSD.ORG Fri Jun 15 22:49:19 2012 Return-Path: Delivered-To: svn-src-stable-9@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B67691065677; Fri, 15 Jun 2012 22:49:19 +0000 (UTC) (envelope-from ghelmer@palisadesystems.com) Received: from mail.palisadesystems.com (mail.palisadesystems.com [216.81.178.129]) by mx1.freebsd.org (Postfix) with ESMTP id 6CC988FC14; Fri, 15 Jun 2012 22:49:19 +0000 (UTC) Received: from [192.168.1.107] (173-25-206-151.client.mchsi.com [173.25.206.151]) (authenticated bits=0) by mail.palisadesystems.com (8.14.3/8.14.3) with ESMTP id q5FMXRLt036836 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Fri, 15 Jun 2012 17:33:27 -0500 (CDT) (envelope-from ghelmer@palisadesystems.com) X-DKIM: Sendmail DKIM Filter v2.8.3 mail.palisadesystems.com q5FMXRLt036836 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=palisadesystems.com; s=mail; t=1339799608; bh=yoBitkjwxK1TY+PxvpZnl1H4Zvpybj3K6cfAErUHLzM=; l=128; h=Subject:Mime-Version:Content-Type:From:In-Reply-To:Date:Cc: Content-Transfer-Encoding:Message-Id:References:To; b=iuSFZISkdLjkeRGyxcpFx8D4Gc3YWCsDraVTpT0hzW8XFvWXHiWe2NPIREXwpqF0Q xq7KVrg/XrZPo1M9tlPa6FPm9Jv9Jea0WqAYJdTvPsx201rn74gh9UMGHldxUocnUb 4iHtnbkIK0UtaoGFfE6svzQwx6ytF8hWDbSQReM8= Mime-Version: 1.0 (Apple Message framework v1278) Content-Type: text/plain; charset=windows-1252 From: Guy Helmer In-Reply-To: <20120615212651.GH2337@deviant.kiev.zoral.com.ua> Date: Fri, 15 Jun 2012 17:33:27 -0500 Content-Transfer-Encoding: quoted-printable Message-Id: <880C698D-7620-48CB-B0EB-68BBC7FECDBB@palisadesystems.com> References: <201206142148.q5ELmFul023002@svn.freebsd.org> <20120615212651.GH2337@deviant.kiev.zoral.com.ua> To: Konstantin Belousov X-Mailer: Apple Mail (2.1278) X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.5 (mail.palisadesystems.com [172.16.1.5]); Fri, 15 Jun 2012 17:33:28 -0500 (CDT) X-Palisade-MailScanner-Information: Please contact the ISP for more information X-Palisade-MailScanner-ID: q5FMXRLt036836 X-Palisade-MailScanner: Found to be clean X-Palisade-MailScanner-SpamCheck: not spam, SpamAssassin (score=2.084, required 5, ALL_TRUSTED -1.00, BAYES_00 -1.90, J_CHICKENPOX_53 0.60, J_CHICKENPOX_56 0.60, J_CHICKENPOX_64 0.60, J_CHICKENPOX_65 0.60, J_CHICKENPOX_92 0.60, RP_8BIT 1.98) X-Palisade-MailScanner-SpamScore: ss X-Palisade-MailScanner-From: ghelmer@palisadesystems.com X-Spam-Status: No 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 X-BeenThere: svn-src-stable-9@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for only the 9-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Jun 2012 22:49:19 -0000 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 > 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 > 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