diff mbox series

Fix fold_checksum_tree buffer overflow (PR bootstrap/89560)

Message ID 20190305083840.GT7611@tucnak
State New
Headers show
Series Fix fold_checksum_tree buffer overflow (PR bootstrap/89560) | expand

Commit Message

Jakub Jelinek March 5, 2019, 8:38 a.m. UTC
Hi!

My earlier change to fold_checksum_tree unfortunately can result in buffer
overflow for CALL_EXPRs with TREE_NO_WARNING bit set and more than 21
arguments, because the code used fixed size 216 byte (on x86_64) buffer
and CALL_EXPR is variable length size 48 + nargs*8.

Which means at least for EXPR_P which doesn't fit we'd need to use alloca
or heap or GC for the larger trees.  When implementing that, I've realized
that fold_checksum_tree can be used in deep recursions and that we don't
really copy a tree at every level, so using the fixed 216-byte buffer can be
undesirable for deep recursion.  Thus the following patch uses alloca
whenever we need to copy something.

Bootstrapped/regtested with --enable-checking=yes,df,fold,rtl,extra ,
ok for trunk?

2019-03-05  Jakub Jelinek  <jakub@redhat.com>

	PR bootstrap/89560
	* fold-const.c (fold_checksum_tree): Don't use fixed size buffer,
	instead alloca it only when needed with the needed size.

	* g++.dg/other/pr89560.C: New test.


	Jakub

Comments

Richard Biener March 5, 2019, 8:43 a.m. UTC | #1
On Tue, 5 Mar 2019, Jakub Jelinek wrote:

> Hi!
> 
> My earlier change to fold_checksum_tree unfortunately can result in buffer
> overflow for CALL_EXPRs with TREE_NO_WARNING bit set and more than 21
> arguments, because the code used fixed size 216 byte (on x86_64) buffer
> and CALL_EXPR is variable length size 48 + nargs*8.
> 
> Which means at least for EXPR_P which doesn't fit we'd need to use alloca
> or heap or GC for the larger trees.  When implementing that, I've realized
> that fold_checksum_tree can be used in deep recursions and that we don't
> really copy a tree at every level, so using the fixed 216-byte buffer can be
> undesirable for deep recursion.  Thus the following patch uses alloca
> whenever we need to copy something.
> 
> Bootstrapped/regtested with --enable-checking=yes,df,fold,rtl,extra ,
> ok for trunk?

OK.

Richard.

