From patchwork Thu Feb 18 22:30:58 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 1441907 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=LhEec+H7; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4DhTvV3kkQz9sCD for ; Fri, 19 Feb 2021 09:31:16 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0A01D3892454; Thu, 18 Feb 2021 22:31:04 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0A01D3892454 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1613687464; bh=oPSM0m3+m+5KNB3dfK5EVLkOIXdejQXAbod/0Ywa20s=; h=Subject:To:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=LhEec+H7d58FobiYcF9YAGMDa6+3WCpgUjuiaMP6HDkwQr2ijCQVjVj1IgKfUNnBM 8yQcM/q+1u1JSMddR6kYqw3MgWPaGA96aRE8rv9fRYjwlOqjS666+62k1IpdaYj5+g SbWrEclPzGawQ+n+I25mH4zCVU6GKMeLhHhIdtD0= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-qk1-x730.google.com (mail-qk1-x730.google.com [IPv6:2607:f8b0:4864:20::730]) by sourceware.org (Postfix) with ESMTPS id C0E6E383442F for ; Thu, 18 Feb 2021 22:31:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C0E6E383442F Received: by mail-qk1-x730.google.com with SMTP id x14so3861902qkm.2 for ; Thu, 18 Feb 2021 14:31:00 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:message-id:date:user-agent :mime-version:content-language; bh=oPSM0m3+m+5KNB3dfK5EVLkOIXdejQXAbod/0Ywa20s=; b=rRhMDBsS1xrRsFGiwHKOCJDcMUOH1hpxEtujqCHxjGG1zw9LRW8oZC/V/yKbNThEvr /50mIrwsO6iYhX/U7VaEVdOyv3S6dxOW2yZaLlLlRv2O5Gs5ptO3k4kKHYfXMsT6ZxsF Yj/2UbPIt2ZhqA7D6b2uyMg8UdZv3QABPzb4r1is9b7pInzMn2Efq+yPDScyDqaHzJ5L 4OEkO5R5ZiX6FETT4hZGcSpeCADLJaBjaoJBsAALMc9eU+8OKyyVkZaoM4SGnB1IcVBN PY3Pcgxwk6cju96msfzXOCgrD26wAs/OXmq82peVZz3lGs8cRfwTTHNEqK4FoSIyrFcy dU1A== X-Gm-Message-State: AOAM5311KP/vT0iBheJEaUh6wAqml42LWglF8CTrD3HK2WCyBcLk0Mzh H0lFZNZ1qldAJV/+/z0/24vIJaUicp8= X-Google-Smtp-Source: ABdhPJxMUCmUYW9aMEn5rC7jZCian2qFb+5KEnIaMBed5NVgxle8VIWHxSmcQ2tl0uM72emuiZIHIQ== X-Received: by 2002:a05:620a:755:: with SMTP id i21mr6570202qki.225.1613687460243; Thu, 18 Feb 2021 14:31:00 -0800 (PST) Received: from [192.168.0.41] ([97.118.127.0]) by smtp.gmail.com with ESMTPSA id m8sm2195348qkm.58.2021.02.18.14.30.59 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 18 Feb 2021 14:30:59 -0800 (PST) Subject: [PATCH] constrain writing one too many bytes" warning (PR 97631) To: gcc-patches Message-ID: <072fc2a1-da9d-853c-22a5-089857e8793b@gmail.com> Date: Thu, 18 Feb 2021 15:30:58 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 Content-Language: en-US X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Martin Sebor via Gcc-patches From: Martin Sebor Reply-To: Martin Sebor Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" The "writing one too many bytes" form of -Wstringop-overflow is designed to trigger for strcpy(d, s) calls into allocated destinations whose size is the result of (or depends on) strlen(s). But the warning is in code that's also called from handlers for bounded functions like memcpy and strncpy, and the code doesn't differentiate between the two kinds of callers, causing false positives. The attached patch corrects both the warning routine and its callers to properly distinguish these two classes of callers. In addition, it corrects a mistake where -Wstringop-overflow is being issued for destinations of unknown size instead of the more appropriate -Wstringop-truncation. Tested on x86_64-linux. The bug is a P2 10/11 regression and I'm looking to commit this change both into the trunk and 10-branch. Martin 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-65.c: New test. 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-65.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-65.c new file mode 100644 index 00000000000..0ecf51149e5 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-65.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..de88c42fe3f 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,10 @@ 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 string plus 1. */ + tree dstlenp1 = NULL_TREE, srclenp1 = NULL_TREE;; int didx = get_stridx (dst); if (strinfo *sidst = didx > 0 ? get_strinfo (didx) : NULL) @@ -3096,11 +3094,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 +3124,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 +3168,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 +3180,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 +3228,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 +3696,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). */