| Message ID | 5b68c166-e94b-2660-04f3-e3fafe69112c@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | extend -Wstringop-overflow to allocated objects (PR 91582) | expand |
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00652.html On 11/8/19 3:11 PM, Martin Sebor wrote: > Unless it's used with _FORTIFY_SOURCE, -Wstringop-overflow > doesn't consider out-of-bounds accesses to objects allocated > by alloca, malloc, other functions declared with attribute > alloc_size, or even VLAs with variable bounds. This was > a known limitation of the checks (done just before expansion) > relying on the the object size pass when they were introduced > in GCC 7. > > But since its introduction in GCC 7, the warning has evolved > beyond some of the limitations of the object size pass. Unlike > it, the warning considers non-constant offsets and stores with > non-constant sizes. Attached is a simple enhancement that > (finally) adds the ability to also detect overflow in allocated > objects to the warning. > > With the patch GCC detects the overflow in code like this: > > char* f (void) > { > char s[] = "12345"; > char *p = malloc (strlen (s)); > strcpy (p, s); // warning here > return p; > } > > but not (yet) in something like this: > > char* g (const char *s) > { > char *p = malloc (strlen (s)); > strcpy (p, s); // no warning (yet) > return p; > } > > and quite a few other examples. Doing better requires extending > the strlen pass. I'm working on this extension and expect to > submit a patch before stage 1 ends. > > Martin > > PS I was originally planning to do all the allocation checking > in the strlen pass but it occurred to me that by also enhancing > the compute_objsize function, all warnings that use it will > benefit. Besides -Wstringop-overflow this includes a subset > of -Warray-bounds, -Wformat-overflow, and -Wrestrict. It's > nice when a small enhancement has such a broad positive effect.
Ping: https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00652.html This change is independent of either of the two patches below: https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00429.html https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00652.html On 11/18/19 11:22 AM, Martin Sebor wrote: > Ping: https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00652.html > > On 11/8/19 3:11 PM, Martin Sebor wrote: >> Unless it's used with _FORTIFY_SOURCE, -Wstringop-overflow >> doesn't consider out-of-bounds accesses to objects allocated >> by alloca, malloc, other functions declared with attribute >> alloc_size, or even VLAs with variable bounds. This was >> a known limitation of the checks (done just before expansion) >> relying on the the object size pass when they were introduced >> in GCC 7. >> >> But since its introduction in GCC 7, the warning has evolved >> beyond some of the limitations of the object size pass. Unlike >> it, the warning considers non-constant offsets and stores with >> non-constant sizes. Attached is a simple enhancement that >> (finally) adds the ability to also detect overflow in allocated >> objects to the warning. >> >> With the patch GCC detects the overflow in code like this: >> >> char* f (void) >> { >> char s[] = "12345"; >> char *p = malloc (strlen (s)); >> strcpy (p, s); // warning here >> return p; >> } >> >> but not (yet) in something like this: >> >> char* g (const char *s) >> { >> char *p = malloc (strlen (s)); >> strcpy (p, s); // no warning (yet) >> return p; >> } >> >> and quite a few other examples. Doing better requires extending >> the strlen pass. I'm working on this extension and expect to >> submit a patch before stage 1 ends. >> >> Martin >> >> PS I was originally planning to do all the allocation checking >> in the strlen pass but it occurred to me that by also enhancing >> the compute_objsize function, all warnings that use it will >> benefit. Besides -Wstringop-overflow this includes a subset >> of -Warray-bounds, -Wformat-overflow, and -Wrestrict. It's >> nice when a small enhancement has such a broad positive effect. >
On 11/8/19 3:11 PM, Martin Sebor wrote: > Unless it's used with _FORTIFY_SOURCE, -Wstringop-overflow > doesn't consider out-of-bounds accesses to objects allocated > by alloca, malloc, other functions declared with attribute > alloc_size, or even VLAs with variable bounds. This was > a known limitation of the checks (done just before expansion) > relying on the the object size pass when they were introduced > in GCC 7. > > But since its introduction in GCC 7, the warning has evolved > beyond some of the limitations of the object size pass. Unlike > it, the warning considers non-constant offsets and stores with > non-constant sizes. Attached is a simple enhancement that > (finally) adds the ability to also detect overflow in allocated > objects to the warning. > > With the patch GCC detects the overflow in code like this: > > char* f (void) > { > char s[] = "12345"; > char *p = malloc (strlen (s)); > strcpy (p, s); // warning here > return p; > } > > but not (yet) in something like this: > > char* g (const char *s) > { > char *p = malloc (strlen (s)); > strcpy (p, s); // no warning (yet) > return p; > } > > and quite a few other examples. Doing better requires extending > the strlen pass. I'm working on this extension and expect to > submit a patch before stage 1 ends. > > Martin > > PS I was originally planning to do all the allocation checking > in the strlen pass but it occurred to me that by also enhancing > the compute_objsize function, all warnings that use it will > benefit. Besides -Wstringop-overflow this includes a subset > of -Warray-bounds, -Wformat-overflow, and -Wrestrict. It's > nice when a small enhancement has such a broad positive effect. > PR middle-end/91582 - missing heap overflow detection for strcpy > > gcc/ChangeLog: > > * builtins.c (gimple_call_alloc_size): New function. > (compute_objsize): Add argument. Call gimple_call_alloc_size. > Handle variable offsets and indices. > * builtins.h (gimple_call_alloc_size): Declare. > (compute_objsize): Add argument. > * tree-ssa-strlen.c (handle_store): Handle calls to allocated objects. > > gcc/testsuite/ChangeLog: > > * c-c++-common/Wstringop-truncation.c: Remove xfails. > * gcc/testsuite/g++.dg/ext/attr-alloc_size.C: Suppress -Warray-bounds. > * gcc.dg/Wstringop-overflow-22.c: New test. > * gcc/testsuite/gcc.dg/attr-alloc_size.c: Suppress -Warray-bounds. > * gcc/testsuite/gcc.dg/attr-copy-2.c: Same. > * gcc.dg/builtin-stringop-chk-5.c: Remove xfails. > * gcc.dg/builtin-stringop-chk-8.c: Same. Correct the text of expected > warnings. > * gcc.target/i386/pr82002-2a.c: Prune expected warning. > * gcc.target/i386/pr82002-2b.c: Same. [ ... ] > Index: gcc/builtins.c > =================================================================== > --- gcc/builtins.c (revision 277978) > +++ gcc/builtins.c (working copy) > @@ -3563,6 +3563,80 @@ check_access (tree exp, tree, tree, tree dstwrite, > return true; > } > > +/* If STMT is a call to an allocation function, returns the size > + of the object allocated by the call. */ > + > +tree > +gimple_call_alloc_size (gimple *stmt) > +{ > + tree size = gimple_call_arg (stmt, argidx1); > + tree n = argidx2 < nargs ? gimple_call_arg (stmt, argidx2) : integer_one_node; > + > + /* To handle ranges do the math in wide_int and return the product > + of the upper bounds as a constant. Ignore anti-ranges. */ > + wide_int rng1[2]; > + if (TREE_CODE (size) == INTEGER_CST) > + rng1[0] = rng1[1] = wi::to_wide (size); > + else if (TREE_CODE (size) != SSA_NAME > + || get_range_info (size, rng1, rng1 + 1) != VR_RANGE) > + return NULL_TREE; > + > + wide_int rng2[2]; > + if (TREE_CODE (n) == INTEGER_CST) > + rng2[0] = rng2[1] = wi::to_wide (n); > + else if (TREE_CODE (n) != SSA_NAME > + || get_range_info (n, rng2 + 1, rng2 + 1) != VR_RANGE) > + return NULL_TREE; Should that 2nd call to get_range_info be "get_range_info (n, rng2, rng2 + 1)? I don't think it makes any difference in practice due to the implementation of get_range_info, but if it wasn't intentional let's get it fixed. > Index: gcc/builtins.h > =================================================================== > --- gcc/builtins.h (revision 277978) > +++ gcc/builtins.h (working copy) > @@ -134,7 +134,8 @@ extern tree fold_call_stmt (gcall *, bool); > extern void set_builtin_user_assembler_name (tree decl, const char *asmspec); > extern bool is_simple_builtin (tree); > extern bool is_inexpensive_builtin (tree); > -extern tree compute_objsize (tree, int, tree * = NULL); > +tree gimple_call_alloc_size (gimple *); > +extern tree compute_objsize (tree, int, tree * = NULL, tree * = NULL); > > extern bool readonly_data_expr (tree exp); > extern bool init_target_chars (void); Is there a reason there's no "extern" on the gimple_call_alloc_size prototype? I think this is fine with those nits fixed. You'll have a minor merge conflict with the compute_objsize changes due to recent fixes in the same hunk of code, but I don't think it warrants reposting/resubmission. Jeff
On 12/2/19 10:06 AM, Jeff Law wrote: > On 11/8/19 3:11 PM, Martin Sebor wrote: >> Unless it's used with _FORTIFY_SOURCE, -Wstringop-overflow >> doesn't consider out-of-bounds accesses to objects allocated >> by alloca, malloc, other functions declared with attribute >> alloc_size, or even VLAs with variable bounds. This was >> a known limitation of the checks (done just before expansion) >> relying on the the object size pass when they were introduced >> in GCC 7. >> >> But since its introduction in GCC 7, the warning has evolved >> beyond some of the limitations of the object size pass. Unlike >> it, the warning considers non-constant offsets and stores with >> non-constant sizes. Attached is a simple enhancement that >> (finally) adds the ability to also detect overflow in allocated >> objects to the warning. >> >> With the patch GCC detects the overflow in code like this: >> >> char* f (void) >> { >> char s[] = "12345"; >> char *p = malloc (strlen (s)); >> strcpy (p, s); // warning here >> return p; >> } >> >> but not (yet) in something like this: >> >> char* g (const char *s) >> { >> char *p = malloc (strlen (s)); >> strcpy (p, s); // no warning (yet) >> return p; >> } >> >> and quite a few other examples. Doing better requires extending >> the strlen pass. I'm working on this extension and expect to >> submit a patch before stage 1 ends. >> >> Martin >> >> PS I was originally planning to do all the allocation checking >> in the strlen pass but it occurred to me that by also enhancing >> the compute_objsize function, all warnings that use it will >> benefit. Besides -Wstringop-overflow this includes a subset >> of -Warray-bounds, -Wformat-overflow, and -Wrestrict. It's >> nice when a small enhancement has such a broad positive effect. > >> PR middle-end/91582 - missing heap overflow detection for strcpy >> >> gcc/ChangeLog: >> >> * builtins.c (gimple_call_alloc_size): New function. >> (compute_objsize): Add argument. Call gimple_call_alloc_size. >> Handle variable offsets and indices. >> * builtins.h (gimple_call_alloc_size): Declare. >> (compute_objsize): Add argument. >> * tree-ssa-strlen.c (handle_store): Handle calls to allocated objects. >> >> gcc/testsuite/ChangeLog: >> >> * c-c++-common/Wstringop-truncation.c: Remove xfails. >> * gcc/testsuite/g++.dg/ext/attr-alloc_size.C: Suppress -Warray-bounds. >> * gcc.dg/Wstringop-overflow-22.c: New test. >> * gcc/testsuite/gcc.dg/attr-alloc_size.c: Suppress -Warray-bounds. >> * gcc/testsuite/gcc.dg/attr-copy-2.c: Same. >> * gcc.dg/builtin-stringop-chk-5.c: Remove xfails. >> * gcc.dg/builtin-stringop-chk-8.c: Same. Correct the text of expected >> warnings. >> * gcc.target/i386/pr82002-2a.c: Prune expected warning. >> * gcc.target/i386/pr82002-2b.c: Same. > [ ... ] > > >> Index: gcc/builtins.c >> =================================================================== >> --- gcc/builtins.c (revision 277978) >> +++ gcc/builtins.c (working copy) >> @@ -3563,6 +3563,80 @@ check_access (tree exp, tree, tree, tree dstwrite, >> return true; >> } >> >> +/* If STMT is a call to an allocation function, returns the size >> + of the object allocated by the call. */ >> + >> +tree >> +gimple_call_alloc_size (gimple *stmt) >> +{ >> + tree size = gimple_call_arg (stmt, argidx1); >> + tree n = argidx2 < nargs ? gimple_call_arg (stmt, argidx2) : integer_one_node; >> + >> + /* To handle ranges do the math in wide_int and return the product >> + of the upper bounds as a constant. Ignore anti-ranges. */ >> + wide_int rng1[2]; >> + if (TREE_CODE (size) == INTEGER_CST) >> + rng1[0] = rng1[1] = wi::to_wide (size); >> + else if (TREE_CODE (size) != SSA_NAME >> + || get_range_info (size, rng1, rng1 + 1) != VR_RANGE) >> + return NULL_TREE; >> + >> + wide_int rng2[2]; >> + if (TREE_CODE (n) == INTEGER_CST) >> + rng2[0] = rng2[1] = wi::to_wide (n); >> + else if (TREE_CODE (n) != SSA_NAME >> + || get_range_info (n, rng2 + 1, rng2 + 1) != VR_RANGE) >> + return NULL_TREE; > Should that 2nd call to get_range_info be "get_range_info (n, rng2, rng2 > + 1)? I don't think it makes any difference in practice due to the > implementation of get_range_info, but if it wasn't intentional let's get > it fixed. Yes, it should be. It's correct in my tree but didn't post the updated revision. > > >> Index: gcc/builtins.h >> =================================================================== >> --- gcc/builtins.h (revision 277978) >> +++ gcc/builtins.h (working copy) >> @@ -134,7 +134,8 @@ extern tree fold_call_stmt (gcall *, bool); >> extern void set_builtin_user_assembler_name (tree decl, const char *asmspec); >> extern bool is_simple_builtin (tree); >> extern bool is_inexpensive_builtin (tree); >> -extern tree compute_objsize (tree, int, tree * = NULL); >> +tree gimple_call_alloc_size (gimple *); >> +extern tree compute_objsize (tree, int, tree * = NULL, tree * = NULL); >> >> extern bool readonly_data_expr (tree exp); >> extern bool init_target_chars (void); > Is there a reason there's no "extern" on the gimple_call_alloc_size > prototype? I'm sure it was copied and pasted from the definition. It makes no difference either way so it didn't get caught by anything. > > I think this is fine with those nits fixed. You'll have a minor merge > conflict with the compute_objsize changes due to recent fixes in the > same hunk of code, but I don't think it warrants reposting/resubmission. > I've fixed the nits above and committed r278983 after retesting. This is an improvement in the buffer overflow detection but there is still the (arguably more important) second half of it: extending the strlen pass to detect the overflow that cannot be caught later (e.g., all stores by MEM_REFs are only handled in strlen): https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02340.html Martin
On Thu, 5 Dec 2019 at 02:37, Martin Sebor <msebor@gmail.com> wrote: > > On 12/2/19 10:06 AM, Jeff Law wrote: > > On 11/8/19 3:11 PM, Martin Sebor wrote: > >> Unless it's used with _FORTIFY_SOURCE, -Wstringop-overflow > >> doesn't consider out-of-bounds accesses to objects allocated > >> by alloca, malloc, other functions declared with attribute > >> alloc_size, or even VLAs with variable bounds. This was > >> a known limitation of the checks (done just before expansion) > >> relying on the the object size pass when they were introduced > >> in GCC 7. > >> > >> But since its introduction in GCC 7, the warning has evolved > >> beyond some of the limitations of the object size pass. Unlike > >> it, the warning considers non-constant offsets and stores with > >> non-constant sizes. Attached is a simple enhancement that > >> (finally) adds the ability to also detect overflow in allocated > >> objects to the warning. > >> > >> With the patch GCC detects the overflow in code like this: > >> > >> char* f (void) > >> { > >> char s[] = "12345"; > >> char *p = malloc (strlen (s)); > >> strcpy (p, s); // warning here > >> return p; > >> } > >> > >> but not (yet) in something like this: > >> > >> char* g (const char *s) > >> { > >> char *p = malloc (strlen (s)); > >> strcpy (p, s); // no warning (yet) > >> return p; > >> } > >> > >> and quite a few other examples. Doing better requires extending > >> the strlen pass. I'm working on this extension and expect to > >> submit a patch before stage 1 ends. > >> > >> Martin > >> > >> PS I was originally planning to do all the allocation checking > >> in the strlen pass but it occurred to me that by also enhancing > >> the compute_objsize function, all warnings that use it will > >> benefit. Besides -Wstringop-overflow this includes a subset > >> of -Warray-bounds, -Wformat-overflow, and -Wrestrict. It's > >> nice when a small enhancement has such a broad positive effect. > > > >> PR middle-end/91582 - missing heap overflow detection for strcpy > >> > >> gcc/ChangeLog: > >> > >> * builtins.c (gimple_call_alloc_size): New function. > >> (compute_objsize): Add argument. Call gimple_call_alloc_size. > >> Handle variable offsets and indices. > >> * builtins.h (gimple_call_alloc_size): Declare. > >> (compute_objsize): Add argument. > >> * tree-ssa-strlen.c (handle_store): Handle calls to allocated objects. > >> > >> gcc/testsuite/ChangeLog: > >> > >> * c-c++-common/Wstringop-truncation.c: Remove xfails. > >> * gcc/testsuite/g++.dg/ext/attr-alloc_size.C: Suppress -Warray-bounds. > >> * gcc.dg/Wstringop-overflow-22.c: New test. > >> * gcc/testsuite/gcc.dg/attr-alloc_size.c: Suppress -Warray-bounds. > >> * gcc/testsuite/gcc.dg/attr-copy-2.c: Same. > >> * gcc.dg/builtin-stringop-chk-5.c: Remove xfails. > >> * gcc.dg/builtin-stringop-chk-8.c: Same. Correct the text of expected > >> warnings. > >> * gcc.target/i386/pr82002-2a.c: Prune expected warning. > >> * gcc.target/i386/pr82002-2b.c: Same. > > [ ... ] > > > > > >> Index: gcc/builtins.c > >> =================================================================== > >> --- gcc/builtins.c (revision 277978) > >> +++ gcc/builtins.c (working copy) > >> @@ -3563,6 +3563,80 @@ check_access (tree exp, tree, tree, tree dstwrite, > >> return true; > >> } > >> > >> +/* If STMT is a call to an allocation function, returns the size > >> + of the object allocated by the call. */ > >> + > >> +tree > >> +gimple_call_alloc_size (gimple *stmt) > >> +{ > >> + tree size = gimple_call_arg (stmt, argidx1); > >> + tree n = argidx2 < nargs ? gimple_call_arg (stmt, argidx2) : integer_one_node; > >> + > >> + /* To handle ranges do the math in wide_int and return the product > >> + of the upper bounds as a constant. Ignore anti-ranges. */ > >> + wide_int rng1[2]; > >> + if (TREE_CODE (size) == INTEGER_CST) > >> + rng1[0] = rng1[1] = wi::to_wide (size); > >> + else if (TREE_CODE (size) != SSA_NAME > >> + || get_range_info (size, rng1, rng1 + 1) != VR_RANGE) > >> + return NULL_TREE; > >> + > >> + wide_int rng2[2]; > >> + if (TREE_CODE (n) == INTEGER_CST) > >> + rng2[0] = rng2[1] = wi::to_wide (n); > >> + else if (TREE_CODE (n) != SSA_NAME > >> + || get_range_info (n, rng2 + 1, rng2 + 1) != VR_RANGE) > >> + return NULL_TREE; > > Should that 2nd call to get_range_info be "get_range_info (n, rng2, rng2 > > + 1)? I don't think it makes any difference in practice due to the > > implementation of get_range_info, but if it wasn't intentional let's get > > it fixed. > > Yes, it should be. It's correct in my tree but didn't post > the updated revision. > > > > > > >> Index: gcc/builtins.h > >> =================================================================== > >> --- gcc/builtins.h (revision 277978) > >> +++ gcc/builtins.h (working copy) > >> @@ -134,7 +134,8 @@ extern tree fold_call_stmt (gcall *, bool); > >> extern void set_builtin_user_assembler_name (tree decl, const char *asmspec); > >> extern bool is_simple_builtin (tree); > >> extern bool is_inexpensive_builtin (tree); > >> -extern tree compute_objsize (tree, int, tree * = NULL); > >> +tree gimple_call_alloc_size (gimple *); > >> +extern tree compute_objsize (tree, int, tree * = NULL, tree * = NULL); > >> > >> extern bool readonly_data_expr (tree exp); > >> extern bool init_target_chars (void); > > Is there a reason there's no "extern" on the gimple_call_alloc_size > > prototype? > > I'm sure it was copied and pasted from the definition. It makes > no difference either way so it didn't get caught by anything. > > > > > I think this is fine with those nits fixed. You'll have a minor merge > > conflict with the compute_objsize changes due to recent fixes in the > > same hunk of code, but I don't think it warrants reposting/resubmission. > > > > I've fixed the nits above and committed r278983 after retesting. > Hi Martin, I've noticed that this patch introduces a new FAIL: gcc.dg/Warray-bounds-56.c (test for warnings, line 82) when GCC is configured with: --target arm-none-linux-gnueabihf --with-mode arm --with-cpu cortex-a5 --with-fpu vfpv3-d16-fp16 This test passes when using cortex-a9, a15, a57. Christophe > This is an improvement in the buffer overflow detection but there > is still the (arguably more important) second half of it: > extending the strlen pass to detect the overflow that cannot be > caught later (e.g., all stores by MEM_REFs are only handled in > strlen): https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02340.html > > Martin
On 12/6/19 8:44 AM, Christophe Lyon wrote: > On Thu, 5 Dec 2019 at 02:37, Martin Sebor <msebor@gmail.com> wrote: >> >> On 12/2/19 10:06 AM, Jeff Law wrote: >>> On 11/8/19 3:11 PM, Martin Sebor wrote: >>>> Unless it's used with _FORTIFY_SOURCE, -Wstringop-overflow >>>> doesn't consider out-of-bounds accesses to objects allocated >>>> by alloca, malloc, other functions declared with attribute >>>> alloc_size, or even VLAs with variable bounds. This was >>>> a known limitation of the checks (done just before expansion) >>>> relying on the the object size pass when they were introduced >>>> in GCC 7. >>>> >>>> But since its introduction in GCC 7, the warning has evolved >>>> beyond some of the limitations of the object size pass. Unlike >>>> it, the warning considers non-constant offsets and stores with >>>> non-constant sizes. Attached is a simple enhancement that >>>> (finally) adds the ability to also detect overflow in allocated >>>> objects to the warning. >>>> >>>> With the patch GCC detects the overflow in code like this: >>>> >>>> char* f (void) >>>> { >>>> char s[] = "12345"; >>>> char *p = malloc (strlen (s)); >>>> strcpy (p, s); // warning here >>>> return p; >>>> } >>>> >>>> but not (yet) in something like this: >>>> >>>> char* g (const char *s) >>>> { >>>> char *p = malloc (strlen (s)); >>>> strcpy (p, s); // no warning (yet) >>>> return p; >>>> } >>>> >>>> and quite a few other examples. Doing better requires extending >>>> the strlen pass. I'm working on this extension and expect to >>>> submit a patch before stage 1 ends. >>>> >>>> Martin >>>> >>>> PS I was originally planning to do all the allocation checking >>>> in the strlen pass but it occurred to me that by also enhancing >>>> the compute_objsize function, all warnings that use it will >>>> benefit. Besides -Wstringop-overflow this includes a subset >>>> of -Warray-bounds, -Wformat-overflow, and -Wrestrict. It's >>>> nice when a small enhancement has such a broad positive effect. >>> >>>> PR middle-end/91582 - missing heap overflow detection for strcpy >>>> >>>> gcc/ChangeLog: >>>> >>>> * builtins.c (gimple_call_alloc_size): New function. >>>> (compute_objsize): Add argument. Call gimple_call_alloc_size. >>>> Handle variable offsets and indices. >>>> * builtins.h (gimple_call_alloc_size): Declare. >>>> (compute_objsize): Add argument. >>>> * tree-ssa-strlen.c (handle_store): Handle calls to allocated objects. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> * c-c++-common/Wstringop-truncation.c: Remove xfails. >>>> * gcc/testsuite/g++.dg/ext/attr-alloc_size.C: Suppress -Warray-bounds. >>>> * gcc.dg/Wstringop-overflow-22.c: New test. >>>> * gcc/testsuite/gcc.dg/attr-alloc_size.c: Suppress -Warray-bounds. >>>> * gcc/testsuite/gcc.dg/attr-copy-2.c: Same. >>>> * gcc.dg/builtin-stringop-chk-5.c: Remove xfails. >>>> * gcc.dg/builtin-stringop-chk-8.c: Same. Correct the text of expected >>>> warnings. >>>> * gcc.target/i386/pr82002-2a.c: Prune expected warning. >>>> * gcc.target/i386/pr82002-2b.c: Same. >>> [ ... ] >>> >>> >>>> Index: gcc/builtins.c >>>> =================================================================== >>>> --- gcc/builtins.c (revision 277978) >>>> +++ gcc/builtins.c (working copy) >>>> @@ -3563,6 +3563,80 @@ check_access (tree exp, tree, tree, tree dstwrite, >>>> return true; >>>> } >>>> >>>> +/* If STMT is a call to an allocation function, returns the size >>>> + of the object allocated by the call. */ >>>> + >>>> +tree >>>> +gimple_call_alloc_size (gimple *stmt) >>>> +{ >>>> + tree size = gimple_call_arg (stmt, argidx1); >>>> + tree n = argidx2 < nargs ? gimple_call_arg (stmt, argidx2) : integer_one_node; >>>> + >>>> + /* To handle ranges do the math in wide_int and return the product >>>> + of the upper bounds as a constant. Ignore anti-ranges. */ >>>> + wide_int rng1[2]; >>>> + if (TREE_CODE (size) == INTEGER_CST) >>>> + rng1[0] = rng1[1] = wi::to_wide (size); >>>> + else if (TREE_CODE (size) != SSA_NAME >>>> + || get_range_info (size, rng1, rng1 + 1) != VR_RANGE) >>>> + return NULL_TREE; >>>> + >>>> + wide_int rng2[2]; >>>> + if (TREE_CODE (n) == INTEGER_CST) >>>> + rng2[0] = rng2[1] = wi::to_wide (n); >>>> + else if (TREE_CODE (n) != SSA_NAME >>>> + || get_range_info (n, rng2 + 1, rng2 + 1) != VR_RANGE) >>>> + return NULL_TREE; >>> Should that 2nd call to get_range_info be "get_range_info (n, rng2, rng2 >>> + 1)? I don't think it makes any difference in practice due to the >>> implementation of get_range_info, but if it wasn't intentional let's get >>> it fixed. >> >> Yes, it should be. It's correct in my tree but didn't post >> the updated revision. >> >>> >>> >>>> Index: gcc/builtins.h >>>> =================================================================== >>>> --- gcc/builtins.h (revision 277978) >>>> +++ gcc/builtins.h (working copy) >>>> @@ -134,7 +134,8 @@ extern tree fold_call_stmt (gcall *, bool); >>>> extern void set_builtin_user_assembler_name (tree decl, const char *asmspec); >>>> extern bool is_simple_builtin (tree); >>>> extern bool is_inexpensive_builtin (tree); >>>> -extern tree compute_objsize (tree, int, tree * = NULL); >>>> +tree gimple_call_alloc_size (gimple *); >>>> +extern tree compute_objsize (tree, int, tree * = NULL, tree * = NULL); >>>> >>>> extern bool readonly_data_expr (tree exp); >>>> extern bool init_target_chars (void); >>> Is there a reason there's no "extern" on the gimple_call_alloc_size >>> prototype? >> >> I'm sure it was copied and pasted from the definition. It makes >> no difference either way so it didn't get caught by anything. >> >>> >>> I think this is fine with those nits fixed. You'll have a minor merge >>> conflict with the compute_objsize changes due to recent fixes in the >>> same hunk of code, but I don't think it warrants reposting/resubmission. >>> >> >> I've fixed the nits above and committed r278983 after retesting. >> > > Hi Martin, > > I've noticed that this patch introduces a new FAIL: > gcc.dg/Warray-bounds-56.c (test for warnings, line 82) > when GCC is configured with: > --target arm-none-linux-gnueabihf > --with-mode arm > --with-cpu cortex-a5 > --with-fpu vfpv3-d16-fp16 > > This test passes when using cortex-a9, a15, a57. Thanks for the heads up. It's most likely due to the same issue as PR 92829 (seen on powerpc64*) that I started looking into yesterday. Martin > > Christophe > > >> This is an improvement in the buffer overflow detection but there >> is still the (arguably more important) second half of it: >> extending the strlen pass to detect the overflow that cannot be >> caught later (e.g., all stores by MEM_REFs are only handled in >> strlen): https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02340.html >> >> Martin
PR middle-end/91582 - missing heap overflow detection for strcpy gcc/ChangeLog: * builtins.c (gimple_call_alloc_size): New function. (compute_objsize): Add argument. Call gimple_call_alloc_size. Handle variable offsets and indices. * builtins.h (gimple_call_alloc_size): Declare. (compute_objsize): Add argument. * tree-ssa-strlen.c (handle_store): Handle calls to allocated objects. gcc/testsuite/ChangeLog: * c-c++-common/Wstringop-truncation.c: Remove xfails. * gcc/testsuite/g++.dg/ext/attr-alloc_size.C: Suppress -Warray-bounds. * gcc.dg/Wstringop-overflow-22.c: New test. * gcc/testsuite/gcc.dg/attr-alloc_size.c: Suppress -Warray-bounds. * gcc/testsuite/gcc.dg/attr-copy-2.c: Same. * gcc.dg/builtin-stringop-chk-5.c: Remove xfails. * gcc.dg/builtin-stringop-chk-8.c: Same. Correct the text of expected warnings. * gcc.target/i386/pr82002-2a.c: Prune expected warning. * gcc.target/i386/pr82002-2b.c: Same. Index: gcc/builtins.c =================================================================== --- gcc/builtins.c (revision 277978) +++ gcc/builtins.c (working copy) @@ -3563,6 +3563,80 @@ check_access (tree exp, tree, tree, tree dstwrite, return true; } +/* If STMT is a call to an allocation function, returns the size + of the object allocated by the call. */ + +tree +gimple_call_alloc_size (gimple *stmt) +{ + if (!stmt) + return NULL_TREE; + + tree allocfntype; + if (tree fndecl = gimple_call_fndecl (stmt)) + allocfntype = TREE_TYPE (fndecl); + else + allocfntype = gimple_call_fntype (stmt); + + if (!allocfntype) + return NULL_TREE; + + unsigned argidx1 = UINT_MAX, argidx2 = UINT_MAX; + tree at = lookup_attribute ("alloc_size", TYPE_ATTRIBUTES (allocfntype)); + if (!at) + { + if (!gimple_call_builtin_p (stmt, BUILT_IN_ALLOCA_WITH_ALIGN)) + return NULL_TREE; + + argidx1 = 0; + } + + unsigned nargs = gimple_call_num_args (stmt); + + if (argidx1 == UINT_MAX) + { + tree atval = TREE_VALUE (at); + if (!atval) + return NULL_TREE; + + argidx1 = TREE_INT_CST_LOW (TREE_VALUE (atval)) - 1; + if (nargs <= argidx1) + return NULL_TREE; + + atval = TREE_CHAIN (atval); + if (atval) + { + argidx2 = TREE_INT_CST_LOW (TREE_VALUE (atval)) - 1; + if (nargs <= argidx2) + return NULL_TREE; + } + } + + tree size = gimple_call_arg (stmt, argidx1); + tree n = argidx2 < nargs ? gimple_call_arg (stmt, argidx2) : integer_one_node; + + /* To handle ranges do the math in wide_int and return the product + of the upper bounds as a constant. Ignore anti-ranges. */ + wide_int rng1[2]; + if (TREE_CODE (size) == INTEGER_CST) + rng1[0] = rng1[1] = wi::to_wide (size); + else if (TREE_CODE (size) != SSA_NAME + || get_range_info (size, rng1, rng1 + 1) != VR_RANGE) + return NULL_TREE; + + wide_int rng2[2]; + if (TREE_CODE (n) == INTEGER_CST) + rng2[0] = rng2[1] = wi::to_wide (n); + else if (TREE_CODE (n) != SSA_NAME + || get_range_info (n, rng2 + 1, rng2 + 1) != VR_RANGE) + return NULL_TREE; + + const int prec = TYPE_PRECISION (sizetype); + rng1[1] = wide_int::from (rng1[1], prec, UNSIGNED); + rng2[1] = wide_int::from (rng2[1], prec, UNSIGNED); + return wide_int_to_tree (sizetype, rng1[1] * rng2[1]); +} + /* Helper to compute the size of the object referenced by the DEST expression which must have pointer type, using Object Size type OSTYPE (only the least significant 2 bits are used). Return @@ -3571,17 +3645,23 @@ check_access (tree exp, tree, tree, tree dstwrite, a non-constant offset in some range the returned value represents the largest size given the smallest non-negative offset in the range. If nonnull, set *PDECL to the decl of the referenced - subobject if it can be determined, or to null otherwise. + subobject if it can be determined, or to null otherwise. Likewise, + when POFF is nonnull *POFF is set to the offset into *PDECL. The function is intended for diagnostics and should not be used to influence code generation or optimization. */ tree -compute_objsize (tree dest, int ostype, tree *pdecl /* = NULL */) +compute_objsize (tree dest, int ostype, tree *pdecl /* = NULL */, + tree *poff /* = NULL */) { - tree dummy = NULL_TREE; + tree dummy_decl = NULL_TREE; if (!pdecl) - pdecl = &dummy; + pdecl = &dummy_decl; + tree dummy_off = size_zero_node; + if (!poff) + poff = &dummy_off; + unsigned HOST_WIDE_INT size; /* Only the two least significant bits are meaningful. */ @@ -3593,6 +3673,13 @@ tree if (TREE_CODE (dest) == SSA_NAME) { gimple *stmt = SSA_NAME_DEF_STMT (dest); + if (is_gimple_call (stmt)) + { + /* If STMT is a call to an allocation function get the size + from its argument(s). */ + return gimple_call_alloc_size (stmt); + } + if (!is_gimple_assign (stmt)) return NULL_TREE; @@ -3608,7 +3695,7 @@ tree tree off = gimple_assign_rhs2 (stmt); if (TREE_CODE (off) == INTEGER_CST) { - if (tree size = compute_objsize (dest, ostype, pdecl)) + if (tree size = compute_objsize (dest, ostype, pdecl, poff)) { wide_int wioff = wi::to_wide (off); wide_int wisiz = wi::to_wide (size); @@ -3619,10 +3706,16 @@ tree if (wi::sign_mask (wioff)) ; else if (wi::ltu_p (wioff, wisiz)) - return wide_int_to_tree (TREE_TYPE (size), - wi::sub (wisiz, wioff)); + { + *poff = size_binop (PLUS_EXPR, *poff, off); + return wide_int_to_tree (TREE_TYPE (size), + wi::sub (wisiz, wioff)); + } else - return size_zero_node; + { + *poff = size_binop (PLUS_EXPR, *poff, off); + return size_zero_node; + } } } else if (TREE_CODE (off) == SSA_NAME @@ -3644,10 +3737,18 @@ tree || wi::sign_mask (max)) ; else if (wi::ltu_p (min, wisiz)) - return wide_int_to_tree (TREE_TYPE (size), - wi::sub (wisiz, min)); + { + *poff = size_binop (PLUS_EXPR, *poff, + wide_int_to_tree (sizetype, min)); + return wide_int_to_tree (TREE_TYPE (size), + wi::sub (wisiz, min)); + } else - return size_zero_node; + { + *poff = size_binop (PLUS_EXPR, *poff, + wide_int_to_tree (sizetype, min)); + return size_zero_node; + } } } } @@ -3666,19 +3767,17 @@ tree { tree ref = TREE_OPERAND (dest, 0); tree off = TREE_OPERAND (dest, 1); - if (tree size = compute_objsize (ref, ostype, pdecl)) + if (tree size = compute_objsize (ref, ostype, pdecl, poff)) { - /* If the declaration of the destination object is known - to have zero size, return zero. */ - if (integer_zerop (size)) - return integer_zero_node; + /* A valid offset into a declared object cannot be negative. */ + if (tree_int_cst_sgn (*poff) < 0) + return size_zero_node; - if (TREE_CODE (off) != INTEGER_CST - || TREE_CODE (size) != INTEGER_CST) - return NULL_TREE; - + /* Adjust SIZE either up or down by the sum of *POFF and OFF + above. */ if (TREE_CODE (dest) == ARRAY_REF) { + /* Convert the array index into a byte offset. */ tree eltype = TREE_TYPE (dest); tree tpsize = TYPE_SIZE_UNIT (eltype); if (tpsize && TREE_CODE (tpsize) == INTEGER_CST) @@ -3687,9 +3786,74 @@ tree return NULL_TREE; } - if (tree_int_cst_lt (off, size)) - return fold_build2 (MINUS_EXPR, size_type_node, size, off); - return integer_zero_node; + wide_int offrng[2]; + if (TREE_CODE (off) == INTEGER_CST) + offrng[0] = offrng[1] = wi::to_wide (off); + else if (TREE_CODE (off) == SSA_NAME) + { + wide_int min, max; + enum value_range_kind rng + = get_range_info (off, offrng, offrng + 1); + if (rng != VR_RANGE) + return NULL_TREE; + } + else + return NULL_TREE; + + /* Convert to the same precision to keep wide_int from "helpfuly" + crashing whenever it sees other argumments. */ + offrng[0] = wide_int::from (offrng[0], ADDR_MAX_BITSIZE, SIGNED); + offrng[1] = wide_int::from (offrng[1], ADDR_MAX_BITSIZE, SIGNED); + + tree dstoff = *poff; + if (integer_zerop (*poff)) + *poff = off; + else if (!integer_zerop (off)) + { + *poff = fold_convert (ptrdiff_type_node, *poff); + off = fold_convert (ptrdiff_type_node, off); + *poff = size_binop (PLUS_EXPR, *poff, off); + } + + if (wi::sign_mask (offrng[0]) >= 0) + { + if (TREE_CODE (size) != INTEGER_CST) + return NULL_TREE; + + /* Return the difference between the size and the offset + or zero if the offset is greater. */ + wide_int wisize = wi::to_wide (size, ADDR_MAX_BITSIZE); + if (wi::ltu_p (wisize, offrng[0])) + return size_zero_node; + + return wide_int_to_tree (sizetype, wisize - offrng[0]); + } + + wide_int dstoffrng[2]; + if (TREE_CODE (dstoff) == INTEGER_CST) + dstoffrng[0] = dstoffrng[1] = wi::to_wide (dstoff); + else if (TREE_CODE (dstoff) == SSA_NAME) + { + enum value_range_kind rng + = get_range_info (dstoff, dstoffrng, dstoffrng + 1); + if (rng != VR_RANGE) + return NULL_TREE; + } + else + return NULL_TREE; + + dstoffrng[0] = wide_int::from (dstoffrng[0], ADDR_MAX_BITSIZE, SIGNED); + dstoffrng[1] = wide_int::from (dstoffrng[1], ADDR_MAX_BITSIZE, SIGNED); + + wide_int declsize = wi::to_wide (size); + if (wi::sign_mask (dstoffrng[0]) > 0) + declsize += dstoffrng[0]; + + offrng[1] += dstoffrng[1]; + if (wi::sign_mask (offrng[1]) < 0) + return size_zero_node; + + return wide_int_to_tree (sizetype, declsize); } return NULL_TREE; @@ -3701,14 +3865,13 @@ tree return component_ref_size (dest); } - if (TREE_CODE (dest) != ADDR_EXPR) - return NULL_TREE; + if (TREE_CODE (dest) == ADDR_EXPR) + dest = TREE_OPERAND (dest, 0); - tree ref = TREE_OPERAND (dest, 0); - if (DECL_P (ref)) + if (DECL_P (dest)) { - *pdecl = ref; - return DECL_SIZE_UNIT (ref); + *pdecl = dest; + return DECL_SIZE_UNIT (dest); } tree type = TREE_TYPE (dest); @@ -3718,7 +3881,7 @@ tree type = TYPE_MAIN_VARIANT (type); if (TREE_CODE (type) == ARRAY_TYPE - && !array_at_struct_end_p (ref)) + && !array_at_struct_end_p (dest)) { if (tree size = TYPE_SIZE_UNIT (type)) return TREE_CODE (size) == INTEGER_CST ? size : NULL_TREE; Index: gcc/builtins.h =================================================================== --- gcc/builtins.h (revision 277978) +++ gcc/builtins.h (working copy) @@ -134,7 +134,8 @@ extern tree fold_call_stmt (gcall *, bool); extern void set_builtin_user_assembler_name (tree decl, const char *asmspec); extern bool is_simple_builtin (tree); extern bool is_inexpensive_builtin (tree); -extern tree compute_objsize (tree, int, tree * = NULL); +tree gimple_call_alloc_size (gimple *); +extern tree compute_objsize (tree, int, tree * = NULL, tree * = NULL); extern bool readonly_data_expr (tree exp); extern bool init_target_chars (void); Index: gcc/testsuite/c-c++-common/Wstringop-truncation.c =================================================================== --- gcc/testsuite/c-c++-common/Wstringop-truncation.c (revision 277978) +++ gcc/testsuite/c-c++-common/Wstringop-truncation.c (working copy) @@ -300,8 +300,7 @@ void test_strncpy_array (Dest *pd, int i, const ch CPY (pd->a5, s, 5); /* { dg-warning "specified bound 5 equals destination size" } */ CPY (pd->a5, s, sizeof pd->a5); /* { dg-warning "specified bound 5 equals destination size" } */ - /* The following is not yet handled. */ - CPY (pd->a5 + i, s, sizeof pd->a5); /* { dg-warning "specified bound 5 equals destination size" "member array" { xfail *-*-* } } */ + CPY (pd->a5 + i, s, sizeof pd->a5); /* { dg-warning "specified bound 5 equals destination size" "member array" } */ /* Verify that a copy that nul-terminates is not diagnosed. */ CPY (pd->a5, "1234", sizeof pd->a5); @@ -425,7 +424,7 @@ void test_strncpy_alloc (const char* s) size_t n = 7; char *d = (char *)__builtin_malloc (n); - CPY (d, s, n); /* { dg-warning "specified bound 7 equals destination size" "bug 79016" { xfail *-*-* } } */ + CPY (d, s, n); /* { dg-warning "specified bound 7 equals destination size" } */ Dest *pd = (Dest *)__builtin_malloc (sizeof *pd * n); CPY (pd->a5, s, 5); /* { dg-warning "specified bound 5 equals destination size" } */ Index: gcc/testsuite/g++.dg/ext/attr-alloc_size.C =================================================================== --- gcc/testsuite/g++.dg/ext/attr-alloc_size.C (revision 277978) +++ gcc/testsuite/g++.dg/ext/attr-alloc_size.C (working copy) @@ -1,6 +1,6 @@ /* PR c++/87541 - ICE using a constant decl as an attribute alloc_size argument { dg-do compile } - { dg-options "-O2 -Wall" } */ + { dg-options "-O2 -Wall -Wno-array-bounds" } */ #define ALLOC_SIZE(N) __attribute__ ((alloc_size (N))) Index: gcc/testsuite/gcc.dg/Wstringop-overflow-22.c =================================================================== --- gcc/testsuite/gcc.dg/Wstringop-overflow-22.c (nonexistent) +++ gcc/testsuite/gcc.dg/Wstringop-overflow-22.c (working copy) @@ -0,0 +1,285 @@ +/* PR middle-end/91582 - missing heap overflow detection for strcpy + { dg-do compile } + { dg-options "-O2 -Wall -Wno-array-bounds -ftrack-macro-expansion=0" } */ + +#include "range.h" + +#define ATTR(...) __attribute__ ((__VA_ARGS__)) +#define NOIPA ATTR (noipa) + +extern void* alloca (size_t); +extern void* calloc (size_t, size_t); +extern void* malloc (size_t); + +extern ATTR (alloc_size (1), malloc) void* + alloc1 (size_t, int); +extern ATTR (alloc_size (2), malloc) void* + alloc2 (int, size_t); +extern ATTR (alloc_size (2, 4), malloc) void* + alloc2_4 (int, size_t, int, size_t); + +extern char* strcpy (char*, const char*); + +void sink (void*); + +#define S36 "0123456789abcdefghijklmnopqrstuvwxyz" +#define S(N) (S36 + sizeof S36 - N - 1) + +#define T(src, alloc) do { \ + char *s = src; \ + char *d = alloc; \ + strcpy (d, s); \ + sink (d); \ + } while (0) + + +NOIPA void test_strcpy_alloca (void) +{ + size_t r_0_1 = UR (0, 1); + size_t r_1_2 = UR (1, 2); + size_t r_2_3 = UR (2, 3); + + T (S (0), alloca (r_0_1)); + T (S (1), alloca (r_0_1)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloca (r_1_2)); + T (S (1), alloca (r_1_2)); + T (S (2), alloca (r_1_2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloca (r_2_3)); + T (S (2), alloca (r_2_3)); + T (S (3), alloca (r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } + T (S (9), alloca (r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } +} + +NOIPA void test_strcpy_calloc (void) +{ + size_t r_1_2 = UR (1, 2); + size_t r_2_3 = UR (2, 3); + + T (S (0), calloc (r_1_2, 1)); + T (S (1), calloc (r_1_2, 1)); + T (S (2), calloc (r_1_2, 1)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (2), calloc (r_2_3, 1)); + T (S (3), calloc (r_2_3, 1)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), calloc (1, r_1_2)); + T (S (1), calloc (1, r_1_2)); + T (S (2), calloc (1, r_1_2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (2), calloc (1, r_2_3)); + T (S (3), calloc (1, r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), calloc (r_1_2, 2)); + T (S (1), calloc (r_1_2, 2)); + T (S (2), calloc (r_1_2, 2)); + T (S (3), calloc (r_1_2, 2)); + T (S (4), calloc (r_1_2, 2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), calloc (r_2_3, 2)); + T (S (1), calloc (r_2_3, 2)); + T (S (2), calloc (r_2_3, 2)); + T (S (5), calloc (r_2_3, 2)); + T (S (6), calloc (r_2_3, 2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), calloc (r_1_2, 2)); + T (S (1), calloc (r_1_2, 2)); + T (S (2), calloc (r_1_2, 2)); + T (S (3), calloc (r_1_2, 2)); + T (S (4), calloc (r_1_2, 2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), calloc (r_2_3, 2)); + T (S (1), calloc (r_2_3, 2)); + T (S (2), calloc (r_2_3, 2)); + T (S (5), calloc (r_2_3, 2)); + T (S (6), calloc (r_2_3, 2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), calloc (r_1_2, r_2_3)); + T (S (1), calloc (r_1_2, r_2_3)); + T (S (2), calloc (r_1_2, r_2_3)); + T (S (3), calloc (r_1_2, r_2_3)); + T (S (4), calloc (r_1_2, r_2_3)); + T (S (5), calloc (r_1_2, r_2_3)); + T (S (6), calloc (r_1_2, r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } + T (S (9), calloc (r_1_2, r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } +} + + +NOIPA void test_strcpy_malloc (void) +{ + size_t r_0_1 = UR (0, 1); + size_t r_1_2 = UR (1, 2); + size_t r_2_3 = UR (2, 3); + + T (S (0), malloc (r_0_1)); + T (S (1), malloc (r_0_1)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), malloc (r_1_2)); + T (S (1), malloc (r_1_2)); + T (S (2), malloc (r_1_2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), malloc (r_2_3)); + T (S (2), malloc (r_2_3)); + T (S (3), malloc (r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } + T (S (9), malloc (r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } +} + + +NOIPA void test_strcpy_alloc1 (void) +{ + size_t r_0_1 = UR (0, 1); + size_t r_1_2 = UR (1, 2); + size_t r_2_3 = UR (2, 3); + +#define alloc1(n) alloc1 (n, 1) + + T (S (0), alloc1 (r_0_1)); + T (S (1), alloc1 (r_0_1)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloc1 (r_1_2)); + T (S (1), alloc1 (r_1_2)); + T (S (2), alloc1 (r_1_2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloc1 (r_2_3)); + T (S (2), alloc1 (r_2_3)); + T (S (3), alloc1 (r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } + T (S (9), alloc1 (r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } +} + +NOIPA void test_strcpy_alloc2 (void) +{ + size_t r_0_1 = UR (0, 1); + size_t r_1_2 = UR (1, 2); + size_t r_2_3 = UR (2, 3); + +#define alloc2(n) alloc2 (1, n) + + T (S (0), alloc1 (r_0_1)); + T (S (1), alloc1 (r_0_1)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloc1 (r_1_2)); + T (S (1), alloc1 (r_1_2)); + T (S (2), alloc1 (r_1_2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloc1 (r_2_3)); + T (S (2), alloc1 (r_2_3)); + T (S (3), alloc1 (r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } + T (S (9), alloc1 (r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } +} + + +NOIPA void test_strcpy_alloc2_4 (void) +{ + size_t r_1_2 = UR (1, 2); + size_t r_2_3 = UR (2, 3); + +#define alloc2_4(n1, n2) alloc2_4 (1, n1, 2, n2) + + T (S (0), alloc2_4 (r_1_2, 1)); + T (S (1), alloc2_4 (r_1_2, 1)); + T (S (2), alloc2_4 (r_1_2, 1)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (2), alloc2_4 (r_2_3, 1)); + T (S (3), alloc2_4 (r_2_3, 1)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloc2_4 (1, r_1_2)); + T (S (1), alloc2_4 (1, r_1_2)); + T (S (2), alloc2_4 (1, r_1_2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (2), alloc2_4 (1, r_2_3)); + T (S (3), alloc2_4 (1, r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloc2_4 (r_1_2, 2)); + T (S (1), alloc2_4 (r_1_2, 2)); + T (S (2), alloc2_4 (r_1_2, 2)); + T (S (3), alloc2_4 (r_1_2, 2)); + T (S (4), alloc2_4 (r_1_2, 2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloc2_4 (r_2_3, 2)); + T (S (1), alloc2_4 (r_2_3, 2)); + T (S (2), alloc2_4 (r_2_3, 2)); + T (S (5), alloc2_4 (r_2_3, 2)); + T (S (6), alloc2_4 (r_2_3, 2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloc2_4 (r_1_2, 2)); + T (S (1), alloc2_4 (r_1_2, 2)); + T (S (2), alloc2_4 (r_1_2, 2)); + T (S (3), alloc2_4 (r_1_2, 2)); + T (S (4), alloc2_4 (r_1_2, 2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloc2_4 (r_2_3, 2)); + T (S (1), alloc2_4 (r_2_3, 2)); + T (S (2), alloc2_4 (r_2_3, 2)); + T (S (5), alloc2_4 (r_2_3, 2)); + T (S (6), alloc2_4 (r_2_3, 2)); // { dg-warning "\\\[-Wstringop-overflow" } + + T (S (0), alloc2_4 (r_1_2, r_2_3)); + T (S (1), alloc2_4 (r_1_2, r_2_3)); + T (S (2), alloc2_4 (r_1_2, r_2_3)); + T (S (3), alloc2_4 (r_1_2, r_2_3)); + T (S (4), alloc2_4 (r_1_2, r_2_3)); + T (S (5), alloc2_4 (r_1_2, r_2_3)); + T (S (6), alloc2_4 (r_1_2, r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } + T (S (9), alloc2_4 (r_1_2, r_2_3)); // { dg-warning "\\\[-Wstringop-overflow" } +} + +#undef T +#define T(T, src, n) do { \ + char *s = src; \ + T vla[n]; \ + char *d = (char*)vla; \ + strcpy (d, s); \ + sink (vla); \ + } while (0) + +NOIPA void test_strcpy_vla (void) +{ + size_t r_0_1 = UR (0, 1); + size_t r_1_2 = UR (1, 2); + size_t r_2_3 = UR (2, 3); + + T (char, S (0), r_0_1); + T (char, S (1), r_0_1); // { dg-warning "\\\[-Wstringop-overflow" } + + T (char, S (0), r_1_2); + T (char, S (1), r_1_2); + T (char, S (2), r_1_2); // { dg-warning "\\\[-Wstringop-overflow" } + + T (char, S (0), r_2_3); + T (char, S (2), r_2_3); + T (char, S (3), r_2_3); // { dg-warning "\\\[-Wstringop-overflow" } + T (char, S (9), r_2_3); // { dg-warning "\\\[-Wstringop-overflow" } + +#ifdef __INT16_TYPE__ + typedef __INT16_TYPE__ int16_t; + + T (int16_t, S (0), r_1_2); + T (int16_t, S (2), r_1_2); + T (int16_t, S (3), r_1_2); + T (int16_t, S (4), r_1_2); // { dg-warning "\\\[-Wstringop-overflow" } + T (int16_t, S (5), r_1_2); // { dg-warning "\\\[-Wstringop-overflow" } + T (int16_t, S (9), r_1_2); // { dg-warning "\\\[-Wstringop-overflow" } + + T (int16_t, S (0), r_2_3); + T (int16_t, S (2), r_2_3); + T (int16_t, S (3), r_2_3); + T (int16_t, S (4), r_2_3); + T (int16_t, S (5), r_2_3); + T (int16_t, S (6), r_2_3); // { dg-warning "\\\[-Wstringop-overflow" } +#endif + +#ifdef __INT32_TYPE__ + typedef __INT32_TYPE__ int32_t; + + T (int32_t, S ( 0), r_2_3); + T (int32_t, S ( 2), r_2_3); + T (int32_t, S ( 3), r_2_3); + T (int32_t, S ( 4), r_2_3); + T (int32_t, S ( 5), r_2_3); + T (int32_t, S ( 6), r_2_3); + T (int32_t, S (11), r_2_3); + T (int32_t, S (12), r_2_3); // { dg-warning "\\\[-Wstringop-overflow" } + T (int32_t, S (36), r_2_3); // { dg-warning "\\\[-Wstringop-overflow" } +#endif +} Index: gcc/testsuite/gcc.dg/attr-alloc_size.c =================================================================== --- gcc/testsuite/gcc.dg/attr-alloc_size.c (revision 277978) +++ gcc/testsuite/gcc.dg/attr-alloc_size.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */ +/* { dg-options "-O2 -Wall -Wno-array-bounds -ftrack-macro-expansion=0" } */ extern void abort (void); Index: gcc/testsuite/gcc.dg/attr-copy-2.c =================================================================== --- gcc/testsuite/gcc.dg/attr-copy-2.c (revision 277978) +++ gcc/testsuite/gcc.dg/attr-copy-2.c (working copy) @@ -2,7 +2,7 @@ Exercise attribute copy for functions. { dg-do compile } { dg-require-alias "" } - { dg-options "-O2 -Wall" } */ + { dg-options "-O2 -Wall -Wno-array-bounds" } */ #define Assert(expr) typedef char AssertExpr[2 * !!(expr) - 1] Index: gcc/testsuite/gcc.dg/builtin-stringop-chk-5.c =================================================================== --- gcc/testsuite/gcc.dg/builtin-stringop-chk-5.c (revision 277978) +++ gcc/testsuite/gcc.dg/builtin-stringop-chk-5.c (working copy) @@ -1,4 +1,4 @@ -/* Test exercising -Wrawmem-overflow and -Wstringop-overflow warnings. */ +/* Test exercising -Wstringop-overflow warnings. */ /* { dg-do compile } */ /* { dg-options "-O2 -Wstringop-overflow=1" } */ @@ -49,7 +49,7 @@ void test_memop_warn_local (const void *src) memcpy (a, src, n); /* { dg-warning "writing between 8 and 32 bytes into a region of size 4 overflows the destination" } */ escape (a, src); - /* At -Wrawmem-overflow=1 the destination is considered to be + /* At -Wstringop-overflow=1 the destination is considered to be the whole array and its size is therefore sizeof a. */ memcpy (&a[0], src, n); /* { dg-warning "writing between 8 and 32 bytes into a region of size 4 overflows the destination" } */ escape (a, src); @@ -110,12 +110,12 @@ void test_memop_warn_alloc (const void *src) struct A *a = __builtin_malloc (sizeof *a * 2); - memcpy (a, src, n); /* { dg-warning "writing between 8 and 32 bytes into a region of size 4 overflows the destination" "memcpy into allocated" { xfail *-*-*} } */ + memcpy (a, src, n); /* { dg-warning "writing between 8 and 32 bytes into a region of size 4 overflows the destination" "memcpy into allocated" } */ escape (a, src); - /* At -Wrawmem-overflow=1 the destination is considered to be + /* At -Wstringop-overflow=1 the destination is considered to be the whole array and its size is therefore sizeof a. */ - memcpy (&a[0], src, n); /* { dg-warning "writing between 8 and 32 bytes into a region of size 4 overflows the destination" "memcpy into allocated" { xfail *-*-*} } */ + memcpy (&a[0], src, n); /* { dg-warning "writing between 8 and 32 bytes into a region of size 4 overflows the destination" "memcpy into allocated" } */ escape (a, src); /* Verify the same as above but by writing into the first mmeber @@ -127,7 +127,7 @@ void test_memop_warn_alloc (const void *src) struct B *b = __builtin_malloc (sizeof *b * 2); - memcpy (&b[0], src, n); /* { dg-warning "writing between 12 and 32 bytes into a region of size 8 overflows the destination" "memcpy into allocated" { xfail *-*-*} } */ + memcpy (&b[0], src, n); /* { dg-warning "writing between 12 and 32 bytes into a region of size 8 overflows the destination" "memcpy into allocated" } */ escape (b); /* The following idiom of clearing multiple members of a struct is Index: gcc/testsuite/gcc.dg/builtin-stringop-chk-8.c =================================================================== --- gcc/testsuite/gcc.dg/builtin-stringop-chk-8.c (revision 277978) +++ gcc/testsuite/gcc.dg/builtin-stringop-chk-8.c (working copy) @@ -102,9 +102,9 @@ void test_memop_warn_alloc (void *p) struct A *a = __builtin_malloc (sizeof *a * 2); - memcpy (p, a, n); /* { dg-warning "reading between 8 and 32 bytes from region of size 4" "memcpy from allocated" { xfail *-*-*} } */ + memcpy (p, a, n); /* { dg-warning "reading between 8 and 32 bytes from a region of size 4" "memcpy from allocated" } */ - memcpy (p, &a[0], n); /* { dg-warning "reading between 8 and 32 bytes from a region of size 4" "memcpy from allocated" { xfail *-*-*} } */ + memcpy (p, &a[0], n); /* { dg-warning "reading between 8 and 32 bytes from a region of size 4" "memcpy from allocated" } */ memcpy (p, &a[0].a, n); /* { dg-warning "reading between 8 and 32 bytes from a region of size 4" "memcpy from allocated" { xfail *-*-*} } */ @@ -112,13 +112,13 @@ void test_memop_warn_alloc (void *p) struct B *b = __builtin_malloc (sizeof *b * 2); - memcpy (p, &b[0], n); /* { dg-warning "reading between 12 and 32 bytes from a region of size 8" "memcpy from allocated" { xfail *-*-*} } */ + memcpy (p, &b[0], n); /* { dg-warning "reading between 12 and 32 bytes from a region of size 8" "memcpy from allocated" } */ /* Verify memchr/memcmp. */ n = sizeof *b * 2 + 1; - memchr (b, 1, n); /* { dg-warning "reading 5 bytes from a region of size 4" "memcmp from allocated" { xfail *-*-* } } */ - memcmp (p, b, n); /* { dg-warning "reading 5 bytes from a region of size 4" "memcmp from allocated" { xfail *-*-* } } */ + memchr (b, 1, n); /* { dg-warning "reading 9 bytes from a region of size 8" "memcmp from allocated" } */ + memcmp (p, b, n); /* { dg-warning "reading 9 bytes from a region of size 8" "memcmp from allocated" } */ } Index: gcc/testsuite/gcc.target/i386/pr82002-2a.c =================================================================== --- gcc/testsuite/gcc.target/i386/pr82002-2a.c (revision 277978) +++ gcc/testsuite/gcc.target/i386/pr82002-2a.c (working copy) @@ -10,3 +10,5 @@ b () a (c); a (c); } + +/* { dg-prune-output "\\\[-Wstringop-overflow" } */ Index: gcc/testsuite/gcc.target/i386/pr82002-2b.c =================================================================== --- gcc/testsuite/gcc.target/i386/pr82002-2b.c (revision 277978) +++ gcc/testsuite/gcc.target/i386/pr82002-2b.c (working copy) @@ -10,3 +10,5 @@ b () a (c); a (c); } + +/* { dg-prune-output "\\\[-Wstringop-overflow" } */ Index: gcc/tree-ssa-strlen.c =================================================================== --- gcc/tree-ssa-strlen.c (revision 277978) +++ gcc/tree-ssa-strlen.c (working copy) @@ -4384,8 +4384,22 @@ handle_store (gimple_stmt_iterator *gsi, bool *zer stmt, lenrange[2], dstsize)) { if (decl) - inform (DECL_SOURCE_LOCATION (decl), - "destination object declared here"); + { + if (TREE_CODE (decl) == SSA_NAME) + { + gimple *stmt = SSA_NAME_DEF_STMT (decl); + if (is_gimple_call (stmt)) + { + tree allocfn = gimple_call_fndecl (stmt); + inform (gimple_location (stmt), + "destination region allocated by %qD " + "here", allocfn); + } + } + else + inform (DECL_SOURCE_LOCATION (decl), + "destination object declared here"); + } gimple_set_no_warning (stmt, true); } }
Unless it's used with _FORTIFY_SOURCE, -Wstringop-overflow doesn't consider out-of-bounds accesses to objects allocated by alloca, malloc, other functions declared with attribute alloc_size, or even VLAs with variable bounds. This was a known limitation of the checks (done just before expansion) relying on the the object size pass when they were introduced in GCC 7. But since its introduction in GCC 7, the warning has evolved beyond some of the limitations of the object size pass. Unlike it, the warning considers non-constant offsets and stores with non-constant sizes. Attached is a simple enhancement that (finally) adds the ability to also detect overflow in allocated objects to the warning. With the patch GCC detects the overflow in code like this: char* f (void) { char s[] = "12345"; char *p = malloc (strlen (s)); strcpy (p, s); // warning here return p; } but not (yet) in something like this: char* g (const char *s) { char *p = malloc (strlen (s)); strcpy (p, s); // no warning (yet) return p; } and quite a few other examples. Doing better requires extending the strlen pass. I'm working on this extension and expect to submit a patch before stage 1 ends. Martin PS I was originally planning to do all the allocation checking in the strlen pass but it occurred to me that by also enhancing the compute_objsize function, all warnings that use it will benefit. Besides -Wstringop-overflow this includes a subset of -Warray-bounds, -Wformat-overflow, and -Wrestrict. It's nice when a small enhancement has such a broad positive effect.