Re: [PATCH 1.1] Clean up netgroupcache

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Thu, Mar 27, 2014 at 10:15:16PM +0100, OndÅej BÃlka wrote:
> Had typo there, here is fixed patch.

Please add a note on how you tested the fix.

> 	* nscd/netgroupcache.c: Clean up implementation.

Incorrect ChangeLog entry.

> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 820d823..1412113 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -138,8 +138,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>    char *key_copy = NULL;
>    struct __netgrent data;
>    size_t buflen = MAX (1024, sizeof (*dataset) + req->key_len);
> +  size_t tempbuflen = 1024;
>    size_t buffilled = sizeof (*dataset);
>    char *buffer = NULL;
> +  char *tempbuf;
>    size_t nentries = 0;
>    size_t group_len = strlen (key) + 1;
>    union
> @@ -159,6 +161,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  
>    memset (&data, '\0', sizeof (data));
>    buffer = xmalloc (buflen);
> +  tempbuf = xmalloc (tempbuflen);
> +
>    first_needed.elem.next = &first_needed.elem;
>    memcpy (first_needed.elem.name, key, group_len);
>    data.needed_groups = &first_needed.elem;
> @@ -201,8 +205,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  		while (1)
>  		  {
>  		    int e;
> -		    status = getfct.f (&data, buffer + buffilled,
> -				       buflen - buffilled - req->key_len, &e);
> +		    status = getfct.f (&data, tempbuf, tempbuflen, &e);
>  		    if (status == NSS_STATUS_RETURN
>  			|| status == NSS_STATUS_NOTFOUND)
>  		      /* This was either the last one for this group or the
> @@ -220,53 +223,11 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  			    size_t userlen = strlen (nuser ?: "") + 1;
>  			    size_t domainlen = strlen (ndomain ?: "") + 1;
>  
> -			    if (nhost == NULL || nuser == NULL || ndomain == NULL
> -				|| nhost > nuser || nuser > ndomain)
> +			    size_t newlen = buffilled + hostlen + userlen + domainlen;
> +			    if (newlen + req->key_len > buflen)

Might as well put req->key_len in newlen and simplify this further.

>  			      {
> -				const char *last = nhost;
> -				if (last == NULL
> -				    || (nuser != NULL && nuser > last))
> -				  last = nuser;
> -				if (last == NULL
> -				    || (ndomain != NULL && ndomain > last))
> -				  last = ndomain;
> -
> -				size_t bufused
> -				  = (last == NULL
> -				     ? buffilled
> -				     : last + strlen (last) + 1 - buffer);
> -
> -				/* We have to make temporary copies.  */
> -				size_t needed = hostlen + userlen + domainlen;
> -
> -				if (buflen - req->key_len - bufused < needed)
> -				  {
> -				    buflen += MAX (buflen, 2 * needed);
> -				    /* Save offset in the old buffer.  We don't
> -				       bother with the NULL check here since
> -				       we'll do that later anyway.  */
> -				    size_t nhostdiff = nhost - buffer;
> -				    size_t nuserdiff = nuser - buffer;
> -				    size_t ndomaindiff = ndomain - buffer;
> -
> -				    char *newbuf = xrealloc (buffer, buflen);
> -				    /* Fix up the triplet pointers into the new
> -				       buffer.  */
> -				    nhost = (nhost ? newbuf + nhostdiff
> -					     : NULL);
> -				    nuser = (nuser ? newbuf + nuserdiff
> -					     : NULL);
> -				    ndomain = (ndomain ? newbuf + ndomaindiff
> -					       : NULL);
> -				    buffer = newbuf;
> -				  }
> -
> -				nhost = memcpy (buffer + bufused,
> -						nhost ?: "", hostlen);
> -				nuser = memcpy ((char *) nhost + hostlen,
> -						nuser ?: "", userlen);
> -				ndomain = memcpy ((char *) nuser + userlen,
> -						  ndomain ?: "", domainlen);
> +				buflen = 2 * newlen + req->key_len;
> +				buffer = xrealloc (buffer, buflen);
>  			      }
>  
>  			    char *wp = buffer + buffilled;
> @@ -328,8 +289,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  		      }
>  		    else if (status == NSS_STATUS_UNAVAIL && e == ERANGE)
>  		      {
> -			buflen *= 2;
> -			buffer = xrealloc (buffer, buflen);
> +			tempbuflen *= 2;
> +			tempbuf = xrealloc (tempbuf, tempbuflen);

This ought to be unlikely now, since the consolidated lengths of user,
host and domain should fit in the initially allocated 1024 bytes.

>  		      }
>  		  }
>  
> @@ -474,6 +435,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>      }
>  
>   out:
> +  free (tempbuf);
>    free (buffer);
>  
>    *resultp = dataset;
> 

Siddhesh

Attachment: pgpYdqz6dI0ve.pgp
Description: PGP signature