From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121441 invoked by alias); 5 Dec 2019 01:37:10 -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 121424 invoked by uid 89); 5 Dec 2019 01:37:09 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.8 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= X-HELO: mail-ot1-f68.google.com Received: from mail-ot1-f68.google.com (HELO mail-ot1-f68.google.com) (209.85.210.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 05 Dec 2019 01:37:07 +0000 Received: by mail-ot1-f68.google.com with SMTP id p8so1145068oth.10 for ; Wed, 04 Dec 2019 17:37:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:subject:to:references:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=+NrdKjrsdrET+LRiOI2SPEsQVi3LEZ6eDmJX/A5o/3U=; b=pu5BLeHO+4yi+NVW1ZVw/B8oKF8BcpsKyCwXb3N0utFkzPx3ZyUQka5yzMfS8nfPFM QLFMF5GWlG0pnG28J2R3rS2sxkwMNtfVeQZLZRBjp4J23YquuZtNM8lQrTetO9OazrCt Z56MmSPjEov0Ef1Sum6fZl8YRSuudS9PXX2OI7yuyBMcRAz25G7Or42nu85nmjVdbZf2 6a7d2RGw6m/FplJ9zStk9WEi/BKu4ezfcwzDfZLmG6q/lviVpLbQJBuYx016yHrGARKM xA2Y1j/bz67yPuvJlY7rOmOAKRclx7xLuAMXpc2elDnmzbodNPAUxuztFFGT6mxKjM+3 AbtA== 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 47sm2850083otu.37.2019.12.04.17.37.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 04 Dec 2019 17:37:04 -0800 (PST) From: Martin Sebor Subject: Re: [PATCH] extend -Wstringop-overflow to allocated objects (PR 91582) To: Jeff Law , gcc-patches References: <5b68c166-e94b-2660-04f3-e3fafe69112c@gmail.com> <432df580-5630-a6ff-581d-731222a34669@redhat.com> Message-ID: Date: Thu, 05 Dec 2019 01:37: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: <432df580-5630-a6ff-581d-731222a34669@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-12/txt/msg00263.txt.bz2 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. 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