From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67856 invoked by alias); 19 Sep 2017 15:44:48 -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 67847 invoked by uid 89); 19 Sep 2017 15:44:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=intervening, H*Ad:U*msebor, 81849 X-HELO: mail-io0-f173.google.com Received: from mail-io0-f173.google.com (HELO mail-io0-f173.google.com) (209.85.223.173) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 19 Sep 2017 15:44:46 +0000 Received: by mail-io0-f173.google.com with SMTP id v36so81347ioi.1 for ; Tue, 19 Sep 2017 08:44:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=3Tdd7rNUAqNfZOsHyppJwYuK+0ZfzNF2+1TbMxiMOlI=; b=N+Ayy6o2eHIBp/h13VU9N6E+h8fry1jniV1OFJX7t+nISUlnwkroHPdwE7xIKUyxSI gWfeD3rpcX/d2Omw0YCLTH+2orHwgRqvU61gsL01lgsBlSV3T082j/30bNZKUnGUJF69 Ec17++VVzLag4vOynDZdxOkkhY23NDvaJGgTG7p86FBDJ29MqdH/+EMks1RBHin+GQ4W 5zJpfToROfrEqg3zbcxz1Cquitf9AIxdhAOej8IOAbogKQtecZ/hi++DJTuMqfOTg5qg XhzymYX+Que4d5VjxiU0xTCOHoDF/Z8rg8dPC35SZsZUtcZhfbDPE6mxbLCNAGBcmj9b FbQg== X-Gm-Message-State: AHPjjUiDbLjyB0AFBXdaV4Pw4PjnRxXeJI7GgeCGNKZGfBbYrZNzk2PP /+QLThuTNB7cV2szNMCF3m3o2g== X-Google-Smtp-Source: AOwi7QDnDUPdQr3lnMB8DCK6UrAnzVO5CSuESEXTcdMKfb+TudYzYKVWu5j8hAu+P4k72tLBLtqhXg== X-Received: by 10.202.73.65 with SMTP id w62mr2141257oia.163.1505835883883; Tue, 19 Sep 2017 08:44:43 -0700 (PDT) Received: from localhost.localdomain (174-16-109-244.hlrn.qwest.net. [174.16.109.244]) by smtp.gmail.com with ESMTPSA id f83sm363994oia.3.2017.09.19.08.44.42 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 19 Sep 2017 08:44:42 -0700 (PDT) Subject: [PING 3] [PATCH 3/4] enhance overflow and truncation detection in strncpy and strncat (PR 81117) To: Jeff Law , 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> <57c7c1f0-4081-6f43-e874-4da3f34f1907@gmail.com> <1ffdb8c3-04e1-26ca-ab07-4a6797df0dd4@gmail.com> <752c529c-7bb2-fae3-bdd6-e0ff842f4f75@gmail.com> From: Martin Sebor Message-ID: <0174ad12-928d-7d03-1b40-bd7a0b901d59@gmail.com> Date: Tue, 19 Sep 2017 15:44:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <752c529c-7bb2-fae3-bdd6-e0ff842f4f75@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg01268.txt.bz2 Ping #3: https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00912.html Thanks Martin On 08/28/2017 08:34 PM, Martin Sebor wrote: > Ping #2: https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00912.html > > On 08/23/2017 01:46 PM, Martin Sebor wrote: >> Ping: https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00912.html >> >> Jeff, is this version good to commit or are there any other >> changes you'd like to see? >> >> Martin >> >> On 08/14/2017 04:40 PM, Martin Sebor wrote: >>> On 08/10/2017 01:29 PM, Martin Sebor wrote: >>>>>> 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. >>>> >>>> You're right, there is an API for this (array_at_struct_end_p, >>>> as Richard pointed out). I didn't want to use it because it >>>> treats any array at the end of a struct as a flexible array >>>> member, but simple tests show that that's what -Wstringop- >>>> overflow does now, and it wasn't my intention to tighten up >>>> the checking under this change. It surprises me that no tests >>>> exposed this. Let me relax the check and think about proposing >>>> to tighten it up separately. >>> >>> Done in the attached patch. (I opened bug 81849 for the enhancement >>> to have -Wstringop-overflow diagnose overflows when writing to member >>> arrays bigger than 1 element even if they're last). >>> >>> (I've left the handling for zero size in place because GCC allows >>> global arrays to be declared to have zero elements.) >>> >>>> >>>>>> @@ -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. >>>> >>>> Corrected. The return value is unused by the only caller so >>>> there is no test to exercise it. >>> >>> Done in the attached patch. >>> >>>>>> +/* 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. >>>> >>>> Thanks. I'll have to remember this. >>> >>> I went with this simple approach for now since it worked for GDB. >>> If it turns out that there are important instances of this idiom >>> that rely on intervening statements the warning can be relaxed. >>> >>>>> 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. >>> >>>>>> /* 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'm not sure I understand why. It's a pre-existing function that >>>> just dispatches to the built-in handlers. We don't rename function >>>> callers each time we improve error/warning detection in some >>>> function they call (case in point: all the expanders in builtins.c) >>>> Why do it here? And what would be a suitable name? All that comes >>>> to my mind is awkward variations on strlen_optimize_stmt_and_warn. >>> >>> I've left the function name as is. If you feel strongly that >>> it needs to be renamed let me know. >>> >>> Martin >> >