diff mbox

[libiberty] fix unbounded alloca in make_relative_prefix_1

Message ID 579B4C83.9080306@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez July 29, 2016, 12:30 p.m. UTC 
At least a cursory look at gcc/gcc-ar.c has us doing:

   self = getenv ("GCC_EXEC_PREFIX");
...
   self_exec_prefix = make_relative_prefix (self, ...

So the alloca() in make_relative_prefix() can be called with the strlen 
of some random env var.

Anyways... regardless... all unchecked alloca calls are bad ;-).

OK pending GCC tests?

Aldy
commit 1c6a52635d1c5099b61876e0a364138583e81a6c
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Fri Jul 29 08:17:35 2016 -0400

    include/
    	* libiberty.h (MAX_ALLOCA_SIZE): New macro.
    
    libiberty/
    	* make-relative-prefix.c (make_relative_prefix_1): Fall back to
    	malloc if alloca argument is greater than MAX_ALLOCA_SIZE.

Comments

Ian Lance Taylor July 29, 2016, 2:57 p.m. UTC | #1 
On Fri, Jul 29, 2016 at 5:30 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
> At least a cursory look at gcc/gcc-ar.c has us doing:
>
>   self = getenv ("GCC_EXEC_PREFIX");
> ...
>   self_exec_prefix = make_relative_prefix (self, ...
>
> So the alloca() in make_relative_prefix() can be called with the strlen of
> some random env var.
>
> Anyways... regardless... all unchecked alloca calls are bad ;-).
>
> OK pending GCC tests?

This is OK.  Thanks.

Ian
Bernd Schmidt July 29, 2016, 2:57 p.m. UTC | #2 
On 07/29/2016 02:30 PM, Aldy Hernandez wrote:
> +/* Max number of alloca bytes per call before we must switch to malloc.
> +
> +   ?? Swiped from gnulib's regex_internal.h header.  Is this actually
> +   the case?  This number seems arbitrary, though sane.
> +
> +   The OS usually guarantees only one guard page at the bottom of the stack,
> +   and a page size can be as small as 4096 bytes.  So we cannot safely
> +   allocate anything larger than 4096 bytes.  Also care for the possibility
> +   of a few compiler-allocated temporary stack slots.  */
> +#define MAX_ALLOCA_SIZE	4032

The only question I have is whether this should be in the public 
libiberty.h header, or whether it's an internal value. If there's only 
one case in libiberty we could put the definition into that file.


Bernd
diff mbox

Patch

diff --git a/include/libiberty.h b/include/libiberty.h
index a9c885f..605ff56 100644
--- a/include/libiberty.h
+++ b/include/libiberty.h
@@ -397,6 +397,17 @@  extern void hex_init (void);
 /* Save files used for communication between processes.  */
 #define PEX_SAVE_TEMPS		0x4
 
+/* Max number of alloca bytes per call before we must switch to malloc.
+
+   ?? Swiped from gnulib's regex_internal.h header.  Is this actually
+   the case?  This number seems arbitrary, though sane.
+
+   The OS usually guarantees only one guard page at the bottom of the stack,
+   and a page size can be as small as 4096 bytes.  So we cannot safely
+   allocate anything larger than 4096 bytes.  Also care for the possibility
+   of a few compiler-allocated temporary stack slots.  */
+#define MAX_ALLOCA_SIZE	4032
+
 /* Prepare to execute one or more programs, with standard output of
    each program fed to standard input of the next.
    FLAGS	As above.
diff --git a/libiberty/make-relative-prefix.c b/libiberty/make-relative-prefix.c
index fe639d1..fa81399 100644
--- a/libiberty/make-relative-prefix.c
+++ b/libiberty/make-relative-prefix.c
@@ -233,6 +233,7 @@  make_relative_prefix_1 (const char *progname, const char *bin_prefix,
   int i, n, common;
   int needed_len;
   char *ret = NULL, *ptr, *full_progname;
+  char *alloc_ptr = NULL;
 
   if (progname == NULL || bin_prefix == NULL || prefix == NULL)
     return NULL;
@@ -256,7 +257,10 @@  make_relative_prefix_1 (const char *progname, const char *bin_prefix,
 #ifdef HAVE_HOST_EXECUTABLE_SUFFIX
 	  len += strlen (HOST_EXECUTABLE_SUFFIX);
 #endif
-	  nstore = (char *) alloca (len);
+	  if (len < MAX_ALLOCA_SIZE)
+	    nstore = (char *) alloca (len);
+	  else
+	    alloc_ptr = nstore = (char *) malloc (len);
 
 	  startp = endp = temp;
 	  while (1)
@@ -312,12 +316,12 @@  make_relative_prefix_1 (const char *progname, const char *bin_prefix,
   else
     full_progname = strdup (progname);
   if (full_progname == NULL)
-    return NULL;
+    goto bailout;
 
   prog_dirs = split_directories (full_progname, &prog_num);
   free (full_progname);
   if (prog_dirs == NULL)
-    return NULL;
+    goto bailout;
 
   bin_dirs = split_directories (bin_prefix, &bin_num);
   if (bin_dirs == NULL)
@@ -395,6 +399,7 @@  make_relative_prefix_1 (const char *progname, const char *bin_prefix,
   free_split_directories (prog_dirs);
   free_split_directories (bin_dirs);
   free_split_directories (prefix_dirs);
+  free (alloc_ptr);
 
   return ret;
 }