Re: [patch] Fix for BZ 17905 -- unbounded alloca in catopen

[OndÅej BÃlka <neleai at seznam dot cz>]

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;							      \
> +	}								      \
>      }
>