| Message ID | VI1PR0701MB2862DEE3CE9F931132E8ECDDE4F40@VI1PR0701MB2862.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR 86572 | expand |
On 10/22/2018 09:08 AM, Bernd Edlinger wrote: > Hi! > > This makes c_strlen avoid an unsafe strlen folding of const arguments > with non-const offset. Currently a negative out of bounds offset > makes the strlen function return an extremely large number, and > at the same time, prevents the VRP machinery, to determine the correct > range if the strlen function in this case. > > Fixed by doing the whole computation in size_t and casting the > result back to ssize_t. > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? In general, I view folding to a "safe" value as done in this patch (and as Richard suggests in his comment on the bug) far preferable -- as in far safer/more secure -- to avoiding folding in these cases (currently the prevailing strategy). If there is consensus on adopting this approach for strlen I'd like to see it applied in other such cases as well as a matter of policy. Some other similar examples to consider include: * calling other built-ins on such arrays, including strnlen, memchr/strchr, and strpbrk * indexing into such an array (note that accesses at constant out-of-bounds indices into constant arrays are already folded to zero, although that may be an accidental rather than a conscious decision made to avoid the worst kind of fallout). None of these is currently folded to a safe value. GCC calls the library functions for the built-ins in the first bullet when the argument is a constant string with a non-constant offset, even though it could fold them. At some point it would be also helpful to try to get consensus on whether emitting a trap in these cases (perhaps under an option) is something to start moving toward, as we discussed at Cauldron. Martin
On 10/22/18 9:03 PM, Martin Sebor wrote: > On 10/22/2018 09:08 AM, Bernd Edlinger wrote: >> Hi! >> >> This makes c_strlen avoid an unsafe strlen folding of const arguments >> with non-const offset. Currently a negative out of bounds offset >> makes the strlen function return an extremely large number, and >> at the same time, prevents the VRP machinery, to determine the correct >> range if the strlen function in this case. >> >> Fixed by doing the whole computation in size_t and casting the >> result back to ssize_t. >> >> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >> Is it OK for trunk? > > In general, I view folding to a "safe" value as done in this patch > (and as Richard suggests in his comment on the bug) far preferable > -- as in far safer/more secure -- to avoiding folding in these > cases (currently the prevailing strategy). > Well, in this case we have the slightly different situation, where 'strlen(string_cst + x)' is folded to 'x <= len ? len - x : 0'. The string_cst does not have embedded nuls, and may be declared like 'const char string_cst[100] = "string"'. So the COND_EXPR is not there to separate valid from invalid inputs, but instead we may assume that x does never exceed the array bounds. However it still results in more accurate range infos, to do all computations in unsigned arithmetics, and a slightly more safe result if the x is negative at the same time, so win-win. But I have no doubt that it would be even more safe to have a trap if x is out of bounds, however if we add another conditional branch, that would be something that needs to be optional, and results in slightly less efficient code. > If there is consensus on adopting this approach for strlen I'd like > to see it applied in other such cases as well as a matter of policy. > > Some other similar examples to consider include: > > * calling other built-ins on such arrays, including strnlen, > memchr/strchr, and strpbrk > * indexing into such an array (note that accesses at constant > out-of-bounds indices into constant arrays are already folded > to zero, although that may be an accidental rather than > a conscious decision made to avoid the worst kind of fallout). > I would argue that if the undefinedness of any such construct can be determined statically, there should be a warning, and the folding should not be done, the out-of-bounds index handling is also something that I would remove eventually. The folding should not be done when it is evident that the index exceeds the memory size, as returned by string_constant, while it is still okay to fold to 0, if the index is somewhere in between the string length and the memory size. That can be determined statically, and comes at no extra cost. Bernd.
Ping... For this patch here: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01336.html Thanks Bernd. On 10/22/18 5:08 PM, Bernd Edlinger wrote: > Hi! > > This makes c_strlen avoid an unsafe strlen folding of const arguments > with non-const offset. Currently a negative out of bounds offset > makes the strlen function return an extremely large number, and > at the same time, prevents the VRP machinery, to determine the correct > range if the strlen function in this case. > > Fixed by doing the whole computation in size_t and casting the > result back to ssize_t. > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? > > > Thanks > Bernd.
On 10/22/18 9:08 AM, Bernd Edlinger wrote: > Hi! > > This makes c_strlen avoid an unsafe strlen folding of const arguments > with non-const offset. Currently a negative out of bounds offset > makes the strlen function return an extremely large number, and > at the same time, prevents the VRP machinery, to determine the correct > range if the strlen function in this case. > > Fixed by doing the whole computation in size_t and casting the > result back to ssize_t. > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? > > > Thanks > Bernd. > > > patch-pr86572.diff > > gcc: > 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> > > PR tree-optimization/86572 > * builtins.c (c_strlen): Handle negative offsets in a safe way. > > testsuite: > 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> > > PR tree-optimization/86572 > * gcc.dg/pr86572.c: New test. OK. jeff
On Sun, Nov 4, 2018 at 10:02 AM Jeff Law <law@redhat.com> wrote: > > On 10/22/18 9:08 AM, Bernd Edlinger wrote: > > Hi! > > > > This makes c_strlen avoid an unsafe strlen folding of const arguments > > with non-const offset. Currently a negative out of bounds offset > > makes the strlen function return an extremely large number, and > > at the same time, prevents the VRP machinery, to determine the correct > > range if the strlen function in this case. > > > > Fixed by doing the whole computation in size_t and casting the > > result back to ssize_t. > > > > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > > Is it OK for trunk? > > > > > > Thanks > > Bernd. > > > > > > patch-pr86572.diff > > > > gcc: > > 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> > > > > PR tree-optimization/86572 > > * builtins.c (c_strlen): Handle negative offsets in a safe way. > > > > testsuite: > > 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> > > > > PR tree-optimization/86572 > > * gcc.dg/pr86572.c: New test. > OK. > jeff This caused: /export/gnu/import/git/gcc-test-ia32/src-trunk/gcc/testsuite/gcc.dg/warn-strlen-no-nul.c:56:1: internal compiler error: verify_gimple failed^M 0x8922dc4 verify_gimple_in_seq(gimple*)^M ../../src-trunk/gcc/tree-cfg.c:5082^M 0x86899d7 gimplify_body(tree_node*, bool)^M ../../src-trunk/gcc/gimplify.c:12859^M 0x8689b8b gimplify_function_tree(tree_node*)^M ../../src-trunk/gcc/gimplify.c:12949^M 0x84f7690 cgraph_node::analyze()^M ../../src-trunk/gcc/cgraphunit.c:667^M 0x84fa1d8 analyze_functions^M ../../src-trunk/gcc/cgraphunit.c:1126^M 0x84fadd3 symbol_table::finalize_compilation_unit()^M ../../src-trunk/gcc/cgraphunit.c:2833^M Please submit a full bug report,^M with preprocessed source if appropriate.^M Please include the complete backtrace with any bug report.^M See <https://gcc.gnu.org/bugs/> for instructions.^M compiler exited with status 1 FAIL: gcc.dg/warn-strlen-no-nul.c (internal compiler error) on i386.
On 11/5/18 1:28 AM, H.J. Lu wrote: > On Sun, Nov 4, 2018 at 10:02 AM Jeff Law <law@redhat.com> wrote: >> >> On 10/22/18 9:08 AM, Bernd Edlinger wrote: >>> Hi! >>> >>> This makes c_strlen avoid an unsafe strlen folding of const arguments >>> with non-const offset. Currently a negative out of bounds offset >>> makes the strlen function return an extremely large number, and >>> at the same time, prevents the VRP machinery, to determine the correct >>> range if the strlen function in this case. >>> >>> Fixed by doing the whole computation in size_t and casting the >>> result back to ssize_t. >>> >>> >>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >>> Is it OK for trunk? >>> >>> >>> Thanks >>> Bernd. >>> >>> >>> patch-pr86572.diff >>> >>> gcc: >>> 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> >>> >>> PR tree-optimization/86572 >>> * builtins.c (c_strlen): Handle negative offsets in a safe way. >>> >>> testsuite: >>> 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> >>> >>> PR tree-optimization/86572 >>> * gcc.dg/pr86572.c: New test. >> OK. >> jeff > > This caused: > > /export/gnu/import/git/gcc-test-ia32/src-trunk/gcc/testsuite/gcc.dg/warn-strlen-no-nul.c:56:1: > internal compiler error: verify_gimple failed^M > 0x8922dc4 verify_gimple_in_seq(gimple*)^M > ../../src-trunk/gcc/tree-cfg.c:5082^M > 0x86899d7 gimplify_body(tree_node*, bool)^M > ../../src-trunk/gcc/gimplify.c:12859^M > 0x8689b8b gimplify_function_tree(tree_node*)^M > ../../src-trunk/gcc/gimplify.c:12949^M > 0x84f7690 cgraph_node::analyze()^M > ../../src-trunk/gcc/cgraphunit.c:667^M > 0x84fa1d8 analyze_functions^M > ../../src-trunk/gcc/cgraphunit.c:1126^M > 0x84fadd3 symbol_table::finalize_compilation_unit()^M > ../../src-trunk/gcc/cgraphunit.c:2833^M > Please submit a full bug report,^M > with preprocessed source if appropriate.^M > Please include the complete backtrace with any bug report.^M > See <https://gcc.gnu.org/bugs/> for instructions.^M > compiler exited with status 1 > FAIL: gcc.dg/warn-strlen-no-nul.c (internal compiler error) > > on i386. > Ah yes thanks. This is caused by an incorrect folding in string_constant. After stripping the type casts in the POINTER_PLUS_EXPR we add the offset which is sizetype to what is left over from arg1, which is probably even a correctness issue, if the type cast was a narrowing one. Bootstrapped and reg-tested on x86_64-pc-linux-gnu (this time tested with RUNTESTFLAGS="--target_board=unix\{-m32,\}") Is it OK for trunk? Thanks Bernd. 2018-11-05 Bernd Edlinger <bernd.edlinger@hotmail.de> * expr.c (string_constant): Don't strip NOPS in subexpressions. Fold PLUS_EXPR correctly. Index: gcc/expr.c =================================================================== --- gcc/expr.c (revision 265778) +++ gcc/expr.c (working copy) @@ -11357,19 +11357,16 @@ string_constant (tree arg, tree *ptr_offset, tree tree arg0 = TREE_OPERAND (arg, 0); tree arg1 = TREE_OPERAND (arg, 1); - STRIP_NOPS (arg0); - STRIP_NOPS (arg1); - - if (TREE_CODE (arg0) == ADDR_EXPR) - ; /* Do nothing. */ - else if (TREE_CODE (arg1) == ADDR_EXPR) - std::swap (arg0, arg1); - else - return NULL_TREE; - tree offset; - if (tree str = string_constant (arg0, &offset, mem_size, decl)) + tree str = string_constant (arg0, &offset, mem_size, decl); + if (!str) { + str = string_constant (arg1, &offset, mem_size, decl); + arg1 = arg0; + } + + if (str) + { /* Avoid pointers to arrays (see bug 86622). */ if (POINTER_TYPE_P (TREE_TYPE (arg)) && TREE_CODE (TREE_TYPE (TREE_TYPE (arg))) == ARRAY_TYPE @@ -11379,7 +11376,8 @@ string_constant (tree arg, tree *ptr_offset, tree && tree_int_cst_equal (*mem_size, DECL_SIZE_UNIT (*decl)))) return NULL_TREE; - tree type = TREE_TYPE (arg1); + tree type = TREE_TYPE (offset); + arg1 = fold_convert (type, arg1); *ptr_offset = fold_build2 (PLUS_EXPR, type, offset, arg1); return str; } @@ -11411,7 +11409,8 @@ string_constant (tree arg, tree *ptr_offset, tree return NULL_TREE; tree rhs2 = gimple_assign_rhs2 (stmt); - tree type = TREE_TYPE (rhs2); + tree type = TREE_TYPE (offset); + rhs2 = fold_convert (type, rhs2); *ptr_offset = fold_build2 (PLUS_EXPR, type, offset, rhs2); return str; }
On Mon, Nov 5, 2018 at 3:20 PM Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > > On 11/5/18 1:28 AM, H.J. Lu wrote: > > On Sun, Nov 4, 2018 at 10:02 AM Jeff Law <law@redhat.com> wrote: > >> > >> On 10/22/18 9:08 AM, Bernd Edlinger wrote: > >>> Hi! > >>> > >>> This makes c_strlen avoid an unsafe strlen folding of const arguments > >>> with non-const offset. Currently a negative out of bounds offset > >>> makes the strlen function return an extremely large number, and > >>> at the same time, prevents the VRP machinery, to determine the correct > >>> range if the strlen function in this case. > >>> > >>> Fixed by doing the whole computation in size_t and casting the > >>> result back to ssize_t. > >>> > >>> > >>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > >>> Is it OK for trunk? > >>> > >>> > >>> Thanks > >>> Bernd. > >>> > >>> > >>> patch-pr86572.diff > >>> > >>> gcc: > >>> 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> > >>> > >>> PR tree-optimization/86572 > >>> * builtins.c (c_strlen): Handle negative offsets in a safe way. > >>> > >>> testsuite: > >>> 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> > >>> > >>> PR tree-optimization/86572 > >>> * gcc.dg/pr86572.c: New test. > >> OK. > >> jeff > > > > This caused: > > > > /export/gnu/import/git/gcc-test-ia32/src-trunk/gcc/testsuite/gcc.dg/warn-strlen-no-nul.c:56:1: > > internal compiler error: verify_gimple failed^M > > 0x8922dc4 verify_gimple_in_seq(gimple*)^M > > ../../src-trunk/gcc/tree-cfg.c:5082^M > > 0x86899d7 gimplify_body(tree_node*, bool)^M > > ../../src-trunk/gcc/gimplify.c:12859^M > > 0x8689b8b gimplify_function_tree(tree_node*)^M > > ../../src-trunk/gcc/gimplify.c:12949^M > > 0x84f7690 cgraph_node::analyze()^M > > ../../src-trunk/gcc/cgraphunit.c:667^M > > 0x84fa1d8 analyze_functions^M > > ../../src-trunk/gcc/cgraphunit.c:1126^M > > 0x84fadd3 symbol_table::finalize_compilation_unit()^M > > ../../src-trunk/gcc/cgraphunit.c:2833^M > > Please submit a full bug report,^M > > with preprocessed source if appropriate.^M > > Please include the complete backtrace with any bug report.^M > > See <https://gcc.gnu.org/bugs/> for instructions.^M > > compiler exited with status 1 > > FAIL: gcc.dg/warn-strlen-no-nul.c (internal compiler error) > > > > on i386. > > > > Ah yes thanks. > > This is caused by an incorrect folding in string_constant. > After stripping the type casts in the POINTER_PLUS_EXPR > we add the offset which is sizetype to what is left > over from arg1, which is probably even a correctness issue, > if the type cast was a narrowing one. > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu (this time > tested with RUNTESTFLAGS="--target_board=unix\{-m32,\}") > Is it OK for trunk? > I opened: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87940 to track it.
On 11/5/18 4:20 PM, Bernd Edlinger wrote: > On 11/5/18 1:28 AM, H.J. Lu wrote: >> On Sun, Nov 4, 2018 at 10:02 AM Jeff Law <law@redhat.com> wrote: >>> On 10/22/18 9:08 AM, Bernd Edlinger wrote: >>>> Hi! >>>> >>>> This makes c_strlen avoid an unsafe strlen folding of const arguments >>>> with non-const offset. Currently a negative out of bounds offset >>>> makes the strlen function return an extremely large number, and >>>> at the same time, prevents the VRP machinery, to determine the correct >>>> range if the strlen function in this case. >>>> >>>> Fixed by doing the whole computation in size_t and casting the >>>> result back to ssize_t. >>>> >>>> >>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >>>> Is it OK for trunk? >>>> >>>> >>>> Thanks >>>> Bernd. >>>> >>>> >>>> patch-pr86572.diff >>>> >>>> gcc: >>>> 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> >>>> >>>> PR tree-optimization/86572 >>>> * builtins.c (c_strlen): Handle negative offsets in a safe way. >>>> >>>> testsuite: >>>> 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> >>>> >>>> PR tree-optimization/86572 >>>> * gcc.dg/pr86572.c: New test. >>> OK. >>> jeff >> This caused: >> >> /export/gnu/import/git/gcc-test-ia32/src-trunk/gcc/testsuite/gcc.dg/warn-strlen-no-nul.c:56:1: >> internal compiler error: verify_gimple failed^M >> 0x8922dc4 verify_gimple_in_seq(gimple*)^M >> ../../src-trunk/gcc/tree-cfg.c:5082^M >> 0x86899d7 gimplify_body(tree_node*, bool)^M >> ../../src-trunk/gcc/gimplify.c:12859^M >> 0x8689b8b gimplify_function_tree(tree_node*)^M >> ../../src-trunk/gcc/gimplify.c:12949^M >> 0x84f7690 cgraph_node::analyze()^M >> ../../src-trunk/gcc/cgraphunit.c:667^M >> 0x84fa1d8 analyze_functions^M >> ../../src-trunk/gcc/cgraphunit.c:1126^M >> 0x84fadd3 symbol_table::finalize_compilation_unit()^M >> ../../src-trunk/gcc/cgraphunit.c:2833^M >> Please submit a full bug report,^M >> with preprocessed source if appropriate.^M >> Please include the complete backtrace with any bug report.^M >> See <https://gcc.gnu.org/bugs/> for instructions.^M >> compiler exited with status 1 >> FAIL: gcc.dg/warn-strlen-no-nul.c (internal compiler error) >> >> on i386. >> > Ah yes thanks. > > This is caused by an incorrect folding in string_constant. > After stripping the type casts in the POINTER_PLUS_EXPR > we add the offset which is sizetype to what is left > over from arg1, which is probably even a correctness issue, > if the type cast was a narrowing one. > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu (this time > tested with RUNTESTFLAGS="--target_board=unix\{-m32,\}") > Is it OK for trunk? > > > Thanks > Bernd. > > > > patch-fix-string-cst.diff > > 2018-11-05 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * expr.c (string_constant): Don't strip NOPS in subexpressions. > Fold PLUS_EXPR correctly. OK. jeff
gcc: 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> PR tree-optimization/86572 * builtins.c (c_strlen): Handle negative offsets in a safe way. testsuite: 2018-10-22 Bernd Edlinger <bernd.edlinger@hotmail.de> PR tree-optimization/86572 * gcc.dg/pr86572.c: New test. diff -Npur gcc/builtins.c gcc/builtins.c --- gcc/builtins.c 2018-10-21 10:41:08.000000000 +0200 +++ gcc/builtins.c 2018-10-22 10:45:25.846236006 +0200 @@ -734,11 +734,14 @@ c_strlen (tree src, int only_value, c_st of the string subtract the offset from the length of the string, and return that. Otherwise the length is zero. Take care to use SAVE_EXPR in case the OFFSET has side-effects. */ - tree offsave = TREE_SIDE_EFFECTS (byteoff) ? save_expr (byteoff) : byteoff; - offsave = fold_convert (ssizetype, offsave); + tree offsave = TREE_SIDE_EFFECTS (byteoff) ? save_expr (byteoff) + : byteoff; + offsave = fold_convert_loc (loc, sizetype, offsave); tree condexp = fold_build2_loc (loc, LE_EXPR, boolean_type_node, offsave, - build_int_cst (ssizetype, len)); - tree lenexp = size_diffop_loc (loc, ssize_int (len), offsave); + size_int (len)); + tree lenexp = fold_build2_loc (loc, MINUS_EXPR, sizetype, size_int (len), + offsave); + lenexp = fold_convert_loc (loc, ssizetype, lenexp); return fold_build3_loc (loc, COND_EXPR, ssizetype, condexp, lenexp, build_zero_cst (ssizetype)); } diff -Npur gcc/testsuite/gcc.dg/pr86572.c gcc/testsuite/gcc.dg/pr86572.c --- gcc/testsuite/gcc.dg/pr86572.c 1970-01-01 01:00:00.000000000 +0100 +++ gcc/testsuite/gcc.dg/pr86572.c 2018-10-22 12:35:25.771640418 +0200 @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-optimized" } */ + +const char buf[40] = "test"; +void test (int x) +{ + if (__builtin_strlen (buf + x) > 4) + __builtin_abort (); +} + +/* { dg-final { scan-tree-dump-not "abort" "optimized" } } */