| Message ID | 24f17d93-7af3-2358-8e26-35d0cf23571b@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | avoid assuming strncpy arrays are nul-terminated (PR 89664) | expand |
On 3/11/19 8:27 PM, Martin Sebor wrote: > The -Wstringop-truncation handling for strncpy/stpncpy neglects > to consider that character arrays tracked by the strlen pass > are not necessarily nul-terminated. It unconditionally adds > one when computing the size of each sequence to account for > the nul. This leads to false positive warnings when checking > the validity of indices/pointers computed by the built-ins. > > The attached patch corrects this by adding one for the nul > only when the character array is known to be nul-terminated. > > Since GCC 7 does not issue the warning this is a 8/9 regression > that I would like to fix in both releases. Is the patch okay > for trunk/gcc-8-branch? > > Tested on x86_64-linux. > > Martin > > gcc-89644.diff > > PR tree-optimization/89644 - False-positive -Warray-bounds diagnostic on strncpy > > gcc/ChangeLog: > > PR tree-optimization/89644 > * tree-ssa-strlen.c (handle_builtin_stxncpy): Consider unterminated > arrays in determining sequence sizes in strncpy and stpncpy. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/89644 > * gcc.dg/Wstringop-truncation-8.c: New test. OK for the trunk as well as the affected release branches. jeff
On 3/19/19 8:33 AM, Jeff Law wrote: > On 3/11/19 8:27 PM, Martin Sebor wrote: >> The -Wstringop-truncation handling for strncpy/stpncpy neglects >> to consider that character arrays tracked by the strlen pass >> are not necessarily nul-terminated. It unconditionally adds >> one when computing the size of each sequence to account for >> the nul. This leads to false positive warnings when checking >> the validity of indices/pointers computed by the built-ins. >> >> The attached patch corrects this by adding one for the nul >> only when the character array is known to be nul-terminated. >> >> Since GCC 7 does not issue the warning this is a 8/9 regression >> that I would like to fix in both releases. Is the patch okay >> for trunk/gcc-8-branch? >> >> Tested on x86_64-linux. >> >> Martin >> >> gcc-89644.diff >> >> PR tree-optimization/89644 - False-positive -Warray-bounds diagnostic on strncpy >> >> gcc/ChangeLog: >> >> PR tree-optimization/89644 >> * tree-ssa-strlen.c (handle_builtin_stxncpy): Consider unterminated >> arrays in determining sequence sizes in strncpy and stpncpy. >> >> gcc/testsuite/ChangeLog: >> >> PR tree-optimization/89644 >> * gcc.dg/Wstringop-truncation-8.c: New test. > OK for the trunk as well as the affected release branches. I just noticed some some -Wrestrict test failures that I had missed in the test report. The patch wasn't quite right. Sorry about that. I'll fix it shortly. Martin
On 3/19/19 12:01 PM, Martin Sebor wrote: > On 3/19/19 8:33 AM, Jeff Law wrote: >> On 3/11/19 8:27 PM, Martin Sebor wrote: >>> The -Wstringop-truncation handling for strncpy/stpncpy neglects >>> to consider that character arrays tracked by the strlen pass >>> are not necessarily nul-terminated. It unconditionally adds >>> one when computing the size of each sequence to account for >>> the nul. This leads to false positive warnings when checking >>> the validity of indices/pointers computed by the built-ins. >>> >>> The attached patch corrects this by adding one for the nul >>> only when the character array is known to be nul-terminated. >>> >>> Since GCC 7 does not issue the warning this is a 8/9 regression >>> that I would like to fix in both releases. Is the patch okay >>> for trunk/gcc-8-branch? >>> >>> Tested on x86_64-linux. >>> >>> Martin >>> >>> gcc-89644.diff >>> >>> PR tree-optimization/89644 - False-positive -Warray-bounds diagnostic >>> on strncpy >>> >>> gcc/ChangeLog: >>> >>> PR tree-optimization/89644 >>> * tree-ssa-strlen.c (handle_builtin_stxncpy): Consider unterminated >>> arrays in determining sequence sizes in strncpy and stpncpy. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR tree-optimization/89644 >>> * gcc.dg/Wstringop-truncation-8.c: New test. >> OK for the trunk as well as the affected release branches. > > I just noticed some some -Wrestrict test failures that I had missed > in the test report. The patch wasn't quite right. Sorry about that. > I'll fix it shortly. The variable to use to check for nul-termination is full_string_p rather than endptr. I've fixed that in 269809. Martin
On 3/19/19 12:01 PM, Martin Sebor wrote: > On 3/19/19 8:33 AM, Jeff Law wrote: >> On 3/11/19 8:27 PM, Martin Sebor wrote: >>> The -Wstringop-truncation handling for strncpy/stpncpy neglects >>> to consider that character arrays tracked by the strlen pass >>> are not necessarily nul-terminated. It unconditionally adds >>> one when computing the size of each sequence to account for >>> the nul. This leads to false positive warnings when checking >>> the validity of indices/pointers computed by the built-ins. >>> >>> The attached patch corrects this by adding one for the nul >>> only when the character array is known to be nul-terminated. >>> >>> Since GCC 7 does not issue the warning this is a 8/9 regression >>> that I would like to fix in both releases. Is the patch okay >>> for trunk/gcc-8-branch? >>> >>> Tested on x86_64-linux. >>> >>> Martin >>> >>> gcc-89644.diff >>> >>> PR tree-optimization/89644 - False-positive -Warray-bounds diagnostic >>> on strncpy >>> >>> gcc/ChangeLog: >>> >>> PR tree-optimization/89644 >>> * tree-ssa-strlen.c (handle_builtin_stxncpy): Consider unterminated >>> arrays in determining sequence sizes in strncpy and stpncpy. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR tree-optimization/89644 >>> * gcc.dg/Wstringop-truncation-8.c: New test. >> OK for the trunk as well as the affected release branches. > > I just noticed some some -Wrestrict test failures that I had missed > in the test report. The patch wasn't quite right. Sorry about that. > I'll fix it shortly. Something like this perhaps? > # Comparing 4 common sum files > ## /bin/sh gcc/contrib/compare_tests /tmp/gxx-sum1.6751 /tmp/gxx-sum2.6751 > Tests that now fail, but worked before (15 tests): > > c-c++-common/Wrestrict.c -Wc++-compat (test for excess errors) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 811) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 812) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 813) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 819) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 820) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 827) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 828) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 867) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 875) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 876) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 884) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 885) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 893) > c-c++-common/Wrestrict.c -Wc++-compat strncpy (test for warnings, line 894) My testers just spit this out on a couple of targets in the last half hour or so. jeff
On 3/19/19 12:37 PM, Martin Sebor wrote: > On 3/19/19 12:01 PM, Martin Sebor wrote: >> On 3/19/19 8:33 AM, Jeff Law wrote: >>> On 3/11/19 8:27 PM, Martin Sebor wrote: >>>> The -Wstringop-truncation handling for strncpy/stpncpy neglects >>>> to consider that character arrays tracked by the strlen pass >>>> are not necessarily nul-terminated. It unconditionally adds >>>> one when computing the size of each sequence to account for >>>> the nul. This leads to false positive warnings when checking >>>> the validity of indices/pointers computed by the built-ins. >>>> >>>> The attached patch corrects this by adding one for the nul >>>> only when the character array is known to be nul-terminated. >>>> >>>> Since GCC 7 does not issue the warning this is a 8/9 regression >>>> that I would like to fix in both releases. Is the patch okay >>>> for trunk/gcc-8-branch? >>>> >>>> Tested on x86_64-linux. >>>> >>>> Martin >>>> >>>> gcc-89644.diff >>>> >>>> PR tree-optimization/89644 - False-positive -Warray-bounds >>>> diagnostic on strncpy >>>> >>>> gcc/ChangeLog: >>>> >>>> PR tree-optimization/89644 >>>> * tree-ssa-strlen.c (handle_builtin_stxncpy): Consider unterminated >>>> arrays in determining sequence sizes in strncpy and stpncpy. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> PR tree-optimization/89644 >>>> * gcc.dg/Wstringop-truncation-8.c: New test. >>> OK for the trunk as well as the affected release branches. >> >> I just noticed some some -Wrestrict test failures that I had missed >> in the test report. The patch wasn't quite right. Sorry about that. >> I'll fix it shortly. > > The variable to use to check for nul-termination is full_string_p > rather than endptr. I've fixed that in 269809. ACK. I'll restart those targets once 269809 lands in the git mirror. jeff
PR tree-optimization/89644 - False-positive -Warray-bounds diagnostic on strncpy gcc/ChangeLog: PR tree-optimization/89644 * tree-ssa-strlen.c (handle_builtin_stxncpy): Consider unterminated arrays in determining sequence sizes in strncpy and stpncpy. gcc/testsuite/ChangeLog: PR tree-optimization/89644 * gcc.dg/Wstringop-truncation-8.c: New test. diff --git a/gcc/testsuite/gcc.dg/Wstringop-truncation-8.c b/gcc/testsuite/gcc.dg/Wstringop-truncation-8.c new file mode 100644 index 00000000000..1745da50a37 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wstringop-truncation-8.c @@ -0,0 +1,94 @@ +/* PR tree-optimization/89644 - False-positive -Warray-bounds diagnostic + on strncpy + { dg-do compile } + { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */ + +#define NONSTR __attribute__ ((nonstring)) + +typedef __SIZE_TYPE__ size_t; + +size_t strlen (const char*); +extern char* stpncpy (char*, const char*, size_t); +extern char* strncpy (char*, const char*, size_t); + +void sink (char*, ...); + +char f0 (char *s) +{ + char a[6] NONSTR = { 1, 2, 3, 4, 5, 6 }; + if (*s) + strncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Warray-bounds" } */ + return a[0]; +} + +void f1 (char *s) +{ + char a[6] NONSTR = { 1, 2, 3, 4, 5, 6 }; + if (*s) + strncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Warray-bounds" } */ + sink (a); +} + +char f2 (void) +{ + char a[6] NONSTR = { 1, 2, 3, 4, 5, 6 }; + char b[6] NONSTR = { 6, 5, 4, 3, 2, 1 }; + strncpy (a, b + 1, 5); /* { dg-bogus "\\\[-Warray-bounds" } */ + return a[0]; +} + +void f3 (void) +{ + char a[6] NONSTR = { 1, 2, 3, 4, 5, 6 }; + char b[6] NONSTR = { 6, 5, 4, 3, 2, 1 }; + strncpy (a, b + 2, 4); /* { dg-bogus "\\\[-Warray-bounds" } */ + sink (a); +} + +void f4 (NONSTR char *d) +{ + char b[6] NONSTR = { 6, 5, 4, 3, 2, 1 }; + strncpy (d, b + 3, 3); /* { dg-bogus "\\\[-Warray-bounds" } */ + sink (d); +} + + +char g0 (char *s) +{ + char a[6] NONSTR = { 1, 2, 3, 4, 5, 6 }; + if (*s) + stpncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Warray-bounds" } */ + return a[0]; +} + +void g1 (char *s) +{ + char a[6] NONSTR = { 1, 2, 3, 4, 5, 6 }; + char *p = 0; + if (*s) + p = stpncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Warray-bounds" } */ + sink (a, p); +} + +char g2 (void) +{ + char a[6] NONSTR = { 1, 2, 3, 4, 5, 6 }; + char b[6] NONSTR = { 6, 5, 4, 3, 2, 1 }; + stpncpy (a, b + 1, 5); /* { dg-bogus "\\\[-Warray-bounds" } */ + return a[0]; +} + +void g3 (void) +{ + char a[6] NONSTR = { 1, 2, 3, 4, 5, 6 }; + char b[6] NONSTR = { 6, 5, 4, 3, 2, 1 }; + char *p = stpncpy (a, b + 2, 4); /* { dg-bogus "\\\[-Warray-bounds" } */ + sink (a, p); +} + +void g4 (NONSTR char *d) +{ + char b[6] NONSTR = { 6, 5, 4, 3, 2, 1 }; + char *p = stpncpy (d, b + 3, 3); /* { dg-bogus "\\\[-Warray-bounds" } */ + sink (d, p); +} diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c index 721832e3f19..1969c728dc7 100644 --- a/gcc/tree-ssa-strlen.c +++ b/gcc/tree-ssa-strlen.c @@ -2191,13 +2191,21 @@ handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *gsi) int didx = get_stridx (dst); if (strinfo *sidst = didx > 0 ? get_strinfo (didx) : NULL) { - /* Compute the size of the destination string including the NUL. */ + /* Compute the size of the destination string including the nul + if it is known to be nul-terminated. */ if (sidst->nonzero_chars) { - tree type = TREE_TYPE (sidst->nonzero_chars); - dstsize = fold_build2 (PLUS_EXPR, type, sidst->nonzero_chars, - build_int_cst (type, 1)); + if (sidst->endptr) + { + /* String is known to be nul-terminated. */ + tree type = TREE_TYPE (sidst->nonzero_chars); + dstsize = fold_build2 (PLUS_EXPR, type, sidst->nonzero_chars, + build_int_cst (type, 1)); + } + else + dstsize = sidst->nonzero_chars; } + dst = sidst->ptr; } @@ -2209,12 +2217,18 @@ handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *gsi) over the terminating nul so SISRC->DONT_INVALIDATE must be left clear. */ - /* Compute the size of the source string including the NUL. */ + /* Compute the size of the source string including the terminating + nul if its known to be nul-terminated. */ if (sisrc->nonzero_chars) { - tree type = TREE_TYPE (sisrc->nonzero_chars); - srcsize = fold_build2 (PLUS_EXPR, type, sisrc->nonzero_chars, - build_int_cst (type, 1)); + if (sisrc->endptr) + { + tree type = TREE_TYPE (sisrc->nonzero_chars); + srcsize = fold_build2 (PLUS_EXPR, type, sisrc->nonzero_chars, + build_int_cst (type, 1)); + } + else + srcsize = sisrc->nonzero_chars; } src = sisrc->ptr;