From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 85496 invoked by alias); 6 Dec 2019 15:44:23 -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 85468 invoked by uid 89); 6 Dec 2019 15:44:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-8.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=Unless, retesting X-HELO: mail-lj1-f195.google.com Received: from mail-lj1-f195.google.com (HELO mail-lj1-f195.google.com) (209.85.208.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 06 Dec 2019 15:44:16 +0000 Received: by mail-lj1-f195.google.com with SMTP id h23so8122517ljc.8 for ; Fri, 06 Dec 2019 07:44:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=i3mHnsLAFkPgsoZyPHCqUZB/vrhsNoDEDeTw3b9t770=; b=h6Iy/DlTPkrLU9nS6YEUu4hh4wjxdg13mYrb7ipxIDtOKxR0fqME/dBeIwv/oyHOMc opMG9No99wLmTfZTYKP1fZADikPBdp09kIvYFBby61xdKNd0GNPRXjcXgEm01cUCCsn3 8Do2K17AlGH6qKo3/klYS/cif6YhuXF7vUdbG9vsCYpM8a+KLaEjPtHPbtq/q1G98rOG 914CGRG5proUt3WSb5akEOrtFkQnZgCfqr7zKG2B91LbpQnqx9RnQzGP1/N5EZZuJqMS C0XaIwA5FNVE7NZO0dyZMegGs9uYyjAPccuEyzquPHg7ezEGcAZafFgTlTHZy8CYk0BG NsdA== MIME-Version: 1.0 References: <5b68c166-e94b-2660-04f3-e3fafe69112c@gmail.com> <432df580-5630-a6ff-581d-731222a34669@redhat.com> In-Reply-To: From: Christophe Lyon Date: Fri, 06 Dec 2019 15:44:00 -0000 Message-ID: Subject: Re: [PATCH] extend -Wstringop-overflow to allocated objects (PR 91582) To: Martin Sebor Cc: Jeff Law , gcc-patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-12/txt/msg00427.txt.bz2 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. 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