Re: [PATCH 4/4] nscd: netgroup: Use two buffers in addgetnetgrentX (bug 31680)

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2024-04-24 12:08, Florian Weimer wrote:
> This avoids potential memory corruption when the underlying NSS
> callback function does not use the buffer space to store all strings
> (e.g., for constant strings).
> 
> Instead of custom buffer management, two scratch buffers are used.
> This increases stack usage somewhat.
> 
> Scratch buffer allocation failure is handled by return -1
> (an invalid timeout value) instead of terminating the process.
> This fixes bug 31679.
> ---

LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

>   nscd/netgroupcache.c | 219 ++++++++++++++++++++++++-------------------
>   1 file changed, 121 insertions(+), 98 deletions(-)
> 
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 8f9eb84e39..8b5ebfaf31 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -23,6 +23,7 @@
>   #include <stdlib.h>
>   #include <unistd.h>
>   #include <sys/mman.h>
> +#include <scratch_buffer.h>
>   
>   #include "../nss/netgroup.h"
>   #include "nscd.h"
> @@ -65,6 +66,16 @@ struct dataset
>     char strdata[0];
>   };
>   
> +/* Send a notfound response to FD.  Always returns -1 to indicate an
> +   ephemeral error.  */
> +static time_t
> +send_notfound (int fd)
> +{
> +  if (fd != -1)
> +    TEMP_FAILURE_RETRY (send (fd, &notfound, sizeof (notfound), MSG_NOSIGNAL));
> +  return -1;
> +}
> +
>   /* Sends a notfound message and prepares a notfound dataset to write to the
>      cache.  Returns true if there was enough memory to allocate the dataset and
>      returns the dataset in DATASETP, total bytes to write in TOTALP and the
> @@ -83,8 +94,7 @@ do_notfound (struct database_dyn *db, int fd, request_header *req,
>     total = sizeof (notfound);
>     timeout = time (NULL) + db->negtimeout;
>   
> -  if (fd != -1)
> -    TEMP_FAILURE_RETRY (send (fd, &notfound, total, MSG_NOSIGNAL));
> +  send_notfound (fd);
>   
>     dataset = mempool_alloc (db, sizeof (struct dataset) + req->key_len, 1);
>     /* If we cannot permanently store the result, so be it.  */
> @@ -109,11 +119,78 @@ do_notfound (struct database_dyn *db, int fd, request_header *req,
>     return cacheable;
>   }
>   
> +struct addgetnetgrentX_scratch
> +{
> +  /* This is the result that the caller should use.  It can be NULL,
> +     point into buffer, or it can be in the cache.  */
> +  struct dataset *dataset;
> +
> +  struct scratch_buffer buffer;
> +
> +  /* Used internally in addgetnetgrentX as a staging area.  */
> +  struct scratch_buffer tmp;
> +
> +  /* Number of bytes in buffer that are actually used.  */
> +  size_t buffer_used;
> +};
> +
> +static void
> +addgetnetgrentX_scratch_init (struct addgetnetgrentX_scratch *scratch)
> +{
> +  scratch->dataset = NULL;
> +  scratch_buffer_init (&scratch->buffer);
> +  scratch_buffer_init (&scratch->tmp);
> +
> +  /* Reserve space for the header.  */
> +  scratch->buffer_used = sizeof (struct dataset);
> +  static_assert (sizeof (struct dataset) < sizeof (scratch->tmp.__space),
> +		 "initial buffer space");
> +  memset (scratch->tmp.data, 0, sizeof (struct dataset));
> +}
> +
> +static void
> +addgetnetgrentX_scratch_free (struct addgetnetgrentX_scratch *scratch)
> +{
> +  scratch_buffer_free (&scratch->buffer);
> +  scratch_buffer_free (&scratch->tmp);
> +}
> +
> +/* Copy LENGTH bytes from S into SCRATCH.  Returns NULL if SCRATCH
> +   could not be resized, otherwise a pointer to the copy.  */
> +static char *
> +addgetnetgrentX_append_n (struct addgetnetgrentX_scratch *scratch,
> +			  const char *s, size_t length)
> +{
> +  while (true)
> +    {
> +      size_t remaining = scratch->buffer.length - scratch->buffer_used;
> +      if (remaining >= length)
> +	break;
> +      if (!scratch_buffer_grow_preserve (&scratch->buffer))
> +	return NULL;
> +    }
> +  char *copy = scratch->buffer.data + scratch->buffer_used;
> +  memcpy (copy, s, length);
> +  scratch->buffer_used += length;
> +  return copy;
> +}
> +
> +/* Copy S into SCRATCH, including its null terminator.  Returns false
> +   if SCRATCH could not be resized.  */
> +static bool
> +addgetnetgrentX_append (struct addgetnetgrentX_scratch *scratch, const char *s)
> +{
> +  if (s == NULL)
> +    s = "";
> +  return addgetnetgrentX_append_n (scratch, s, strlen (s) + 1) != NULL;
> +}
> +
> +/* Caller must initialize and free *SCRATCH.  If the return value is
> +   negative, this function has sent a notfound response.  */
>   static time_t
>   addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>   		 const char *key, uid_t uid, struct hashentry *he,
> -		 struct datahead *dh, struct dataset **resultp,
> -		 void **tofreep)
> +		 struct datahead *dh, struct addgetnetgrentX_scratch *scratch)
>   {
>     if (__glibc_unlikely (debug_level > 0))
>       {
> @@ -132,14 +209,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 buffilled = sizeof (*dataset);
> -  char *buffer = NULL;
>     size_t nentries = 0;
>     size_t group_len = strlen (key) + 1;
>     struct name_list *first_needed
>       = alloca (sizeof (struct name_list) + group_len);
> -  *tofreep = NULL;
>   
>     if (netgroup_database == NULL
>         && !__nss_database_get (nss_database_netgroup, &netgroup_database))
> @@ -151,8 +224,6 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>       }
>   
>     memset (&data, '\0', sizeof (data));
> -  buffer = xmalloc (buflen);
> -  *tofreep = buffer;
>     first_needed->next = first_needed;
>     memcpy (first_needed->name, key, group_len);
>     data.needed_groups = first_needed;
> @@ -195,8 +266,8 @@ 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, scratch->tmp.data,
> +				       scratch->tmp.length, &e);
>   		    if (status == NSS_STATUS_SUCCESS)
>   		      {
>   			if (data.type == triple_val)
> @@ -204,68 +275,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>   			    const char *nhost = data.val.triple.host;
>   			    const char *nuser = data.val.triple.user;
>   			    const char *ndomain = data.val.triple.domain;
> -
> -			    size_t hostlen = strlen (nhost ?: "") + 1;
> -			    size_t userlen = strlen (nuser ?: "") + 1;
> -			    size_t domainlen = strlen (ndomain ?: "") + 1;
> -
> -			    if (nhost == NULL || nuser == NULL || ndomain == NULL
> -				|| nhost > nuser || nuser > ndomain)
> -			      {
> -				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);
> -				    *tofreep = buffer = newbuf;
> -				  }
> -
> -				nhost = memcpy (buffer + bufused,
> -						nhost ?: "", hostlen);
> -				nuser = memcpy ((char *) nhost + hostlen,
> -						nuser ?: "", userlen);
> -				ndomain = memcpy ((char *) nuser + userlen,
> -						  ndomain ?: "", domainlen);
> -			      }
> -
> -			    char *wp = buffer + buffilled;
> -			    wp = memmove (wp, nhost ?: "", hostlen);
> -			    wp += hostlen;
> -			    wp = memmove (wp, nuser ?: "", userlen);
> -			    wp += userlen;
> -			    wp = memmove (wp, ndomain ?: "", domainlen);
> -			    wp += domainlen;
> -			    buffilled = wp - buffer;
> +			    if (!(addgetnetgrentX_append (scratch, nhost)
> +				  && addgetnetgrentX_append (scratch, nuser)
> +				  && addgetnetgrentX_append (scratch, ndomain)))
> +			      return send_notfound (fd);
>   			    ++nentries;
>   			  }
>   			else
> @@ -317,8 +330,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>   		      }
>   		    else if (status == NSS_STATUS_TRYAGAIN && e == ERANGE)
>   		      {
> -			buflen *= 2;
> -			*tofreep = buffer = xrealloc (buffer, buflen);
> +			if (!scratch_buffer_grow (&scratch->tmp))
> +			  return send_notfound (fd);
>   		      }
>   		    else if (status == NSS_STATUS_RETURN
>   			     || status == NSS_STATUS_NOTFOUND
> @@ -351,10 +364,17 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>         goto maybe_cache_add;
>       }
>   
> -  total = buffilled;
> +  /* Capture the result size without the key appended.   */
> +  total = scratch->buffer_used;
> +
> +  /* Make a copy of the key.  The scratch buffer must not move after
> +     this point.  */
> +  key_copy = addgetnetgrentX_append_n (scratch, key, req->key_len);
> +  if (key_copy == NULL)
> +    return send_notfound (fd);
>   
>     /* Fill in the dataset.  */
> -  dataset = (struct dataset *) buffer;
> +  dataset = scratch->buffer.data;
>     timeout = datahead_init_pos (&dataset->head, total + req->key_len,
>   			       total - offsetof (struct dataset, resp),
>   			       he == NULL ? 0 : dh->nreloads + 1,
> @@ -363,11 +383,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>     dataset->resp.version = NSCD_VERSION;
>     dataset->resp.found = 1;
>     dataset->resp.nresults = nentries;
> -  dataset->resp.result_len = buffilled - sizeof (*dataset);
> -
> -  assert (buflen - buffilled >= req->key_len);
> -  key_copy = memcpy (buffer + buffilled, key, req->key_len);
> -  buffilled += req->key_len;
> +  dataset->resp.result_len = total - sizeof (*dataset);
>   
>     /* Now we can determine whether on refill we have to create a new
>        record or not.  */
> @@ -398,7 +414,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>       if (__glibc_likely (newp != NULL))
>         {
>   	/* Adjust pointer into the memory block.  */
> -	key_copy = (char *) newp + (key_copy - buffer);
> +	key_copy = (char *) newp + (key_copy - (char *) dataset);
>   
>   	dataset = memcpy (newp, dataset, total + req->key_len);
>   	cacheable = true;
> @@ -439,7 +455,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>       }
>   
>    out:
> -  *resultp = dataset;
> +  scratch->dataset = dataset;
>   
>     return timeout;
>   }
> @@ -460,6 +476,9 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>     if (user != NULL)
>       key = strchr (key, '\0') + 1;
>     const char *domain = *key++ ? key : NULL;
> +  struct addgetnetgrentX_scratch scratch;
> +
> +  addgetnetgrentX_scratch_init (&scratch);
>   
>     if (__glibc_unlikely (debug_level > 0))
>       {
> @@ -475,12 +494,8 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>   							    group, group_len,
>   							    db, uid);
>     time_t timeout;
> -  void *tofree;
>     if (result != NULL)
> -    {
> -      timeout = result->head.timeout;
> -      tofree = NULL;
> -    }
> +    timeout = result->head.timeout;
>     else
>       {
>         request_header req_get =
> @@ -489,7 +504,10 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>   	  .key_len = group_len
>   	};
>         timeout = addgetnetgrentX (db, -1, &req_get, group, uid, NULL, NULL,
> -				 &result, &tofree);
> +				 &scratch);
> +      result = scratch.dataset;
> +      if (timeout < 0)
> +	goto out;
>       }
>   
>     struct indataset
> @@ -601,7 +619,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>       }
>   
>    out:
> -  free (tofree);
> +  addgetnetgrentX_scratch_free (&scratch);
>     return timeout;
>   }
>   
> @@ -611,11 +629,12 @@ addgetnetgrentX_ignore (struct database_dyn *db, int fd, request_header *req,
>   			const char *key, uid_t uid, struct hashentry *he,
>   			struct datahead *dh)
>   {
> -  struct dataset *ignore;
> -  void *tofree;
> -  time_t timeout = addgetnetgrentX (db, fd, req, key, uid, he, dh,
> -				    &ignore, &tofree);
> -  free (tofree);
> +  struct addgetnetgrentX_scratch scratch;
> +  addgetnetgrentX_scratch_init (&scratch);
> +  time_t timeout = addgetnetgrentX (db, fd, req, key, uid, he, dh, &scratch);
> +  addgetnetgrentX_scratch_free (&scratch);
> +  if (timeout < 0)
> +    timeout = 0;
>     return timeout;
>   }
>   
> @@ -659,5 +678,9 @@ readdinnetgr (struct database_dyn *db, struct hashentry *he,
>         .key_len = he->len
>       };
>   
> -  return addinnetgrX (db, -1, &req, db->data + he->key, he->owner, he, dh);
> +  int timeout = addinnetgrX (db, -1, &req, db->data + he->key, he->owner,
> +			     he, dh);
> +  if (timeout < 0)
> +    timeout = 0;
> +  return timeout;
>   }