[Patch] [BZ 13761] Fix another unbound alloca

[Jeff Law]

On 07/02/2012 04:02 PM, Roland McGrath wrote:
>> It seems to me that freeing dataset is just wrong if it's allocated via
>> mempool_alloc and things in mempool_alloc are GC'd.
>
> Correct.
>
>> ISTM the safe thing to do would be something like:
>>
>>        if (he == NULL)
>>           dataset = (struct dataset *) mempool_alloc (db, total + n, 1);
>>        else if (! __libc_use_alloca (alloca_used + total + n))
>>           dataset = (struct dataset *) malloc (...);
>
> Agreed.  Note that in the existing code ALLOCA_USED is a Boolean, and it is
> used to mean "cannot be placed in the cache".  So the bookkeeping would
> change somewhat to support the malloc case (which is alloca_used=true in
> the current sense of that variable, but also has to be freed).
>
>> While it might be safe/appropriate to use mempools in the case where the
>> needed space is large and transient, it's hard to prove.
>
> The purpose of the mempool stuff is to use the shared memory space that's
> available to clients as a cache without interacting with nscd.  So it's
> never appropriate for something that is not going to be looked up in that
> cache later.
Sorry this has taken so long to get back to.

To refresh your memory, DATASET can be allocated via alloca within 
cache_addgr and the size is potentially unbound.  The original patch had 
the problem that it could allocate DATASET via mempool_alloc, then try 
to free it later.  As we've discussed, that's clearly wrong.

This new version of the patch implements the changes noted above.  Those 
changes actually make the patch simpler to follow.



-------------- next part --------------
2012-08-09  Jeff Law  <law@redhat.com>

	[BZ #13761]
	* nscd/grpcache.c (cache_addgr): Rename alloca_used to
	dataset_temporary.  Track alloca usage into alloca_used.
	If dataset is large allocate and release it via malloc/free.

diff --git a/NEWS b/NEWS
index eae9834..08caa0a 100644
--- a/NEWS
+++ b/NEWS
@@ -9,7 +9,7 @@ Version 2.17
 
 * The following bugs are resolved with this release:
 
-  6778, 6808, 13717, 14042, 14166, 14150, 14151, 14154, 14157, 14173, 14283,
+  6778, 6808, 13717, 13761, 14042, 14166, 14150, 14151, 14154, 14157, 14173, 14283,
   14298, 14307, 14328, 14331, 14336, 14337, 14347, 14349
 
 * Support for STT_GNU_IFUNC symbols added for s390 and s390x.
diff --git a/nscd/grpcache.c b/nscd/grpcache.c
index d09badf..88a0633 100644
--- a/nscd/grpcache.c
+++ b/nscd/grpcache.c
@@ -177,7 +177,8 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
       char *cp;
       const size_t key_len = strlen (key);
       const size_t buf_len = 3 * sizeof (grp->gr_gid) + key_len + 1;
-      char *buf = alloca (buf_len);
+      size_t alloca_used = 0;
+      char *buf = alloca_account (buf_len, alloca_used);
       ssize_t n;
       size_t cnt;
 
@@ -189,7 +190,8 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
       /* Determine the length of all members.  */
       while (grp->gr_mem[gr_mem_cnt])
 	++gr_mem_cnt;
-      gr_mem_len = (uint32_t *) alloca (gr_mem_cnt * sizeof (uint32_t));
+      gr_mem_len = (uint32_t *) alloca_account (gr_mem_cnt * sizeof (uint32_t),
+						alloca_used);
       for (gr_mem_cnt = 0; grp->gr_mem[gr_mem_cnt]; ++gr_mem_cnt)
 	{
 	  gr_mem_len[gr_mem_cnt] = strlen (grp->gr_mem[gr_mem_cnt]) + 1;
@@ -204,7 +206,8 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
 	 change.  Allocate memory on the cache since it is likely
 	 discarded anyway.  If it turns out to be necessary to have a
 	 new record we can still allocate real memory.  */
-      bool alloca_used = false;
+      bool dataset_temporary = false;
+      bool dataset_malloced = false;
       dataset = NULL;
 
       if (he == NULL)
@@ -215,10 +218,18 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
 	  /* We cannot permanently add the result in the moment.  But
 	     we can provide the result as is.  Store the data in some
 	     temporary memory.  */
-	  dataset = (struct dataset *) alloca (total + n);
+	  if (! __libc_use_alloca (alloca_used + total + n))
+	    {
+	      /* XXX What to do if malloc fails?  */
+	      dataset = (struct dataset *) malloc (total + n);
+	      dataset_malloced = true;
+	    }
+	  else
+	    dataset= (struct dataset *) alloca_account (total + n,
+							alloca_used);
 
 	  /* We cannot add this record to the permanent database.  */
-	  alloca_used = true;
+	  dataset_temporary = true;
 	}
 
       dataset->head.allocsize = total + n;
@@ -272,6 +283,11 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
 		 allocated on the stack and need not be freed.  */
 	      dh->timeout = dataset->head.timeout;
 	      ++dh->nreloads;
+
+	      /* If the new record was allocated via malloc, then we must free
+		 it here.  */
+	      if (dataset_malloced)
+		free (dataset);
 	    }
 	  else
 	    {
@@ -287,7 +303,7 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
 		  key_copy = (char *) newp + (key_copy - (char *) dataset);
 
 		  dataset = memcpy (newp, dataset, total + n);
-		  alloca_used = false;
+		  dataset_temporary = false;
 		}
 
 	      /* Mark the old record as obsolete.  */
@@ -302,7 +318,7 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
 	  assert (fd != -1);
 
 #ifdef HAVE_SENDFILE
-	  if (__builtin_expect (db->mmap_used, 1) && !alloca_used)
+	  if (__builtin_expect (db->mmap_used, 1) && ! dataset_temporary)
 	    {
 	      assert (db->wr_fd != -1);
 	      assert ((char *) &dataset->resp > (char *) db->data);
@@ -329,7 +345,7 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
 
       /* Add the record to the database.  But only if it has not been
 	 stored on the stack.  */
-      if (! alloca_used)
+      if (! dataset_temporary)
 	{
 	  /* If necessary, we also propagate the data to disk.  */
 	  if (db->persistent)