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);
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
>
> commit d5cb6720e8d12b281e89f2487561fe95addd0e34
> Author: Konstantin Belousov <k...@freebsd.org>
> Date: Sat Jun 16 00:21:11 2012 +0300
>
> 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.
>
> 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 = 0;
> if (len > 0) {
> - linep = reallocf(linep, olen + len + 1);
> + linep = malloc(olen + len + 1);
> if (linep == NULL) {
> free(lp->l_groupname);
> free(lp);
> return (NULL);
> }
> + if (olen > 0) {
> + bcopy(olinep, linep, olen);
> + free(olinep);
> + }
> bcopy(pos, linep + olen, len);
> olen += len;
> *(linep + olen) = '\0';
> + olinep = linep;
> }
> if (cont) {
> if (fgets(line, LINSIZ, netf)) {
>
>
> commit 5946753b4b094d5f7ac41792df64915434567fd8
> Author: Konstantin Belousov <k...@freebsd.org>
> Date: Sat Jun 16 00:23:01 2012 +0300
>
> More style.
>
> 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 == NULL || !strlen(group))
> return;
>
> - if (grouphead.gr == (struct netgrp *)0 ||
> - strcmp(group, grouphead.grname)) {
> + if (grouphead.gr == 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 == ENOENT) || _yp_statp.st_size == 0)
> + errno == ENOENT) || _yp_statp.st_size == 0)
> _use_only_yp = _netgr_yp_enabled = 1;
> if ((netf = fopen(_PATH_NETGROUP,"r")) != NULL ||_use_only_yp){
> /*
> @@ -247,27 +246,24 @@ endnetgrent(void)
> lp = lp->l_next;
> free(olp->l_groupname);
> free(olp->l_line);
> - free((char *)olp);
> + free(olp);
> }
> - linehead = (struct linelist *)0;
> + linehead = NULL;
> if (grouphead.grname) {
> free(grouphead.grname);
> - grouphead.grname = (char *)0;
> + grouphead.grname = NULL;
> }
> gp = grouphead.gr;
> while (gp) {
> ogp = gp;
> gp = 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 = (struct netgrp *)0;
> - nextgrp = (struct netgrp *)0;
> + grouphead.gr = NULL;
> + nextgrp = NULL;
> #ifdef YP
> _netgr_yp_enabled = 0;
> #endif
> @@ -282,7 +278,7 @@ _listmatch(const char *list, const char *group, int len)
> int glen = strlen(group);
>
> /* skip possible leading whitespace */
> - while(isspace((unsigned char)*ptr))
> + while (isspace((unsigned char)*ptr))
> ptr++;
>
> while (ptr < list + len) {
> @@ -291,7 +287,7 @@ _listmatch(const char *list, const char *group, int len)
> ptr++;
> if (strncmp(cptr, group, glen) == 0 && glen == (ptr - cptr))
> return (1);
> - while(*ptr == ',' || isspace((unsigned char)*ptr))
> + while (*ptr == ',' || isspace((unsigned char)*ptr))
> ptr++;
> }
>
> @@ -436,8 +432,7 @@ parse_netgrp(const char *group)
> break;
> lp = lp->l_next;
> }
> - if (lp == (struct linelist *)0 &&
> - (lp = read_for_group(group)) == (struct linelist *)0)
> + if (lp == NULL && (lp = read_for_group(group)) == NULL)
> return (1);
> if (lp->l_parsed) {
> #ifdef DEBUG
_______________________________________________
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-unsubscr...@freebsd.org"
