svn commit: r237106 - stable/9/lib/libc/gen

[Konstantin Belousov]

On Fri, Jun 15, 2012 at 05:33:27PM -0500, Guy Helmer wrote:
> 
> 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
> >> 
> >> 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)
> >> 
> > 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.
> > 
> > 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, , )
> > 
> > The http://people.freebsd.org/~kib/misc/netgr.c shows the issue. Run it
> > as "netgr testboxes laptops".
> > 
> > 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.
> > 
> >> @@ -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)) {
> > 
> > Below is partial revert of your changes that cures mountd for me. Also,
> > the second patch does some further cleanup of the getnetgrent.c.
> > 
> > Do you agree ?
> 
> Sorry for the breakage. FWIW, I am having difficulty seeing how:
> 
> 					linep = malloc(olen + len + 1);
> 					?
> 					if (olen > 0) {
> 						bcopy(olinep, linep, olen);
> 						free(olinep);
> 					}
> 
> is different from:
> 
> 					linep = reallocf(linep, olen + len + 1);
> 
The old value of linep is invalid after the call to reallocf() and
must be not used further. But it is saved as l_line value, so it gets
free()d. You de-facto use very long line, containing lines for all
groups, with l_line pointing in the middle of it. It is some luck that
in my case reallocf() was able to extend original chunk, otherwise I
would also get garbage in l_lines.

> 
> 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;".
Fixed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-stable-9/attachments/20120615/ce37908f/attachment.pgp