From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21242 invoked by alias); 2 Dec 2019 17:06:35 -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 21227 invoked by uid 89); 2 Dec 2019 17:06:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,SPF_PASS autolearn=ham version=3.3.1 spammy=limitations, Attached, sk:attr-al, planning X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (205.139.110.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 02 Dec 2019 17:06:33 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575306391; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=Dsmo0kcwavbRRNsrWHS1yVSxV9WoG/gOwF8jfAOAo6I=; b=F2GBBPfkXVOFdpct7YXb06O6KvSEpHEmwZ+c+/5XUqjleBL7yMXSkRxFgDWy068NxsDGFV ZiupHQqqREewxqNjNi2HJLLm0NiY17LdUIwT2Q3VeYBro0Y4LSEXxSGZrrH9s+CSEUxdyp SXTju+VKRrP+80IznhtuALUrCOmYiRw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-220-remcAmKYO2q-x_F6ftRRJw-1; Mon, 02 Dec 2019 12:06:30 -0500 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5E78D19057A5; Mon, 2 Dec 2019 17:06:28 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-14.rdu2.redhat.com [10.10.112.14]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9110360C05; Mon, 2 Dec 2019 17:06:27 +0000 (UTC) Subject: Re: [PATCH] extend -Wstringop-overflow to allocated objects (PR 91582) To: Martin Sebor , gcc-patches References: <5b68c166-e94b-2660-04f3-e3fafe69112c@gmail.com> From: Jeff Law Message-ID: <432df580-5630-a6ff-581d-731222a34669@redhat.com> Date: Mon, 02 Dec 2019 17:06:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <5b68c166-e94b-2660-04f3-e3fafe69112c@gmail.com> X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2019-12/txt/msg00063.txt.bz2 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.=C2=A0 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. >=20 > But since its introduction in GCC 7, the warning has evolved > beyond some of the limitations of the object size pass.=C2=A0 Unlike > it, the warning considers non-constant offsets and stores with > non-constant sizes.=C2=A0 Attached is a simple enhancement that > (finally) adds the ability to also detect overflow in allocated > objects to the warning. >=20 > With the patch GCC detects the overflow in code like this: >=20 > =C2=A0 char* f (void) > =C2=A0 { > =C2=A0=C2=A0=C2=A0 char s[] =3D "12345"; > =C2=A0=C2=A0=C2=A0 char *p =3D malloc (strlen (s)); > =C2=A0=C2=A0=C2=A0 strcpy (p, s);=C2=A0=C2=A0 // warning here > =C2=A0=C2=A0=C2=A0 return p; > =C2=A0 } >=20 > but not (yet) in something like this: >=20 > =C2=A0 char* g (const char *s) > =C2=A0 { > =C2=A0=C2=A0=C2=A0 char *p =3D malloc (strlen (s)); > =C2=A0=C2=A0=C2=A0 strcpy (p, s);=C2=A0=C2=A0 // no warning (yet) > =C2=A0=C2=A0=C2=A0 return p; > =C2=A0 } >=20 > and quite a few other examples.=C2=A0 Doing better requires extending > the strlen pass.=C2=A0 I'm working on this extension and expect to > submit a patch before stage 1 ends. >=20 > Martin >=20 > 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.=C2=A0 Besides -Wstringop-overflow this includes a subset > of -Warray-bounds, -Wformat-overflow, and -Wrestrict.=C2=A0 It's > nice when a small enhancement has such a broad positive effect. > PR middle-end/91582 - missing heap overflow detection for strcpy >=20 > gcc/ChangeLog: >=20 > * 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 obj= ects. >=20 > gcc/testsuite/ChangeLog: >=20 > * c-c++-common/Wstringop-truncation.c: Remove xfails. > * gcc/testsuite/g++.dg/ext/attr-alloc_size.C: Suppress -Warray-bo= unds. > * 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 exp= ected > warnings. > * gcc.target/i386/pr82002-2a.c: Prune expected warning. > * gcc.target/i386/pr82002-2b.c: Same. [ ... ] > Index: gcc/builtins.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- gcc/builtins.c (revision 277978) > +++ gcc/builtins.c (working copy) > @@ -3563,6 +3563,80 @@ check_access (tree exp, tree, tree, tree dstwrite, > return true; > } >=20 > +/* 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 =3D gimple_call_arg (stmt, argidx1); > + tree n =3D 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) =3D=3D INTEGER_CST) > + rng1[0] =3D rng1[1] =3D wi::to_wide (size); > + else if (TREE_CODE (size) !=3D SSA_NAME > + || get_range_info (size, rng1, rng1 + 1) !=3D VR_RANGE) > + return NULL_TREE; > + > + wide_int rng2[2]; > + if (TREE_CODE (n) =3D=3D INTEGER_CST) > + rng2[0] =3D rng2[1] =3D wi::to_wide (n); > + else if (TREE_CODE (n) !=3D SSA_NAME > + || get_range_info (n, rng2 + 1, rng2 + 1) !=3D 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. > Index: gcc/builtins.h > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- 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 *asms= pec); > extern bool is_simple_builtin (tree); > extern bool is_inexpensive_builtin (tree); > -extern tree compute_objsize (tree, int, tree * =3D NULL); > +tree gimple_call_alloc_size (gimple *); > +extern tree compute_objsize (tree, int, tree * =3D NULL, tree * =3D NULL= ); >=20 > 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 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. Jeff