From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 95067 invoked by alias); 6 Dec 2019 17:03:15 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 95057 invoked by uid 89); 6 Dec 2019 17:03:15 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=H*i:sk:81GZ6dM, H*i:2xDsiq2LJa0_T1, H*f:sk:81GZ6dM, H*MI:sk:81GZ6dM X-HELO: mail-pl1-f193.google.com Received: from mail-pl1-f193.google.com (HELO mail-pl1-f193.google.com) (209.85.214.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 06 Dec 2019 17:03:05 +0000 Received: by mail-pl1-f193.google.com with SMTP id q16so2955162plr.10 for ; Fri, 06 Dec 2019 09:03:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=1WWKjnDa8BXTOnri3grRnnaRBU0IxdmmLxGQBa/eChw=; b=mjsEGFskD1+6EO+TVYkmcxuMxWXxHd+25Pz6ubjn/X2h4ptEOrUhuP2N5OVXok1YPy uXZHxtjV/Ux98D6Ak23Q7uNAxkdvyI2JQC50dAeceIxBzWPoxqV0terw1oNtn1MVTGVp OKl5k8NOEuZE0wYvsKSFe4H9Yw4/Di733W1L2/lorguNr/1LItSKG6LmGRpmEbz3U4D+ EOB8jLBAtpnpbSbj17+mGNCN/NP6xKoLXbhWGt6h0y8fNXEhRZIrwP76z7T0M7G1oJvh k7xaxWYAvI8z3I3QP0jDtOHCAGVVpYUBF4kANmSRjfkbbfqraLR4YaU8EcsFuX2uiRRE aTtw== Return-Path: Received: from [192.168.0.41] (75-166-111-174.hlrn.qwest.net. [75.166.111.174]) by smtp.gmail.com with ESMTPSA id i9sm17439940pfk.24.2019.12.06.09.03.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 06 Dec 2019 09:03:02 -0800 (PST) Subject: Re: [PATCH] extend -Wstringop-overflow to allocated objects (PR 91582) To: Christophe Lyon Cc: Jeff Law , gcc-patches References: <5b68c166-e94b-2660-04f3-e3fafe69112c@gmail.com> <432df580-5630-a6ff-581d-731222a34669@redhat.com> From: Martin Sebor Message-ID: <164e3b65-760a-b3ca-074f-265ff0aa26d9@gmail.com> Date: Fri, 06 Dec 2019 17:03:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-12/txt/msg00436.txt.bz2 On 12/6/19 8:44 AM, Christophe Lyon wrote: > On Thu, 5 Dec 2019 at 02:37, Martin Sebor 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