> 2019-03-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR bootstrap/89560
> 	* fold-const.c (fold_checksum_tree): Don't use fixed size buffer,
> 	instead alloca it only when needed with the needed size.
> 
> 	* g++.dg/other/pr89560.C: New test.
> 
> --- gcc/fold-const.c.jj	2019-03-01 10:26:08.717750481 +0100
> +++ gcc/fold-const.c	2019-03-04 16:33:50.509788120 +0100
> @@ -12112,7 +12112,7 @@ fold_checksum_tree (const_tree expr, str
>  {
>    const tree_node **slot;
>    enum tree_code code;
> -  union tree_node buf;
> +  union tree_node *buf;
>    int i, len;
>  
>   recursive_label:
> @@ -12127,11 +12127,13 @@ fold_checksum_tree (const_tree expr, str
>        && HAS_DECL_ASSEMBLER_NAME_P (expr))
>      {
>        /* Allow DECL_ASSEMBLER_NAME and symtab_node to be modified.  */
> -      memcpy ((char *) &buf, expr, tree_size (expr));
> -      SET_DECL_ASSEMBLER_NAME ((tree)&buf, NULL);
> -      buf.decl_with_vis.symtab_node = NULL;
> -      buf.base.nowarning_flag = 0;
> -      expr = (tree) &buf;
> +      size_t sz = tree_size (expr);
> +      buf = XALLOCAVAR (union tree_node, sz);
> +      memcpy ((char *) buf, expr, sz);
> +      SET_DECL_ASSEMBLER_NAME ((tree) buf, NULL);
> +      buf->decl_with_vis.symtab_node = NULL;
> +      buf->base.nowarning_flag = 0;
> +      expr = (tree) buf;
>      }
>    else if (TREE_CODE_CLASS (code) == tcc_type
>  	   && (TYPE_POINTER_TO (expr)
> @@ -12143,8 +12145,10 @@ fold_checksum_tree (const_tree expr, str
>      {
>        /* Allow these fields to be modified.  */
>        tree tmp;
> -      memcpy ((char *) &buf, expr, tree_size (expr));
> -      expr = tmp = (tree) &buf;
> +      size_t sz = tree_size (expr);
> +      buf = XALLOCAVAR (union tree_node, sz);
> +      memcpy ((char *) buf, expr, sz);
> +      expr = tmp = (tree) buf;
>        TYPE_CONTAINS_PLACEHOLDER_INTERNAL (tmp) = 0;
>        TYPE_POINTER_TO (tmp) = NULL;
>        TYPE_REFERENCE_TO (tmp) = NULL;
> @@ -12160,9 +12164,11 @@ fold_checksum_tree (const_tree expr, str
>      {
>        /* Allow TREE_NO_WARNING to be set.  Perhaps we shouldn't allow that
>  	 and change builtins.c etc. instead - see PR89543.  */
> -      memcpy ((char *) &buf, expr, tree_size (expr));
> -      buf.base.nowarning_flag = 0;
> -      expr = (tree) &buf;
> +      size_t sz = tree_size (expr);
> +      buf = XALLOCAVAR (union tree_node, sz);
> +      memcpy ((char *) buf, expr, sz);
> +      buf->base.nowarning_flag = 0;
> +      expr = (tree) buf;
>      }
>    md5_process_bytes (expr, tree_size (expr), ctx);
>    if (CODE_CONTAINS_STRUCT (code, TS_TYPED))
> --- gcc/testsuite/g++.dg/other/pr89560.C.jj	2019-03-04 16:36:49.465886681 +0100
> +++ gcc/testsuite/g++.dg/other/pr89560.C	2019-03-04 16:36:34.396131007 +0100
> @@ -0,0 +1,13 @@
> +// PR bootstrap/89560
> +// { dg-do compile }
> +
> +#define TEN(x) x##0, x##1, x##2, x##3, x##4, x##5, x##6, x##7, x##8, x##9,
> +#define HUNDRED(x) TEN(x##0) TEN(x##1) TEN(x##2) TEN(x##3) TEN(x##4) \
> +		   TEN(x##5) TEN(x##6) TEN(x##7) TEN(x##8) TEN(x##9)
> +int foo (int, ...);
> +
> +int
> +bar ()
> +{
> +  return (foo (HUNDRED (1) 0));
> +}
> 
> 	Jakub
> 
>
diff mbox series

Patch

--- gcc/fold-const.c.jj	2019-03-01 10:26:08.717750481 +0100
+++ gcc/fold-const.c	2019-03-04 16:33:50.509788120 +0100
@@ -12112,7 +12112,7 @@  fold_checksum_tree (const_tree expr, str
 {
   const tree_node **slot;
   enum tree_code code;
-  union tree_node buf;
+  union tree_node *buf;
   int i, len;
 
  recursive_label:
@@ -12127,11 +12127,13 @@  fold_checksum_tree (const_tree expr, str
       && HAS_DECL_ASSEMBLER_NAME_P (expr))
     {
       /* Allow DECL_ASSEMBLER_NAME and symtab_node to be modified.  */
-      memcpy ((char *) &buf, expr, tree_size (expr));
-      SET_DECL_ASSEMBLER_NAME ((tree)&buf, NULL);
-      buf.decl_with_vis.symtab_node = NULL;
-      buf.base.nowarning_flag = 0;
-      expr = (tree) &buf;
+      size_t sz = tree_size (expr);
+      buf = XALLOCAVAR (union tree_node, sz);
+      memcpy ((char *) buf, expr, sz);
+      SET_DECL_ASSEMBLER_NAME ((tree) buf, NULL);
+      buf->decl_with_vis.symtab_node = NULL;
+      buf->base.nowarning_flag = 0;
+      expr = (tree) buf;
     }
   else if (TREE_CODE_CLASS (code) == tcc_type
 	   && (TYPE_POINTER_TO (expr)
@@ -12143,8 +12145,10 @@  fold_checksum_tree (const_tree expr, str
     {
       /* Allow these fields to be modified.  */
       tree tmp;
-      memcpy ((char *) &buf, expr, tree_size (expr));
-      expr = tmp = (tree) &buf;
+      size_t sz = tree_size (expr);
+      buf = XALLOCAVAR (union tree_node, sz);
+      memcpy ((char *) buf, expr, sz);
+      expr = tmp = (tree) buf;
       TYPE_CONTAINS_PLACEHOLDER_INTERNAL (tmp) = 0;
       TYPE_POINTER_TO (tmp) = NULL;
       TYPE_REFERENCE_TO (tmp) = NULL;
@@ -12160,9 +12164,11 @@  fold_checksum_tree (const_tree expr, str
     {
       /* Allow TREE_NO_WARNING to be set.  Perhaps we shouldn't allow that
 	 and change builtins.c etc. instead - see PR89543.  */
-      memcpy ((char *) &buf, expr, tree_size (expr));
-      buf.base.nowarning_flag = 0;
-      expr = (tree) &buf;
+      size_t sz = tree_size (expr);
+      buf = XALLOCAVAR (union tree_node, sz);
+      memcpy ((char *) buf, expr, sz);
+      buf->base.nowarning_flag = 0;
+      expr = (tree) buf;
     }
   md5_process_bytes (expr, tree_size (expr), ctx);
   if (CODE_CONTAINS_STRUCT (code, TS_TYPED))
--- gcc/testsuite/g++.dg/other/pr89560.C.jj	2019-03-04 16:36:49.465886681 +0100
+++ gcc/testsuite/g++.dg/other/pr89560.C	2019-03-04 16:36:34.396131007 +0100
@@ -0,0 +1,13 @@ 
+// PR bootstrap/89560
+// { dg-do compile }
+
+#define TEN(x) x##0, x##1, x##2, x##3, x##4, x##5, x##6, x##7, x##8, x##9,
+#define HUNDRED(x) TEN(x##0) TEN(x##1) TEN(x##2) TEN(x##3) TEN(x##4) \
+		   TEN(x##5) TEN(x##6) TEN(x##7) TEN(x##8) TEN(x##9)
+int foo (int, ...);
+
+int
+bar ()
+{
+  return (foo (HUNDRED (1) 0));
+}