From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 117915 invoked by alias); 10 Aug 2017 05:26:45 -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 117853 invoked by uid 89); 10 Aug 2017 05:26:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 10 Aug 2017 05:26:36 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5B56B53625; Thu, 10 Aug 2017 05:26:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5B56B53625 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=law@redhat.com Received: from localhost.localdomain (ovpn-116-95.phx2.redhat.com [10.3.116.95]) by smtp.corp.redhat.com (Postfix) with ESMTP id 17EB977C04; Thu, 10 Aug 2017 05:26:33 +0000 (UTC) Subject: Re: [PATCH 3/4] enhance overflow and truncation detection in strncpy and strncat (PR 81117) To: Martin Sebor , Gcc Patch List References: <13944863-99a8-4144-1703-c6e1a2f36425@gmail.com> <0bbc91cd-fcdb-be61-e1d0-4b230f23b1a9@redhat.com> <4f4fbd4c-cb46-b80d-5749-ebb6bb050bc4@gmail.com> From: Jeff Law Message-ID: Date: Thu, 10 Aug 2017 07:17:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00717.txt.bz2 On 08/06/2017 02:07 PM, Martin Sebor wrote: > Part 3 of the series contains the meat of the patch: the new > -Wstringop-truncation option, and enhancements to -Wstringop- > overflow, and -Wpointer-sizeof-memaccess to detect misuses of > strncpy and strncat. > > Martin > > gcc-81117-3.diff > > > PR c/81117 - Improve buffer overflow checking in strncpy > > gcc/ChangeLog: > > PR c/81117 > * builtins.c (compute_objsize): Handle arrays that > compute_builtin_object_size likes to fail for. Make extern. > * builtins.h (compute_objsize): Declare. > (check_strncpy_sizes): New function. > (expand_builtin_strncpy): Call check_strncpy_sizes. > * gimple-fold.c (gimple_fold_builtin_strncpy): Implement > -Wstringop-truncation. > (gimple_fold_builtin_strncat): Same. > * gimple.c (gimple_build_call_from_tree): Set call location. > * tree-ssa-strlen.c (strlen_to_stridx): New global variable. > (maybe_diag_bound_equal_length, is_strlen_related_p): New functions. > (handle_builtin_stxncpy, handle_builtin_strncat): Same. > (handle_builtin_strlen): Use strlen_to_stridx. > (strlen_optimize_stmt): Handle flavors of strncat, strncpy, and > stpncpy. > Use strlen_to_stridx. > (pass_strlen::execute): Release strlen_to_stridx. > * doc/invoke.texi (-Wsizeof-pointer-memaccess): Document enhancement. > (-Wstringop-truncation): Document new option. > > gcc/c-family/ChangeLog: > > PR c/81117 > * c-warn.c (sizeof_pointer_memaccess_warning): Handle arrays. > * c.opt (-Wstriingop-truncation): New option. > > gcc/testsuite/ChangeLog: > > PR c/81117 > * c-c++-common/Wsizeof-pointer-memaccess3.c: New test. > * c-c++-common/Wstringop-overflow.c: Same. > * c-c++-common/Wstringop-truncation.c: Same. > * c-c++-common/Wsizeof-pointer-memaccess2.c: Adjust. > * c-c++-common/attr-nonstring-2.c: New test. > * g++.dg/torture/Wsizeof-pointer-memaccess1.C: Adjust. > * g++.dg/torture/Wsizeof-pointer-memaccess2.C: Same. > * gcc.dg/torture/pr63554.c: Same. > * gcc.dg/Walloca-1.c: Disable macro tracking. > > diff --git a/gcc/builtins.c b/gcc/builtins.c > index 016f68d..1aa9e22 100644 > --- a/gcc/builtins.c > +++ b/gcc/builtins.c [ ... ] > + > + if (TREE_CODE (type) == ARRAY_TYPE) > + { > + /* Return the constant size unless it's zero (that's a zero-length > + array likely at the end of a struct). */ > + tree size = TYPE_SIZE_UNIT (type); > + if (size && TREE_CODE (size) == INTEGER_CST > + && !integer_zerop (size)) > + return size; > + } Q. Do we have a canonical test for the trailing array idiom? In some contexts isn't it size 1? ISTM This test needs slight improvement. Ideally we'd use some canonical test for detect the trailing array idiom rather than open-coding it here. You might look at the array index warnings in tree-vrp.c to see if it's got a canonical test you can call or factor and use. > @@ -3883,6 +3920,30 @@ expand_builtin_strncat (tree exp, rtx) > return NULL_RTX; > } > > +/* Helper to check the sizes of sequences and the destination of calls > + to __builtin_strncpy (DST, SRC, LEN) and __builtin___strncpy_chk. > + Returns true on success (no overflow warning), false otherwise. */ > + > +static bool > +check_strncpy_sizes (tree exp, tree dst, tree src, tree len) > +{ > + tree dstsize = compute_objsize (dst, warn_stringop_overflow - 1); > + > + if (!check_sizes (OPT_Wstringop_overflow_, > + exp, len, /*maxlen=*/NULL_TREE, src, dstsize)) > + return false; > + > + if (!dstsize || TREE_CODE (len) != INTEGER_CST) > + return true; > + > + if (tree_int_cst_lt (dstsize, len)) > + warning_at (EXPR_LOCATION (exp), OPT_Wstringop_truncation, > + "%K%qD specified bound %E exceeds destination size %E", > + exp, get_callee_fndecl (exp), len, dstsize); > + > + return true; So in the case where you issue the warning, what should the return value be? According to the comment it should be false. It looks like you got the wrong return value for the tree_int_cst_lt (dstsize, len) test. > > return false; > diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c > index b0563fe..ac6503f 100644 > --- a/gcc/tree-ssa-strlen.c > +++ b/gcc/tree-ssa-strlen.c > + > +/* A helper of handle_builtin_stxncpy. Check to see if the specified > + bound is a) equal to the size of the destination DST and if so, b) > + if it's immediately followed by DST[LEN - 1] = '\0'. If a) holds > + and b) does not, warn. Otherwise, do nothing. Return true if > + diagnostic has been issued. > + > + The purpose is to diagnose calls to strncpy and stpncpy that do > + not nul-terminate the copy while allowing for the idiom where > + such a call is immediately followed by setting the last element > + to nul, as in: > + char a[32]; > + strncpy (a, s, sizeof a); > + a[sizeof a - 1] = '\0'; > +*/ So using gsi_next to find the next statement could make the heuristic fail to find the a[sizeof a - 1] = '\0'; statement when debugging is enabled. gsi_next_nondebug would be better as it would skip over any debug insns. What might be even better would be to use the immediate uses of the memory tag. For your case there should be only one immediate use and it should point to the statement which NUL terminates the destination. Or maybe that would be worse in that you only want to allow this exception when the statements are consecutive. > + > + /* Look for dst[i] = '\0'; after the stxncpy() call and if found > + avoid the truncation warning. */ > + gsi_next (&gsi); > + gimple *next_stmt = gsi_stmt (gsi); Here's the gsi_next I'm referring to. > + else > + { > + /* The source length is uknown. Try to determine the destination s/uknown/unknown/ > /* Handle a memcpy-like ({mem{,p}cpy,__mem{,p}cpy_chk}) call. > If strlen of the second argument is known and length of the third argument > is that plus one, strlen of the first argument is the same after this > @@ -2512,6 +2789,19 @@ strlen_optimize_stmt (gimple_stmt_iterator *gsi) You still need to rename strlen_optimize_stmt since after your changes it does both optimizations and warnings. I think we're going to need one more iteration on this patch within the kit. I'm glazing over a bit tonight. jeff