Re: [PATCH][BZ #16453] Don't use alloca in addgetnetgrentX

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Wed, Jan 15, 2014 at 06:35:54PM +0100, OndÅej BÃlka wrote:
> If we are at simplyfing buflen could be written as 
> 
>                                  buflen = buflen + MAX (buflen, 2 * needed)
> 
> to fit a line.

Thanks, I also removed the unused malloc_used variable; I forgot to do
that and didn't notice the warning earlier since I was building with
rpmbuild.  The following is what I finally committed:

Siddhesh

commit 980cb5180e1b71224a57ca52b995c959b7148c09
Author: Siddhesh Poyarekar <siddhesh@redhat.com>
Date:   Thu Jan 16 10:20:22 2014 +0530

    Don't use alloca in addgetnetgrentX (BZ #16453)
    
    addgetnetgrentX has a buffer which is grown as per the needs of the
    requested size either by using alloca or by falling back to malloc if
    the size is larger than 1K.  There are two problems with the alloca
    bits: firstly, it doesn't really extend the buffer since it does not
    use the return value of the extend_alloca macro, which is the location
    of the reallocated buffer.  Due to this the buffer does not actually
    extend itself and hence a subsequent write may overwrite stuff on the
    stack.
    
    The second problem is more subtle - the buffer growth on the stack is
    discontinuous due to block scope local variables.  Combine that with
    the fact that unlike realloc, extend_alloca does not copy over old
    content and you have a situation where the buffer just has garbage in
    the space where it should have had data.
    
    This could have been fixed by adding code to copy over old data
    whenever we call extend_alloca, but it seems unnecessarily
    complicated.  This code is not exactly a performance hotspot (it's
    called when there is a cache miss, so factors like network lookup or
    file reads will dominate over memory allocation/reallocation), so this
    premature optimization is unnecessary.
    
    Thanks Brad Hubbard <bhubbard@redhat.com> for his help with debugging
    the problem.

diff --git a/ChangeLog b/ChangeLog
index 703b0ee..a61bfde 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2013-01-16  Siddhesh Poyarekar  <siddhesh@redhat.com>
+
+	[BZ #16453]
+	* nscd/netgroupcache.c (addgetnetgrentX): Don't use alloca.
+
 2014-01-15  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
 
 	* sysdeps/powerpc/sotruss-lib.c: New file: sotruss-lib.so
diff --git a/NEWS b/NEWS
index f406522..248f2c3 100644
--- a/NEWS
+++ b/NEWS
@@ -25,7 +25,7 @@ Version 2.19
   16151, 16153, 16167, 16172, 16195, 16214, 16245, 16271, 16274, 16283,
   16289, 16293, 16314, 16316, 16330, 16337, 16338, 16356, 16365, 16366,
   16369, 16372, 16375, 16379, 16384, 16385, 16386, 16387, 16390, 16394,
-  16400, 16407, 16408, 16414.
+  16400, 16407, 16408, 16414, 16453.
 
 * Slovenian translations for glibc messages have been contributed by the
   Translation Project's Slovenian team of translators.
diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
index 9fc1664..58234b1 100644
--- a/nscd/netgroupcache.c
+++ b/nscd/netgroupcache.c
@@ -141,7 +141,6 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
   size_t buffilled = sizeof (*dataset);
   char *buffer = NULL;
   size_t nentries = 0;
-  bool use_malloc = false;
   size_t group_len = strlen (key) + 1;
   union
   {
@@ -159,7 +158,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
     }
 
   memset (&data, '\0', sizeof (data));
-  buffer = alloca (buflen);
+  buffer = xmalloc (buflen);
   first_needed.elem.next = &first_needed.elem;
   memcpy (first_needed.elem.name, key, group_len);
   data.needed_groups = &first_needed.elem;
@@ -241,21 +240,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 
 				if (buflen - req->key_len - bufused < needed)
 				  {
-				    size_t newsize = MAX (2 * buflen,
-							  buflen + 2 * needed);
-				    if (use_malloc || newsize > 1024 * 1024)
-				      {
-					buflen = newsize;
-					char *newbuf = xrealloc (use_malloc
-								 ? buffer
-								 : NULL,
-								 buflen);
-
-					buffer = newbuf;
-					use_malloc = true;
-				      }
-				    else
-				      extend_alloca (buffer, buflen, newsize);
+				    buflen += MAX (buflen, 2 * needed);
+				    buffer = xrealloc (buffer, buflen);
 				  }
 
 				nhost = memcpy (buffer + bufused,
@@ -322,18 +308,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 		      }
 		    else if (status == NSS_STATUS_UNAVAIL && e == ERANGE)
 		      {
-			size_t newsize = 2 * buflen;
-			if (use_malloc || newsize > 1024 * 1024)
-			  {
-			    buflen = newsize;
-			    char *newbuf = xrealloc (use_malloc
-						     ? buffer : NULL, buflen);
-
-			    buffer = newbuf;
-			    use_malloc = true;
-			  }
-			else
-			  extend_alloca (buffer, buflen, newsize);
+			buflen *= 2;
+			buffer = xrealloc (buffer, buflen);
 		      }
 		  }
 
@@ -478,8 +454,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
     }
 
  out:
-  if (use_malloc)
-    free (buffer);
+  free (buffer);
 
   *resultp = dataset;