From 7f5ff78ff3f971c11ec67f422b2fd34281db9123 Mon Sep 17 00:00:00 2001 From: Martin Sebor Date: Mon, 8 Mar 2021 13:28:52 -0700 Subject: [PATCH] PR middle-end/97631 - bogus "writing one too many bytes" warning for memcpy with strlen argument gcc/ChangeLog: PR middle-end/97631 * tree-ssa-strlen.c (maybe_warn_overflow): Test rawmem. (handle_builtin_stxncpy_strncat): Rename locals. Determine destination size from allocation calls. Issue a more appropriate kind of warning. (handle_builtin_memcpy): Pass true as rawmem to maybe_warn_overflow. (handle_builtin_memset): Same. gcc/testsuite/ChangeLog: PR middle-end/97631 * c-c++-common/Wstringop-overflow.c: Remove unexpected warnings. Add an xfail. * c-c++-common/Wstringop-truncation.c: Add expected warnings. * gcc.dg/Wstringop-overflow-10.c: Also enable -Wstringop-truncation. * gcc.dg/Wstringop-overflow-66.c: New test. * gcc.dg/tree-ssa/strncpy-2.c: Adjust expected warning. --- .../c-c++-common/Wstringop-overflow.c | 30 +-- .../c-c++-common/Wstringop-truncation.c | 9 +- gcc/testsuite/gcc.dg/Wstringop-overflow-10.c | 6 +- gcc/testsuite/gcc.dg/Wstringop-overflow-66.c | 180 ++++++++++++++++++ gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c | 4 +- gcc/tree-ssa-strlen.c | 60 +++--- 6 files changed, 242 insertions(+), 47 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/Wstringop-overflow-66.c diff --git a/gcc/testsuite/c-c++-common/Wstringop-overflow.c b/gcc/testsuite/c-c++-common/Wstringop-overflow.c index 53f5166f30a..5757a23141e 100644 --- a/gcc/testsuite/c-c++-common/Wstringop-overflow.c +++ b/gcc/testsuite/c-c++-common/Wstringop-overflow.c @@ -115,28 +115,31 @@ void test_strncpy (char **d, const char* s, int i) T (d, "123", sizeof "123"); T (d, ar, sizeof ar); - T (d, s, strlen (s)); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ + /* There is no overflow in the following calls but they are diagnosed + by -Wstringop-truncation. Verify that they aren'y also diagnosed + by -Wstringop-overflow. */ + T (d, s, strlen (s)); { - int n = strlen (s); /* { dg-message "length computed here" } */ - T (d, s, n); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ + int n = strlen (s); + T (d, s, n); } { - unsigned n = strlen (s); /* { dg-message "length computed here" } */ - T (d, s, n); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ + unsigned n = strlen (s); + T (d, s, n); } { size_t n; - n = strlen (s); /* { dg-message "length computed here" } */ - T (d, s, n); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ + n = strlen (s); + T (d, s, n); } { size_t n; - n = strlen (s) - 1; /* { dg-message "length computed here" } */ - T (d, s, n); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ + n = strlen (s) - 1; + T (d, s, n); } { @@ -148,11 +151,8 @@ void test_strncpy (char **d, const char* s, int i) { /* This use of strncpy is certainly dubious and it could well be - diagnosed by -Wstringop-truncation but it isn't. That it is - diagnosed with -Wstringop-overflow is more by accident than - by design. -Wstringop-overflow considers any dependency of - the bound on strlen(s) a potential bug. */ - size_t n = i < strlen (s) ? i : strlen (s); /* { dg-message "length computed here" } */ - T (d, s, n); /* { dg-message ".strncpy\[^\n\r]* specified bound depends on the length of the source argument" } */ + diagnosed by -Wstringop-truncation but it isn't. */ + size_t n = i < strlen (s) ? i : strlen (s); /* { dg-message "length computed here" "note" { xfail *-*-* } } */ + T (d, s, n); /* { dg-message ".strncpy\[^\n\r]* specified bound depends on the length of the source argument" "pr?????" { xfail *-*-* } } */ } } diff --git a/gcc/testsuite/c-c++-common/Wstringop-truncation.c b/gcc/testsuite/c-c++-common/Wstringop-truncation.c index f29eee29e85..114837b2b64 100644 --- a/gcc/testsuite/c-c++-common/Wstringop-truncation.c +++ b/gcc/testsuite/c-c++-common/Wstringop-truncation.c @@ -226,19 +226,18 @@ void test_strncpy_ptr (char *d, const char* s, const char *t, int i) } { - /* The following is likely buggy but there's no apparent truncation - so it's not diagnosed by -Wstringop-truncation. Instead, it is - diagnosed by -Wstringop-overflow (tested elsewhere). */ + /* The following truncates the terminating nul. The warning should + say that but doesn't. */ int n; n = strlen (s) - 1; - CPY (d, s, n); + CPY (d, s, n); /* { dg-warning "\\\[-Wstringop-truncation" } */ } { /* Same as above. */ size_t n; n = strlen (s) - 1; - CPY (d, s, n); + CPY (d, s, n); /* { dg-warning "\\\[-Wstringop-truncation" } */ } { diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-10.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-10.c index 2e22130fa7e..bace08ad5d3 100644 --- a/gcc/testsuite/gcc.dg/Wstringop-overflow-10.c +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-10.c @@ -1,5 +1,7 @@ -/* { dg-do compile } */ -/* { dg-options "-O2 -Wstringop-overflow" } */ +/* PR tree-optimization/89500 - ICE: tree check: expected integer_cst, + have ssa_name in get_len + { dg-do compile } + { dg-options "-O2 -Wstringop-overflow -Wstringop-truncation" } */ void foo (char *a) diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-66.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-66.c new file mode 100644 index 00000000000..0ecf51149e5 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-66.c @@ -0,0 +1,180 @@ +/* PR middle-end/97631 - bogus "writing one too many bytes" warning for + memcpy with strlen argument + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +#define NOIPA __attribute__ ((noipa)) + +typedef __SIZE_TYPE__ size_t; + +extern void* malloc (size_t); +extern void* memcpy (void*, const void*, size_t); +extern void* memmove (void*, const void*, size_t); +extern void* memset (void*, int, size_t); +extern char* strcpy (char*, const char*); +extern char* strncpy (char*, const char*, size_t); +extern size_t strlen (const char*); + + +NOIPA char* nowarn_strcpy (char *s) +{ + size_t n = strlen (s); + char *d = malloc (n + 1); + strcpy (d, s); + return d; +} + + +NOIPA char* warn_strcpy (char *s) +{ + size_t n = strlen (s); + char *d = malloc (n); + strcpy (d, s); // { dg-warning "\\\[-Wstringop-overflow" } + return d; +} + +NOIPA char* warn_strcpy_nz (char *s) +{ + size_t n = strlen (s); + if (n == 0) + return 0; + + char *d = malloc (n); + strcpy (d, s); // { dg-warning "\\\[-Wstringop-overflow" } + return d; +} + +NOIPA char* warn_strcpy_nn (char *s) +{ + size_t n = strlen (s); + char *d = malloc (n); + if (!d) + return 0; + + strcpy (d, s); // { dg-warning "\\\[-Wstringop-overflow" } + return d; +} + +NOIPA char* warn_strcpy_nz_nn (char *s) +{ + size_t n = strlen (s); + if (n == 0) + return 0; + + char *d = malloc (n); + if (!d) + return 0; + + strcpy (d, s); // { dg-warning "\\\[-Wstringop-overflow" } + return d; +} + + +NOIPA char* nowarn_strncpy_1 (char *s) +{ + /* There's no overflow or truncation below so verify there is no + warning either. */ + size_t n = strlen (s) + 1; + char *d = malloc (n); + strncpy (d, s, n); + return d; +} + + +NOIPA char* warn_strncpy (char *s) +{ + size_t n = strlen (s); + char *d = malloc (n); + strncpy (d, s, n); // { dg-warning "\\\[-Wstringop-truncation" } + return d; +} + +NOIPA char* warn_strncpy_p1 (char *s) +{ + size_t n = strlen (s); + char *d = malloc (n + 1); + strncpy (d, s, n); // { dg-warning "\\\[-Wstringop-truncation" } + return d; +} + +NOIPA char* warn_strncpy_nz (char *s) +{ + size_t n = strlen (s); + if (n == 0) + return 0; + + char *d = malloc (n); + strncpy (d, s, n); // { dg-warning "\\\[-Wstringop-truncation" } + return d; + +} + + +NOIPA char* nowarn_memcpy (char *s) +{ + size_t n = strlen (s); + char *d = malloc (n); + memcpy (d, s, n); // { dg-bogus "\\\[-Wstringop-overflow" } + return d; +} + +NOIPA char* nowarn_memcpy_nz (char *s) +{ + size_t n = strlen (s); + if (n == 0) + return 0; + + char *d = malloc (n); + memcpy (d, s, n); // { dg-bogus "\\\[-Wstringop-overflow" } + return d; +} + +NOIPA char* nowarn_memcpy_nn (char *s) +{ + size_t n = strlen (s); + char *d = malloc (n); + if (!d) + return 0; + + memcpy (d, s, n); // { dg-bogus "\\\[-Wstringop-overflow" } + return d; +} + +NOIPA char* nowarn_memcpy_nn_nz (char *s) +{ + size_t n = strlen (s); + if (n == 0) + return 0; + + char *d = malloc (n); + if (!d) + return 0; + + memcpy (d, s, n); // { dg-bogus "\\\[-Wstringop-overflow" } + return d; + +} + + +NOIPA char* nowarn_memmove (char *s) +{ + size_t n = strlen (s); + if (n == 0) + return 0; + + char *d = malloc (n); + memmove (d, s, n); // { dg-bogus "\\\[-Wstringop-overflow" } + return d; +} + + +NOIPA char* nowarn_memset (char *s, int c) +{ + size_t n = strlen (s); + if (n == 0) + return 0; + + char *d = malloc (n); + memset (d, c, n); // { dg-bogus "\\\[-Wstringop-overflow" } + return d; +} diff --git a/gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c b/gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c index 2ef9cd61bee..e2216abbcbf 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c @@ -1,6 +1,6 @@ /* PR tree-optimization/83075 - Invalid strncpy optimization */ /* { dg-do run } */ -/* { dg-options "-O2 -Wstringop-overflow" } */ +/* { dg-options "-O2 -Wstringop-truncation" } */ typedef __SIZE_TYPE__ size_t; @@ -8,7 +8,7 @@ __attribute__((noipa)) size_t foo (char *p, char *q, size_t *r) { size_t n0 = __builtin_strlen (p); - __builtin_strncpy (q, p, n0); /* { dg-warning "specified bound depends on the length" } */ + __builtin_strncpy (q, p, n0); /* { dg-warning "\\\[-Wstringop-truncation" } */ size_t n1 = __builtin_strlen (p); *r = n0; return n1; diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c index 8912a113de9..cccd4a06771 100644 --- a/gcc/tree-ssa-strlen.c +++ b/gcc/tree-ssa-strlen.c @@ -1992,17 +1992,12 @@ maybe_warn_overflow (gimple *stmt, tree len, pointer_query &ptr_qry, && wi::leu_p (lenrng[1], spcrng[1])) return; - if (lenrng[0] == spcrng[1] - && (len != destsize - || !si || !is_strlen_related_p (si->ptr, len))) - return; - location_t loc = gimple_or_expr_nonartificial_location (stmt, dest); bool warned = false; if (wi::leu_p (lenrng[0], spcrng[1])) { if (len != destsize - && (!si || !is_strlen_related_p (si->ptr, len))) + && (!si || rawmem || !is_strlen_related_p (si->ptr, len))) return; warned = (writefn @@ -3083,7 +3078,12 @@ handle_builtin_stxncpy_strncat (bool append_p, gimple_stmt_iterator *gsi) tree dst = gimple_call_arg (stmt, 0); tree src = gimple_call_arg (stmt, 1); tree len = gimple_call_arg (stmt, 2); - tree dstsize = NULL_TREE, srcsize = NULL_TREE; + /* An upper bound of the size of the destination. */ + tree dstsize = NULL_TREE; + /* The length of the destination and source strings (plus 1 for those + whose FULL_STRING_P is set, i.e., whose length is exact rather than + a lower bound). */ + tree dstlenp1 = NULL_TREE, srclenp1 = NULL_TREE;; int didx = get_stridx (dst); if (strinfo *sidst = didx > 0 ? get_strinfo (didx) : NULL) @@ -3096,11 +3096,16 @@ handle_builtin_stxncpy_strncat (bool append_p, gimple_stmt_iterator *gsi) { /* String is known to be nul-terminated. */ tree type = TREE_TYPE (sidst->nonzero_chars); - dstsize = fold_build2 (PLUS_EXPR, type, sidst->nonzero_chars, + dstlenp1 = fold_build2 (PLUS_EXPR, type, sidst->nonzero_chars, build_int_cst (type, 1)); } else - dstsize = sidst->nonzero_chars; + dstlenp1 = sidst->nonzero_chars; + } + else if (TREE_CODE (sidst->ptr) == SSA_NAME) + { + gimple *def_stmt = SSA_NAME_DEF_STMT (sidst->ptr); + dstsize = gimple_call_alloc_size (def_stmt); } dst = sidst->ptr; @@ -3121,19 +3126,19 @@ handle_builtin_stxncpy_strncat (bool append_p, gimple_stmt_iterator *gsi) if (sisrc->full_string_p) { tree type = TREE_TYPE (sisrc->nonzero_chars); - srcsize = fold_build2 (PLUS_EXPR, type, sisrc->nonzero_chars, + srclenp1 = fold_build2 (PLUS_EXPR, type, sisrc->nonzero_chars, build_int_cst (type, 1)); } else - srcsize = sisrc->nonzero_chars; + srclenp1 = sisrc->nonzero_chars; } src = sisrc->ptr; } else - srcsize = NULL_TREE; + srclenp1 = NULL_TREE; - if (check_bounds_or_overlap (stmt, dst, src, dstsize, srcsize)) + if (check_bounds_or_overlap (stmt, dst, src, dstlenp1, srclenp1)) { gimple_set_no_warning (stmt, true); return; @@ -3165,9 +3170,10 @@ handle_builtin_stxncpy_strncat (bool append_p, gimple_stmt_iterator *gsi) /* When -Wstringop-truncation is set, try to determine truncation before diagnosing possible overflow. Truncation is implied by the LEN argument being equal to strlen(SRC), regardless of - whether its value is known. Otherwise, issue the more generic - -Wstringop-overflow which triggers for LEN arguments that in - any meaningful way depend on strlen(SRC). */ + whether its value is known. Otherwise, when appending, or + when copying into a destination of known size, issue the more + generic -Wstringop-overflow which triggers for LEN arguments + that in any meaningful way depend on strlen(SRC). */ if (!append_p && sisrc == silen && is_strlen_related_p (src, len) @@ -3176,11 +3182,19 @@ handle_builtin_stxncpy_strncat (bool append_p, gimple_stmt_iterator *gsi) "copying as many bytes from a string as its length", stmt, func)) warned = true; - else if (silen && is_strlen_related_p (src, silen->ptr)) - warned = warning_at (callloc, OPT_Wstringop_overflow_, - "%G%qD specified bound depends on the length " - "of the source argument", - stmt, func); + else if ((append_p || !dstsize || len == dstlenp1) + && silen && is_strlen_related_p (src, silen->ptr)) + { + /* Issue -Wstringop-overflow when appending or when writing into + a destination of a known size. Otherwise, when copying into + a destination of an unknown size, it's truncation. */ + int opt = (append_p || dstsize + ? OPT_Wstringop_overflow_ : OPT_Wstringop_truncation); + warned = warning_at (callloc, opt, + "%G%qD specified bound depends on the length " + "of the source argument", + stmt, func); + } if (warned) { location_t strlenloc = pss->second; @@ -3216,7 +3230,7 @@ handle_builtin_memcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi, if (olddsi != NULL && !integer_zerop (len)) { - maybe_warn_overflow (stmt, len, ptr_qry, olddsi, false, false); + maybe_warn_overflow (stmt, len, ptr_qry, olddsi, false, true); adjust_last_stmt (olddsi, stmt, false, ptr_qry); } @@ -3684,7 +3698,7 @@ handle_builtin_memset (gimple_stmt_iterator *gsi, bool *zero_write, tree memset_size = gimple_call_arg (memset_stmt, 2); /* Check for overflow. */ - maybe_warn_overflow (memset_stmt, memset_size, ptr_qry, NULL, false, false); + maybe_warn_overflow (memset_stmt, memset_size, ptr_qry, NULL, false, true); /* Bail when there is no statement associated with the destination (the statement may be null even when SI1->ALLOC is not). */