* [patch] Fix for BZ 17905 -- unbounded alloca in catopen
@ 2015-07-12 23:41 Paul Pluzhnikov
2015-07-13 6:54 ` Ondřej Bílka
0 siblings, 1 reply; 4+ messages in thread
From: Paul Pluzhnikov @ 2015-07-12 23:41 UTC (permalink / raw)
To: GLIBC Devel; +Cc: Paul Pluzhnikov
[-- Attachment #1: Type: text/plain, Size: 458 bytes --]
Greetings,
Attached patch fixes BZ #17905 -- unbounded alloca in catopen, and
adds a test for it.
Thanks,
--
Paul Pluzhnikov
2015-07-12 Paul Pluzhnikov <ppluzhnikov@google.com>
[BZ #17905]
* catgets/Makefile (tst-catgets-mem): New test.
* catgets/catgets.c (catopen): Don't use unbounded alloca.
* catgets/open_catalog.c (__open_catalog): Likewise.
* catgets/tst-catgets.c (do_bz17905): Test unbounded alloca.
[-- Attachment #2: pr17905-patch.txt --]
[-- Type: text/plain, Size: 5809 bytes --]
diff --git a/catgets/Makefile b/catgets/Makefile
index 4624a88..56de38b 100644
--- a/catgets/Makefile
+++ b/catgets/Makefile
@@ -34,6 +34,7 @@ test-srcs = test-gencat
ifeq ($(run-built-tests),yes)
tests-special += $(objpfx)de/libc.cat $(objpfx)test1.cat $(objpfx)test2.cat \
$(objpfx)sample.SJIS.cat $(objpfx)test-gencat.out
+tests-special += $(objpfx)tst-catgets-mem.out
endif
gencat-modules = xmalloc
@@ -50,9 +51,11 @@ catgets-CPPFLAGS := -DNLSPATH='"$(msgcatdir)/%L/%N:$(msgcatdir)/%L/LC_MESSAGES/%
generated += de.msg test1.cat test1.h test2.cat test2.h sample.SJIS.cat \
test-gencat.h
+generated += tst-catgets.mtrace tst-catgets-mem.out
+
generated-dirs += de
-tst-catgets-ENV = NLSPATH="$(objpfx)%l/%N.cat" LANG=de
+tst-catgets-ENV = NLSPATH="$(objpfx)%l/%N.cat" LANG=de MALLOC_TRACE=$(objpfx)tst-catgets.mtrace
ifeq ($(run-built-tests),yes)
# This test just checks whether the program produces any error or not.
@@ -86,4 +89,8 @@ $(objpfx)test-gencat.out: test-gencat.sh $(objpfx)test-gencat \
$(objpfx)sample.SJIS.cat: sample.SJIS $(objpfx)gencat
$(built-program-cmd) -H $(objpfx)test-gencat.h < $(word 1,$^) > $@; \
$(evaluate-test)
+
+$(objpfx)tst-catgets-mem.out: $(objpfx)tst-catgets.out
+ $(common-objpfx)malloc/mtrace $(objpfx)tst-catgets.mtrace > $@; \
+ $(evaluate-test)
endif
diff --git a/catgets/catgets.c b/catgets/catgets.c
index cf93d56..28a2f59 100644
--- a/catgets/catgets.c
+++ b/catgets/catgets.c
@@ -16,7 +16,6 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
-#include <alloca.h>
#include <errno.h>
#include <locale.h>
#include <nl_types.h>
@@ -35,6 +34,7 @@ catopen (const char *cat_name, int flag)
__nl_catd result;
const char *env_var = NULL;
const char *nlspath = NULL;
+ char *tmp = NULL;
if (strchr (cat_name, '/') == NULL)
{
@@ -54,7 +54,7 @@ catopen (const char *cat_name, int flag)
{
/* Append the system dependent directory. */
size_t len = strlen (nlspath) + 1 + sizeof NLSPATH;
- char *tmp = alloca (len);
+ tmp = malloc (len);
__stpcpy (__stpcpy (__stpcpy (tmp, nlspath), ":"), NLSPATH);
nlspath = tmp;
@@ -64,17 +64,15 @@ catopen (const char *cat_name, int flag)
}
result = (__nl_catd) malloc (sizeof (*result));
- if (result == NULL)
- /* We cannot get enough memory. */
- return (nl_catd) -1;
-
- if (__open_catalog (cat_name, nlspath, env_var, result) != 0)
+ if (result == NULL
+ || __open_catalog (cat_name, nlspath, env_var, result) != 0)
{
- /* Couldn't open the file. */
+ /* We cannot get enough memory. */
free ((void *) result);
- return (nl_catd) -1;
+ result = (nl_catd) -1;
}
+ free (tmp);
return (nl_catd) result;
}
diff --git a/catgets/open_catalog.c b/catgets/open_catalog.c
index e069416..4db4181 100644
--- a/catgets/open_catalog.c
+++ b/catgets/open_catalog.c
@@ -47,6 +47,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
size_t tab_size;
const char *lastp;
int result = -1;
+ char *buf = NULL;
if (strchr (cat_name, '/') != NULL || nlspath == NULL)
fd = open_not_cancel_2 (cat_name, O_RDONLY);
@@ -58,22 +59,22 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
{ \
char *old_buf = buf; \
bufmax += 256 + (n); \
- buf = (char *) alloca (bufmax); \
- memcpy (buf, old_buf, bufact); \
+ buf = realloc (buf, bufmax); \
+ if (__glibc_unlikely (buf == NULL)) \
+ { \
+ free (old_buf); \
+ return -1; \
+ } \
}
/* The RUN_NLSPATH variable contains a colon separated list of
descriptions where we expect to find catalogs. We have to
recognize certain % substitutions and stop when we found the
first existing file. */
- char *buf;
size_t bufact;
- size_t bufmax;
+ size_t bufmax = 0;
size_t len;
- buf = NULL;
- bufmax = 0;
-
fd = -1;
while (*run_nlspath != '\0')
{
@@ -188,7 +189,10 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
/* Avoid dealing with directories and block devices */
if (__builtin_expect (fd, 0) < 0)
- return -1;
+ {
+ free (buf);
+ return -1;
+ }
if (__builtin_expect (__fxstat64 (_STAT_VER, fd, &st), 0) < 0)
goto close_unlock_return;
@@ -325,6 +329,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
/* Release the lock again. */
close_unlock_return:
close_not_cancel_no_status (fd);
+ free (buf);
return result;
}
diff --git a/catgets/tst-catgets.c b/catgets/tst-catgets.c
index a0a4089..140de72 100644
--- a/catgets/tst-catgets.c
+++ b/catgets/tst-catgets.c
@@ -1,7 +1,10 @@
+#include <assert.h>
#include <mcheck.h>
#include <nl_types.h>
#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
+#include <sys/resource.h>
static const char *msgs[] =
@@ -12,6 +15,33 @@ static const char *msgs[] =
};
#define nmsgs (sizeof (msgs) / sizeof (msgs[0]))
+
+/* Test for unbounded alloca. */
+static int
+do_bz17905 (void)
+{
+ char *buf;
+ struct rlimit rl;
+ nl_catd result;
+
+ const int sz = 1024 * 1024;
+
+ getrlimit (RLIMIT_STACK, &rl);
+ rl.rlim_cur = sz;
+ setrlimit (RLIMIT_STACK, &rl);
+
+ buf = malloc (sz + 1);
+ memset (buf, 'A', sz);
+ buf[sz] = '\0';
+ setenv ("NLSPATH", buf, 1);
+
+ result = catopen (buf, NL_CAT_LOCALE);
+ assert (result == (nl_catd) -1);
+
+ free (buf);
+ return 0;
+}
+
#define ROUNDS 5
static int
@@ -62,6 +92,7 @@ do_test (void)
}
}
+ result += do_bz17905 ();
return result;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] Fix for BZ 17905 -- unbounded alloca in catopen
2015-07-12 23:41 [patch] Fix for BZ 17905 -- unbounded alloca in catopen Paul Pluzhnikov
@ 2015-07-13 6:54 ` Ondřej Bílka
2015-07-14 16:06 ` Paul Pluzhnikov
0 siblings, 1 reply; 4+ messages in thread
From: Ondřej Bílka @ 2015-07-13 6:54 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: GLIBC Devel, Paul Pluzhnikov
On Sun, Jul 12, 2015 at 04:41:24PM -0700, Paul Pluzhnikov wrote:
> Greetings,
>
> Attached patch fixes BZ #17905 -- unbounded alloca in catopen, and
> adds a test for it.
>
> Thanks,
> --
> Paul Pluzhnikov
>
>
> 2015-07-12 Paul Pluzhnikov <ppluzhnikov@google.com>
>
> [BZ #17905]
> * catgets/Makefile (tst-catgets-mem): New test.
> * catgets/catgets.c (catopen): Don't use unbounded alloca.
> * catgets/open_catalog.c (__open_catalog): Likewise.
> * catgets/tst-catgets.c (do_bz17905): Test unbounded alloca.
> diff --git a/catgets/Makefile b/catgets/Makefile
> index 4624a88..56de38b 100644
> --- a/catgets/Makefile
> +++ b/catgets/Makefile
> @@ -34,6 +34,7 @@ test-srcs = test-gencat
> ifeq ($(run-built-tests),yes)
> tests-special += $(objpfx)de/libc.cat $(objpfx)test1.cat $(objpfx)test2.cat \
> $(objpfx)sample.SJIS.cat $(objpfx)test-gencat.out
> +tests-special += $(objpfx)tst-catgets-mem.out
> endif
>
> gencat-modules = xmalloc
> @@ -50,9 +51,11 @@ catgets-CPPFLAGS := -DNLSPATH='"$(msgcatdir)/%L/%N:$(msgcatdir)/%L/LC_MESSAGES/%
>
> generated += de.msg test1.cat test1.h test2.cat test2.h sample.SJIS.cat \
> test-gencat.h
> +generated += tst-catgets.mtrace tst-catgets-mem.out
> +
> generated-dirs += de
>
> -tst-catgets-ENV = NLSPATH="$(objpfx)%l/%N.cat" LANG=de
> +tst-catgets-ENV = NLSPATH="$(objpfx)%l/%N.cat" LANG=de MALLOC_TRACE=$(objpfx)tst-catgets.mtrace
>
> ifeq ($(run-built-tests),yes)
> # This test just checks whether the program produces any error or not.
> @@ -86,4 +89,8 @@ $(objpfx)test-gencat.out: test-gencat.sh $(objpfx)test-gencat \
> $(objpfx)sample.SJIS.cat: sample.SJIS $(objpfx)gencat
> $(built-program-cmd) -H $(objpfx)test-gencat.h < $(word 1,$^) > $@; \
> $(evaluate-test)
> +
> +$(objpfx)tst-catgets-mem.out: $(objpfx)tst-catgets.out
> + $(common-objpfx)malloc/mtrace $(objpfx)tst-catgets.mtrace > $@; \
> + $(evaluate-test)
> endif
> diff --git a/catgets/catgets.c b/catgets/catgets.c
> index cf93d56..28a2f59 100644
> --- a/catgets/catgets.c
> +++ b/catgets/catgets.c
> @@ -16,7 +16,6 @@
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
>
> -#include <alloca.h>
> #include <errno.h>
> #include <locale.h>
> #include <nl_types.h>
> @@ -35,6 +34,7 @@ catopen (const char *cat_name, int flag)
> __nl_catd result;
> const char *env_var = NULL;
> const char *nlspath = NULL;
> + char *tmp = NULL;
>
> if (strchr (cat_name, '/') == NULL)
> {
> @@ -54,7 +54,7 @@ catopen (const char *cat_name, int flag)
> {
> /* Append the system dependent directory. */
> size_t len = strlen (nlspath) + 1 + sizeof NLSPATH;
> - char *tmp = alloca (len);
> + tmp = malloc (len);
>
check if result is null.
> diff --git a/catgets/open_catalog.c b/catgets/open_catalog.c
> index e069416..4db4181 100644
> --- a/catgets/open_catalog.c
> +++ b/catgets/open_catalog.c
> @@ -47,6 +47,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
> size_t tab_size;
> const char *lastp;
> int result = -1;
> + char *buf = NULL;
>
> if (strchr (cat_name, '/') != NULL || nlspath == NULL)
> fd = open_not_cancel_2 (cat_name, O_RDONLY);
> @@ -58,22 +59,22 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
> { \
> char *old_buf = buf; \
> bufmax += 256 + (n); \
why this doesn't trigger quadratic behaviour? Doubling sizes would be
faster.
> - buf = (char *) alloca (bufmax); \
> - memcpy (buf, old_buf, bufact); \
> + buf = realloc (buf, bufmax); \
> + if (__glibc_unlikely (buf == NULL)) \
> + { \
> + free (old_buf); \
> + return -1; \
> + } \
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] Fix for BZ 17905 -- unbounded alloca in catopen
2015-07-13 6:54 ` Ondřej Bílka
@ 2015-07-14 16:06 ` Paul Pluzhnikov
2015-08-08 22:56 ` Paul Pluzhnikov
0 siblings, 1 reply; 4+ messages in thread
From: Paul Pluzhnikov @ 2015-07-14 16:06 UTC (permalink / raw)
To: Ondřej Bílka; +Cc: GLIBC Devel
[-- Attachment #1: Type: text/plain, Size: 513 bytes --]
On Sun, Jul 12, 2015 at 11:54 PM, Ondřej Bílka <neleai@seznam.cz> wrote:
> check if result is null.
Thanks for review. Revised patch attached.
2015-07-12 Paul Pluzhnikov <ppluzhnikov@google.com>
[BZ #17905]
* catgets/Makefile (tst-catgets-mem): New test.
* catgets/catgets.c (catopen): Don't use unbounded alloca.
* catgets/open_catalog.c (__open_catalog): Likewise.
* catgets/tst-catgets.c (do_bz17905): Test unbounded alloca.
--
Paul Pluzhnikov
[-- Attachment #2: pr17905-patch-20150714.txt --]
[-- Type: text/plain, Size: 6002 bytes --]
diff --git a/catgets/Makefile b/catgets/Makefile
index 4624a88..56de38b 100644
--- a/catgets/Makefile
+++ b/catgets/Makefile
@@ -34,6 +34,7 @@ test-srcs = test-gencat
ifeq ($(run-built-tests),yes)
tests-special += $(objpfx)de/libc.cat $(objpfx)test1.cat $(objpfx)test2.cat \
$(objpfx)sample.SJIS.cat $(objpfx)test-gencat.out
+tests-special += $(objpfx)tst-catgets-mem.out
endif
gencat-modules = xmalloc
@@ -50,9 +51,11 @@ catgets-CPPFLAGS := -DNLSPATH='"$(msgcatdir)/%L/%N:$(msgcatdir)/%L/LC_MESSAGES/%
generated += de.msg test1.cat test1.h test2.cat test2.h sample.SJIS.cat \
test-gencat.h
+generated += tst-catgets.mtrace tst-catgets-mem.out
+
generated-dirs += de
-tst-catgets-ENV = NLSPATH="$(objpfx)%l/%N.cat" LANG=de
+tst-catgets-ENV = NLSPATH="$(objpfx)%l/%N.cat" LANG=de MALLOC_TRACE=$(objpfx)tst-catgets.mtrace
ifeq ($(run-built-tests),yes)
# This test just checks whether the program produces any error or not.
@@ -86,4 +89,8 @@ $(objpfx)test-gencat.out: test-gencat.sh $(objpfx)test-gencat \
$(objpfx)sample.SJIS.cat: sample.SJIS $(objpfx)gencat
$(built-program-cmd) -H $(objpfx)test-gencat.h < $(word 1,$^) > $@; \
$(evaluate-test)
+
+$(objpfx)tst-catgets-mem.out: $(objpfx)tst-catgets.out
+ $(common-objpfx)malloc/mtrace $(objpfx)tst-catgets.mtrace > $@; \
+ $(evaluate-test)
endif
diff --git a/catgets/catgets.c b/catgets/catgets.c
index cf93d56..7098628 100644
--- a/catgets/catgets.c
+++ b/catgets/catgets.c
@@ -16,7 +16,6 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
-#include <alloca.h>
#include <errno.h>
#include <locale.h>
#include <nl_types.h>
@@ -35,6 +34,7 @@ catopen (const char *cat_name, int flag)
__nl_catd result;
const char *env_var = NULL;
const char *nlspath = NULL;
+ char *tmp = NULL;
if (strchr (cat_name, '/') == NULL)
{
@@ -54,7 +54,10 @@ catopen (const char *cat_name, int flag)
{
/* Append the system dependent directory. */
size_t len = strlen (nlspath) + 1 + sizeof NLSPATH;
- char *tmp = alloca (len);
+ tmp = malloc (len);
+
+ if (__glibc_unlikely (tmp == NULL))
+ return (nl_catd) -1;
__stpcpy (__stpcpy (__stpcpy (tmp, nlspath), ":"), NLSPATH);
nlspath = tmp;
@@ -64,17 +67,15 @@ catopen (const char *cat_name, int flag)
}
result = (__nl_catd) malloc (sizeof (*result));
- if (result == NULL)
- /* We cannot get enough memory. */
- return (nl_catd) -1;
-
- if (__open_catalog (cat_name, nlspath, env_var, result) != 0)
+ if (result == NULL
+ || __open_catalog (cat_name, nlspath, env_var, result) != 0)
{
- /* Couldn't open the file. */
+ /* We cannot get enough memory. */
free ((void *) result);
- return (nl_catd) -1;
+ result = (nl_catd) -1;
}
+ free (tmp);
return (nl_catd) result;
}
diff --git a/catgets/open_catalog.c b/catgets/open_catalog.c
index e069416..3e772f1 100644
--- a/catgets/open_catalog.c
+++ b/catgets/open_catalog.c
@@ -47,6 +47,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
size_t tab_size;
const char *lastp;
int result = -1;
+ char *buf = NULL;
if (strchr (cat_name, '/') != NULL || nlspath == NULL)
fd = open_not_cancel_2 (cat_name, O_RDONLY);
@@ -57,23 +58,23 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
if (__glibc_unlikely (bufact + (n) >= bufmax)) \
{ \
char *old_buf = buf; \
- bufmax += 256 + (n); \
- buf = (char *) alloca (bufmax); \
- memcpy (buf, old_buf, bufact); \
+ bufmax += (bufmax == 0) ? 256 + (n) : bufmax; \
+ buf = realloc (buf, bufmax); \
+ if (__glibc_unlikely (buf == NULL)) \
+ { \
+ free (old_buf); \
+ return -1; \
+ } \
}
/* The RUN_NLSPATH variable contains a colon separated list of
descriptions where we expect to find catalogs. We have to
recognize certain % substitutions and stop when we found the
first existing file. */
- char *buf;
size_t bufact;
- size_t bufmax;
+ size_t bufmax = 0;
size_t len;
- buf = NULL;
- bufmax = 0;
-
fd = -1;
while (*run_nlspath != '\0')
{
@@ -188,7 +189,10 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
/* Avoid dealing with directories and block devices */
if (__builtin_expect (fd, 0) < 0)
- return -1;
+ {
+ free (buf);
+ return -1;
+ }
if (__builtin_expect (__fxstat64 (_STAT_VER, fd, &st), 0) < 0)
goto close_unlock_return;
@@ -325,6 +329,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
/* Release the lock again. */
close_unlock_return:
close_not_cancel_no_status (fd);
+ free (buf);
return result;
}
diff --git a/catgets/tst-catgets.c b/catgets/tst-catgets.c
index a0a4089..140de72 100644
--- a/catgets/tst-catgets.c
+++ b/catgets/tst-catgets.c
@@ -1,7 +1,10 @@
+#include <assert.h>
#include <mcheck.h>
#include <nl_types.h>
#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
+#include <sys/resource.h>
static const char *msgs[] =
@@ -12,6 +15,33 @@ static const char *msgs[] =
};
#define nmsgs (sizeof (msgs) / sizeof (msgs[0]))
+
+/* Test for unbounded alloca. */
+static int
+do_bz17905 (void)
+{
+ char *buf;
+ struct rlimit rl;
+ nl_catd result;
+
+ const int sz = 1024 * 1024;
+
+ getrlimit (RLIMIT_STACK, &rl);
+ rl.rlim_cur = sz;
+ setrlimit (RLIMIT_STACK, &rl);
+
+ buf = malloc (sz + 1);
+ memset (buf, 'A', sz);
+ buf[sz] = '\0';
+ setenv ("NLSPATH", buf, 1);
+
+ result = catopen (buf, NL_CAT_LOCALE);
+ assert (result == (nl_catd) -1);
+
+ free (buf);
+ return 0;
+}
+
#define ROUNDS 5
static int
@@ -62,6 +92,7 @@ do_test (void)
}
}
+ result += do_bz17905 ();
return result;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] Fix for BZ 17905 -- unbounded alloca in catopen
2015-07-14 16:06 ` Paul Pluzhnikov
@ 2015-08-08 22:56 ` Paul Pluzhnikov
0 siblings, 0 replies; 4+ messages in thread
From: Paul Pluzhnikov @ 2015-08-08 22:56 UTC (permalink / raw)
To: Ondřej Bílka; +Cc: GLIBC Devel
On Tue, Jul 14, 2015 at 9:05 AM, Paul Pluzhnikov <ppluzhnikov@gmail.com> wrote:
> On Sun, Jul 12, 2015 at 11:54 PM, Ondřej Bílka <neleai@seznam.cz> wrote:
>
>> check if result is null.
>
> Thanks for review. Revised patch attached.
I revised the patch a tiny bit more, re-tested on Linux/x86_64 and
committed it as 0f58539030e436449f79189b6edab17d7479796e
--
Paul Pluzhnikov
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-08-08 22:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-12 23:41 [patch] Fix for BZ 17905 -- unbounded alloca in catopen Paul Pluzhnikov
2015-07-13 6:54 ` Ondřej Bílka
2015-07-14 16:06 ` Paul Pluzhnikov
2015-08-08 22:56 ` Paul Pluzhnikov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).