Message ID | 20190305083840.GT7611@tucnak |
---|---|
State | New |
Headers | show |
Series | Fix fold_checksum_tree buffer overflow (PR bootstrap/89560) | expand |
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 > >
--- 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)); +}