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 703b0ee00f78d956768725221c2d07e10ec9277a..a61bfde9185f080e9cf7511325b4587326830975 100644 (file)
--- 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 f406522882af5731e18b4e006349fae97c5169a2..248f2c30ad220dec7d77e112f0571e646bce3d24 100644 (file)
--- 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 9fc16640ae7b5f67524ff13b24f407c72d83370a..58234b1492380ee9a18b1e4c53beb2dd38bd0e4f 100644 (file)
--- 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;