This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [Patch] [BZ 13761] Fix another unbound alloca
- From: Jeff Law <law at redhat dot com>
- To: Roland McGrath <roland at hack dot frob dot com>
- Cc: libc-alpha <libc-alpha at sourceware dot org>
- Date: Wed, 28 Nov 2012 15:12:48 -0700
- Subject: Re: [Patch] [BZ 13761] Fix another unbound alloca
- References: <5034064B.9070103@redhat.com> <20120821221812.B4EC52C0B2@topped-with-meat.com>
On 08/21/2012 04:18 PM, Roland McGrath wrote:
Off hand I see one typo (missing space before an =).
Fixed.
An XXX comment is not sufficient handling of malloc failure.
We need to figure out what is the robust behavior there.
In theory, it's a cache and we ought to be able to just ignore the
request to add the entry to the cache. Other paths through this code
will do precisely that when cache_add fails. They also return whatever
value is sitting in TIMEOUT and release the lock. I've changed this
code to do the same in the event that malloc fails, though I'd feel
better if someone with a better understanding of the context of this
code would chime in on the right behaviour.
Also, it's never really necessary to cast the result of malloc or alloca.
That's what void * is for (this is not C++). There were gratuitous casts
in the old code, but there's no need to repeat the pattern.
I was just following the existing conventions in the code. I've removed
the gratuitous casts in the code I added/changed, the remainder were
left as-is.
To recap (from the BZ):
nss_compat allocates buffer space on stack using alloca (and
extend_alloca) for initgroup and keeps extending it to fit in larger
lines. This breaks for cases where the number of members in a gorup
are very large, causing the alloca reference to go beyond thread stack
boundary. Siddhesh partially fixed this problem a while back, this
patch completes the fix.
Jeff
diff --git a/ChangeLog b/ChangeLog
index b74cde7..a075867 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2012-11-28 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.
+
2012-11-28 Jeff Law <law@redhat.com>
Martin Osvald <mosvald@redhat.com>
diff --git a/NEWS b/NEWS
index 4a17295..dadb434 100644
--- a/NEWS
+++ b/NEWS
@@ -12,17 +12,17 @@ Version 2.17
1349, 3439, 3479, 3665, 5044, 5246, 5298, 5400, 6530, 6778, 6808, 9685,
9914, 10014, 10038, 10631, 10873, 11438, 11607, 11638, 11741, 12140,
13412, 13542, 13601, 13603, 13604, 13629, 13679, 13696, 13698, 13717,
- 13741, 13759, 13763, 13881, 13939, 13950, 13952, 13966, 14042, 14047,
- 14090, 14150, 14151, 14152, 14154, 14157, 14166, 14173, 14195, 14237,
- 14251, 14252, 14283, 14298, 14303, 14307, 14328, 14331, 14336, 14337,
- 14347, 14349, 14368, 14376, 14417, 14459, 14476, 14477, 14501, 14505,
- 14510, 14516, 14518, 14519, 14530, 14532, 14538, 14543, 14544, 14545,
- 14557, 14562, 14568, 14576, 14579, 14583, 14587, 14595, 14602, 14610,
- 14621, 14638, 14645, 14648, 14652, 14660, 14661, 14669, 14672, 14683,
- 14694, 14716, 14719, 14743, 14767, 14783, 14784, 14785, 14793, 14796,
- 14797, 14801, 14805, 14807, 14811, 14815, 14821, 14822, 14824, 14828,
- 14831, 14835, 14838, 14856, 14863, 14865, 14866, 14868, 14869, 14871,
- 14889.
+ 13741, 13759, 13761, 13763, 13881, 13939, 13950, 13952, 13966, 14042,
+ 14047, 14090, 14150, 14151, 14152, 14154, 14157, 14166, 14173, 14195,
+ 14237, 14251, 14252, 14283, 14298, 14303, 14307, 14328, 14331, 14336,
+ 14337, 14347, 14349, 14368, 14376, 14417, 14459, 14476, 14477, 14501,
+ 14505, 14510, 14516, 14518, 14519, 14530, 14532, 14538, 14543, 14544,
+ 14545, 14557, 14562, 14568, 14576, 14579, 14583, 14587, 14595, 14602,
+ 14610, 14621, 14638, 14645, 14648, 14652, 14660, 14661, 14669, 14672,
+ 14683, 14694, 14716, 14719, 14743, 14767, 14783, 14784, 14785, 14793,
+ 14796, 14797, 14801, 14805, 14807, 14811, 14815, 14821, 14822, 14824,
+ 14828, 14831, 14835, 14838, 14856, 14863, 14865, 14866, 14868, 14869,
+ 14871, 14889.
* CVE-2011-4609 svc_run() produces high cpu usage when accept fails with
EMFILE has been fixed (Bugzilla #14889).
diff --git a/nscd/grpcache.c b/nscd/grpcache.c
index d09badf..a781b38 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,7 @@ 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 = 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 +205,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 +217,20 @@ 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))
+ {
+ dataset = malloc (total + n);
+ /* Perhaps we should log a message that we were unable
+ to allocate memory for a large request. */
+ if (dataset == NULL)
+ goto out;
+ dataset_malloced = true;
+ }
+ else
+ 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 +284,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 +304,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 +319,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 +346,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)