Message ID | 20180313182258.15201-1-siddhesh@sourceware.org |
---|---|
State | New |
Headers | show |
Series | [v2] Fix bogus strncpy source length warning on source bound by constant | expand |
On Tue, Mar 13, 2018 at 7:22 PM, Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > Avoid issuing a bogus warning when the source of strncpy is bound by a > constant known to be less than the minimum size of the destination. > > Changes from v1: > > - Use range-info instead of the MIN_EXPR hack > - Get the minimum size of dst and check for NULL_TREE return > > The patch bootstraps successfully and introduces no new regressions in > the testsuite. > > gcc/ > > * tree-ssa-strlen.c (handle_builtin_stxncpy): Check bounds of > source length if available. > > gcc/testsuite/ > > * gcc.dg/builtin-stringop-chk-10.c: New test case. > --- > gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c | 17 +++++++++++++++++ > gcc/tree-ssa-strlen.c | 15 +++++++++++++++ > 2 files changed, 32 insertions(+) > create mode 100644 gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c > > diff --git a/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c b/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c > new file mode 100644 > index 00000000000..13e4bd2f049 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c > @@ -0,0 +1,17 @@ > +/* Bogus -Wstringop-overflow on strncpy when size is based on strlen but is > + bound by a constant. > + { dg-do compile } > + { dg-options "-O2 -Wstringop-overflow" } */ > + > +char dst[1024]; > + > +void > +f1 (const char *src) > +{ > + unsigned long limit = 512; > + unsigned long len = __builtin_strlen (src); /* { dg-bogus "length computed here" } */ > + if (len > limit) > + len = limit; > + > + __builtin_strncpy (dst, src, len); /* { dg-bogus "specified bound depends on the length of the source argument" } */ > +} > diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c > index 72f6a17cd32..265f351ea85 100644 > --- a/gcc/tree-ssa-strlen.c > +++ b/gcc/tree-ssa-strlen.c > @@ -2125,6 +2125,21 @@ handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *gsi) > return; > } > > + /* Don't bother about the strlen (SRC) in the LEN computation if the range of > + values for LEN ends up within dstsize. */ > + if (TREE_CODE (len) == SSA_NAME) > + { > + wide_int min, max; > + enum value_range_type vr = get_range_info (len, &min, &max); > + tree dstsize = compute_objsize (dst, 3); > + if (vr == VR_RANGE && dstsize) > + { > + tree len_max = wide_int_to_tree (TREE_TYPE (dstsize), max); > + if (tree_int_cst_lt (len_max, dstsize)) Instead of building a tree from max you should use if (wi::to_widest (max) < wi::to_widest (wi::to_wide (dstsize))) return; given compute_objsize is somewhat confused about the type it returns a widest_int compare is required. Note I'm not too familiar with tree-ssa-strlen.c nor this part of the warning code so I'll not approve the patch but after fixing that it looks techincally ok. Richard. > + return; > + } > + } > + > /* Retrieve the strinfo data for the string S that LEN was computed > from as some function F of strlen (S) (i.e., LEN need not be equal > to strlen(S)). */ > -- > 2.14.3 >
On Wednesday 14 March 2018 08:40 PM, Richard Biener wrote: > Instead of building a tree from max you should use > > if (wi::to_widest (max) < wi::to_widest (wi::to_wide (dstsize))) > return; > > given compute_objsize is somewhat confused about the type it returns > a widest_int compare is required. > > Note I'm not too familiar with tree-ssa-strlen.c nor this part of the > warning code > so I'll not approve the patch but after fixing that it looks techincally ok. Thanks, I'll post a fixed up version soon. Siddhesh
diff --git a/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c b/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c new file mode 100644 index 00000000000..13e4bd2f049 --- /dev/null +++ b/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c @@ -0,0 +1,17 @@ +/* Bogus -Wstringop-overflow on strncpy when size is based on strlen but is + bound by a constant. + { dg-do compile } + { dg-options "-O2 -Wstringop-overflow" } */ + +char dst[1024]; + +void +f1 (const char *src) +{ + unsigned long limit = 512; + unsigned long len = __builtin_strlen (src); /* { dg-bogus "length computed here" } */ + if (len > limit) + len = limit; + + __builtin_strncpy (dst, src, len); /* { dg-bogus "specified bound depends on the length of the source argument" } */ +} diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c index 72f6a17cd32..265f351ea85 100644 --- a/gcc/tree-ssa-strlen.c +++ b/gcc/tree-ssa-strlen.c @@ -2125,6 +2125,21 @@ handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *gsi) return; } + /* Don't bother about the strlen (SRC) in the LEN computation if the range of + values for LEN ends up within dstsize. */ + if (TREE_CODE (len) == SSA_NAME) + { + wide_int min, max; + enum value_range_type vr = get_range_info (len, &min, &max); + tree dstsize = compute_objsize (dst, 3); + if (vr == VR_RANGE && dstsize) + { + tree len_max = wide_int_to_tree (TREE_TYPE (dstsize), max); + if (tree_int_cst_lt (len_max, dstsize)) + return; + } + } + /* Retrieve the strinfo data for the string S that LEN was computed from as some function F of strlen (S) (i.e., LEN need not be equal to strlen(S)). */