Re: [PATCH 1.1] Clean up netgroupcache

[OndÅej BÃlka <neleai at seznam dot cz>]

On Thu, Mar 27, 2014 at 09:32:07PM +0100, OndÅej BÃlka wrote:
> On Thu, Mar 27, 2014 at 08:22:54PM +0100, OndÅej BÃlka wrote:
> > On Thu, Mar 27, 2014 at 03:34:11AM -0400, Mike Frysinger wrote:
> > > On Thu 27 Mar 2014 09:34:06 Siddhesh Poyarekar wrote:
> > > > Calls to stpcpy from nscd netgroups code will have overlapping source
> > > > and destination when all three values in the returned triplet are
> > > > non-NULL and in the expected (host,user,domain) order.  This is seen
> > > > in valgrind as:
> > > > 
> > > > Fix this by using memmove instead of stpcpy.  Tested x86_64 using
> > > > various combinations of triplets (including NULL and non-NULL ones) to
> > > > verify that this works correctly and there are no regressions.
> > >
> > This could work only with additional assertion that we do not move host
> > forward otherwise it could overwrite user.
> >  
> > > i feel like we've wanted an equivalent of stpcpy/memccpy for memmove.  good 
> > > time to add it ? :)
> > > 
> > Yes, it would be better to use this at least internally, perhaps this
> > patch instead is cleaner. 
> > 
> > Other possibility is keep these in separate header like second snippet, 
> > do you have better name for that? Also I could make a stpcat and move
> > equivalent, not sure with what name.
> > 
> > Her I would fix a root cause of these bugs which is bad design. We mix
> > temporary buffer with building result. If we use separate buffers for
> > that a code would be lot simpler, I will prepare patch for it.
> > 
> Here is patch that uses separate buffers.
> 
Had typo there, here is fixed patch.

	* nscd/netgroupcache.c: Clean up implementation.


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