| Message ID | 1e65b499-e816-4709-8e58-f591dea6508a@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | avoid bogus -Wstringop-overflow for strncpy with _FORTIFY_SOURCE (PR 82646) | expand |
On 12/05/2017 04:47 PM, Martin Sebor wrote: > PR middle-end/82646 - bogus -Wstringop-overflow with > -D_FORTIFY_SOURCE=2 on strncpy with range to a member array, > > The bug points out a false positive in a call to strncpy() when > _FORTIFY_SOURCE is defined that doesn't exist otherwise. > > The problem is that __builtin_strncpy buffer overflow checking > is done along with the expansion of the intrinsic in one place > and __builtin___strncpy_chk is handled differently in another, > and the two are out of sync. > > The attached patch corrects the choice of arguments used for > overflow detection in __builtin___strncpy_chk and aligns > the diagnostics between the two intrinsics. > > Martin > > gcc-82646.diff > > > PR tree-optimization/82646 - bogus -Wstringop-overflow with -D_FORTIFY_SOURCE=2 on strncpy with range to a member array > > gcc/ChangeLog: > > PR tree-optimization/82646 > * builtins.c (maybe_emit_chk_warning): Use size as the bound for > strncpy, not maxlen. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/82646 > * gcc.dg/builtin-stringop-chk-1.c: Adjust. > * gcc.dg/builtin-stringop-chk-9.c: New test. OK. [ Happy to see something easy fly by that isn't SVE related :-) ] jeff
Hi Martin, On 6 December 2017 at 00:51, Jeff Law <law@redhat.com> wrote: > On 12/05/2017 04:47 PM, Martin Sebor wrote: >> PR middle-end/82646 - bogus -Wstringop-overflow with >> -D_FORTIFY_SOURCE=2 on strncpy with range to a member array, >> >> The bug points out a false positive in a call to strncpy() when >> _FORTIFY_SOURCE is defined that doesn't exist otherwise. >> >> The problem is that __builtin_strncpy buffer overflow checking >> is done along with the expansion of the intrinsic in one place >> and __builtin___strncpy_chk is handled differently in another, >> and the two are out of sync. >> >> The attached patch corrects the choice of arguments used for >> overflow detection in __builtin___strncpy_chk and aligns >> the diagnostics between the two intrinsics. >> >> Martin >> >> gcc-82646.diff >> >> >> PR tree-optimization/82646 - bogus -Wstringop-overflow with -D_FORTIFY_SOURCE=2 on strncpy with range to a member array >> >> gcc/ChangeLog: >> >> PR tree-optimization/82646 >> * builtins.c (maybe_emit_chk_warning): Use size as the bound for >> strncpy, not maxlen. >> >> gcc/testsuite/ChangeLog: >> >> PR tree-optimization/82646 >> * gcc.dg/builtin-stringop-chk-1.c: Adjust. >> * gcc.dg/builtin-stringop-chk-9.c: New test. > OK. > The new test fails on 32 bits platforms (arm, x86_32, aarch64 ilp32): FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 125) FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 133) FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 141) FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 149) Christophe > [ Happy to see something easy fly by that isn't SVE related :-) ] > > jeff
On 12/07/2017 06:46 AM, Christophe Lyon wrote: > Hi Martin, > > > On 6 December 2017 at 00:51, Jeff Law <law@redhat.com> wrote: >> On 12/05/2017 04:47 PM, Martin Sebor wrote: >>> PR middle-end/82646 - bogus -Wstringop-overflow with >>> -D_FORTIFY_SOURCE=2 on strncpy with range to a member array, >>> >>> The bug points out a false positive in a call to strncpy() when >>> _FORTIFY_SOURCE is defined that doesn't exist otherwise. >>> >>> The problem is that __builtin_strncpy buffer overflow checking >>> is done along with the expansion of the intrinsic in one place >>> and __builtin___strncpy_chk is handled differently in another, >>> and the two are out of sync. >>> >>> The attached patch corrects the choice of arguments used for >>> overflow detection in __builtin___strncpy_chk and aligns >>> the diagnostics between the two intrinsics. >>> >>> Martin >>> >>> gcc-82646.diff >>> >>> >>> PR tree-optimization/82646 - bogus -Wstringop-overflow with -D_FORTIFY_SOURCE=2 on strncpy with range to a member array >>> >>> gcc/ChangeLog: >>> >>> PR tree-optimization/82646 >>> * builtins.c (maybe_emit_chk_warning): Use size as the bound for >>> strncpy, not maxlen. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR tree-optimization/82646 >>> * gcc.dg/builtin-stringop-chk-1.c: Adjust. >>> * gcc.dg/builtin-stringop-chk-9.c: New test. >> OK. >> > > The new test fails on 32 bits platforms (arm, x86_32, aarch64 ilp32): > FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 125) > FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 133) > FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 141) > FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 149) I believe these failures were due to bug 83296 that Richard fixed earlier today. With the change in my tree, the test passes for me with the arm-linux-gnueabi cross-compiler. Can you please try again and let me know if the failures persist on any of your targets? Thanks Martin
On 7 December 2017 at 20:28, Martin Sebor <msebor@gmail.com> wrote: > On 12/07/2017 06:46 AM, Christophe Lyon wrote: >> >> Hi Martin, >> >> >> On 6 December 2017 at 00:51, Jeff Law <law@redhat.com> wrote: >>> >>> On 12/05/2017 04:47 PM, Martin Sebor wrote: >>>> >>>> PR middle-end/82646 - bogus -Wstringop-overflow with >>>> -D_FORTIFY_SOURCE=2 on strncpy with range to a member array, >>>> >>>> The bug points out a false positive in a call to strncpy() when >>>> _FORTIFY_SOURCE is defined that doesn't exist otherwise. >>>> >>>> The problem is that __builtin_strncpy buffer overflow checking >>>> is done along with the expansion of the intrinsic in one place >>>> and __builtin___strncpy_chk is handled differently in another, >>>> and the two are out of sync. >>>> >>>> The attached patch corrects the choice of arguments used for >>>> overflow detection in __builtin___strncpy_chk and aligns >>>> the diagnostics between the two intrinsics. >>>> >>>> Martin >>>> >>>> gcc-82646.diff >>>> >>>> >>>> PR tree-optimization/82646 - bogus -Wstringop-overflow with >>>> -D_FORTIFY_SOURCE=2 on strncpy with range to a member array >>>> >>>> gcc/ChangeLog: >>>> >>>> PR tree-optimization/82646 >>>> * builtins.c (maybe_emit_chk_warning): Use size as the bound for >>>> strncpy, not maxlen. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> PR tree-optimization/82646 >>>> * gcc.dg/builtin-stringop-chk-1.c: Adjust. >>>> * gcc.dg/builtin-stringop-chk-9.c: New test. >>> >>> OK. >>> >> >> The new test fails on 32 bits platforms (arm, x86_32, aarch64 ilp32): >> FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 125) >> FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 133) >> FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 141) >> FAIL: gcc.dg/builtin-stringop-chk-9.c (test for warnings, line 149) > > > I believe these failures were due to bug 83296 that Richard fixed > earlier today. With the change in my tree, the test passes for > me with the arm-linux-gnueabi cross-compiler. Can you please > try again and let me know if the failures persist on any of your > targets? > Indeed I can see it was fixed between r255460 and r255467, and Richard's patch is in the range (r255466). Thanks, Christophe > Thanks > Martin
PR tree-optimization/82646 - bogus -Wstringop-overflow with -D_FORTIFY_SOURCE=2 on strncpy with range to a member array gcc/ChangeLog: PR tree-optimization/82646 * builtins.c (maybe_emit_chk_warning): Use size as the bound for strncpy, not maxlen. gcc/testsuite/ChangeLog: PR tree-optimization/82646 * gcc.dg/builtin-stringop-chk-1.c: Adjust. * gcc.dg/builtin-stringop-chk-9.c: New test. diff --git a/gcc/builtins.c b/gcc/builtins.c index 097e1b7..6b25253 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -9861,6 +9861,8 @@ maybe_emit_chk_warning (tree exp, enum built_in_function fcode) (such as __strncat_chk) or null if the operation isn't bounded (such as __strcat_chk). */ tree maxlen = NULL_TREE; + /* The exact size of the access (such as in __strncpy_chk). */ + tree size = NULL_TREE; switch (fcode) { @@ -9888,7 +9890,7 @@ maybe_emit_chk_warning (tree exp, enum built_in_function fcode) case BUILT_IN_STRNCPY_CHK: case BUILT_IN_STPNCPY_CHK: srcstr = CALL_EXPR_ARG (exp, 1); - maxlen = CALL_EXPR_ARG (exp, 2); + size = CALL_EXPR_ARG (exp, 2); objsize = CALL_EXPR_ARG (exp, 3); break; @@ -9911,7 +9913,7 @@ maybe_emit_chk_warning (tree exp, enum built_in_function fcode) } check_sizes (OPT_Wstringop_overflow_, exp, - /*size=*/NULL_TREE, maxlen, srcstr, objsize); + size, maxlen, srcstr, objsize); } /* Emit warning if a buffer overflow is detected at compile time diff --git a/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c b/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c index 35cc6dc..10048f3 100644 --- a/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c +++ b/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c @@ -36,7 +36,7 @@ test (int arg, ...) vx = stpcpy (&buf2[18], "a"); vx = stpcpy (&buf2[18], "ab"); /* { dg-warning "writing 3" "stpcpy" } */ strncpy (&buf2[18], "a", 2); - strncpy (&buf2[18], "a", 3); /* { dg-warning "specified bound 3 exceeds destination size 2" "strncpy" } */ + strncpy (&buf2[18], "a", 3); /* { dg-warning "writing 3 bytes into a region of size 2" "strncpy" } */ strncpy (&buf2[18], "abc", 2); strncpy (&buf2[18], "abc", 3); /* { dg-warning "writing 3 " "strncpy" } */ memset (buf2, '\0', sizeof (buf2)); @@ -93,7 +93,7 @@ void test2 (const H h) { char c; - strncpy (&c, str, 3); /* { dg-warning "specified bound 3 exceeds destination size 1" "strncpy" } */ + strncpy (&c, str, 3); /* { dg-warning "writing 3 bytes into a region of size 1" "strncpy" } */ struct { char b[4]; } x; sprintf (x.b, "%s", "ABCD"); /* { dg-warning "writing 5" "sprintf" } */ diff --git a/gcc/testsuite/gcc.dg/builtin-stringop-chk-9.c b/gcc/testsuite/gcc.dg/builtin-stringop-chk-9.c new file mode 100644 index 0000000..b5464c2 --- /dev/null +++ b/gcc/testsuite/gcc.dg/builtin-stringop-chk-9.c @@ -0,0 +1,150 @@ +/* PR middle-end/82646 - bogus -Wstringop-overflow with -D_FORTIFY_SOURCE=2 + on strncpy with range to a member array + { dg-do compile } + { dg-options "-O2 -Wstringop-overflow -ftrack-macro-expansion=0" } */ + +#define bos(p) __builtin_object_size (p, 1) + +struct S { + char a[5]; + void (*pf)(void); +}; + +/* Verify that none of the string function calls below triggers a warning. */ + +char* test_stpncpy_const_nowarn (struct S *p) +{ + int n = sizeof p->a; + + return __builtin_stpncpy (p->a, "123456", n); +} + +char* test_strncpy_const_nowarn (struct S *p) +{ + int n = sizeof p->a; + + return __builtin_strncpy (p->a, "1234567", n); +} + +char* test_stpncpy_chk_const_nowarn (struct S *p) +{ + int n = sizeof p->a; + + return __builtin___stpncpy_chk (p->a, "12345678", n, bos (p->a)); +} + +char* test_strncpy_chk_const_nowarn (struct S *p) +{ + int n = sizeof p->a; + + return __builtin___strncpy_chk (p->a, "123456789", n, bos (p->a)); +} + + +char* test_stpncpy_range_nowarn (struct S *p, int n) +{ + if (n < sizeof p->a) + n = sizeof p->a; + + return __builtin_stpncpy (p->a, "123456", n); +} + +char* test_strncpy_range_nowarn (struct S *p, int n) +{ + if (n < sizeof p->a) + n = sizeof p->a; + + return __builtin_strncpy (p->a, "1234567", n); +} + +char* test_stpncpy_chk_range_nowarn (struct S *p, int n) +{ + if (n < sizeof p->a) + n = sizeof p->a; + + return __builtin___stpncpy_chk (p->a, "12345678", n, bos (p->a)); /* { dg-bogus "\\\[-Wstringop-overflow=]" } */ +} + +char* test_strncpy_chk_range_nowarn (struct S *p, int n) +{ + if (n < sizeof p->a) + n = sizeof p->a; + + return __builtin___strncpy_chk (p->a, "123456789", n, bos (p->a)); /* { dg-bogus "\\\[-Wstringop-overflow=]" } */ +} + + +/* Verify that all of the string function calls below trigger a warning. */ + +char* test_stpncpy_const_warn (struct S *p) +{ + int n = sizeof p->a; + + ++n; + + return __builtin_stpncpy (p->a, "123456", n); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ +} + +char* test_strncpy_const_warn (struct S *p) +{ + int n = sizeof p->a; + + /* A call to strncpy() with a known string and small bound is folded + into memcpy() which defeats the warning in this case since memcpy + uses Object Size Type 0, i.e., the largest object that p->a may + be a part of. Use a larger bound to get around this here. */ + n += 11; + + return __builtin_strncpy (p->a, "1234567", n); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ +} + +char* test_stpncpy_chk_const_warn (struct S *p) +{ + int n = sizeof p->a; + + ++n; + + return __builtin___stpncpy_chk (p->a, "12345678", n, bos (p->a)); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ +} + +char* test_strncpy_chk_const_warn (struct S *p) +{ + int n = sizeof p->a; + + ++n; + + return __builtin___strncpy_chk (p->a, "123456789", n, bos (p->a)); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ +} + + +char* test_stpncpy_range_warn (struct S *p, int n) +{ + if (n < sizeof p->a + 1) + n = sizeof p->a + 1; + + return __builtin_stpncpy (p->a, "123456", n); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ +} + +char* test_strncpy_range_warn (struct S *p, int n) +{ + if (n < sizeof p->a + 1) + n = sizeof p->a + 1; + + return __builtin_strncpy (p->a, "1234567", n); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ +} + +char* test_stpncpy_chk_range_warn (struct S *p, int n) +{ + if (n < sizeof p->a + 1) + n = sizeof p->a + 1; + + return __builtin___stpncpy_chk (p->a, "12345678", n, bos (p->a)); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ +} + +char* test_strncpy_chk_range_warn (struct S *p, int n) +{ + if (n < sizeof p->a + 1) + n = sizeof p->a + 1; + + return __builtin___strncpy_chk (p->a, "123456789", n, bos (p->a)); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ +}