From: Martin Sebor <msebor@gmail.com>
To: Jeff Law <law@redhat.com>, Gcc Patch List <gcc-patches@gcc.gnu.org>
Subject: [PING 2] [PATCH 3/4] enhance overflow and truncation detection in strncpy and strncat (PR 81117)
Date: Tue, 29 Aug 2017 05:07:00 -0000 [thread overview]
Message-ID: <752c529c-7bb2-fae3-bdd6-e0ff842f4f75@gmail.com> (raw)
In-Reply-To: <1ffdb8c3-04e1-26ca-ab07-4a6797df0dd4@gmail.com>
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
>
next prev parent reply other threads:[~2017-08-29 2:35 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-08 20:45 [PATCH] " Martin Sebor
2017-07-18 2:51 ` [PING] " Martin Sebor
2017-07-25 3:10 ` [PING #2] " Martin Sebor
2017-07-31 17:29 ` Jeff Law
2017-07-31 19:42 ` Martin Sebor
2017-08-02 16:59 ` Jeff Law
2017-08-06 20:07 ` [PATCH 3/4] " Martin Sebor
2017-08-10 7:17 ` Jeff Law
2017-08-10 7:39 ` Richard Biener
2017-08-10 20:21 ` Martin Sebor
2017-08-15 3:06 ` Martin Sebor
2017-08-23 21:11 ` [PING] " Martin Sebor
2017-08-29 5:07 ` Martin Sebor [this message]
2017-09-19 15:44 ` [PING 3] " Martin Sebor
2017-09-26 2:27 ` [PING 4] " Martin Sebor
2017-10-02 22:15 ` Jeff Law
2017-10-21 0:26 ` Martin Sebor
2017-11-04 3:49 ` Jeff Law
2017-11-10 0:17 ` Martin Sebor
2017-11-10 0:31 ` Jeff Law
2017-11-14 9:24 ` [testsuite, committed] Require alloca for c-c++-common/Wstringop-truncation.c Tom de Vries
2017-11-15 15:30 ` [testsuite, committed] Compile strncpy-fix-1.c with -Wno-stringop-truncation Tom de Vries
2017-11-15 15:58 ` Martin Sebor
2017-08-06 20:07 ` [PATCH] enhance overflow and truncation detection in strncpy and strncat (PR 81117) Martin Sebor
2017-08-09 19:21 ` Jeff Law
2017-08-06 20:07 ` [PATCH 4/4] " Martin Sebor
2017-08-06 20:07 ` [PATCH 2/4] " Martin Sebor
2017-08-10 6:39 ` Jeff Law
2017-08-14 18:04 ` Martin Sebor
2017-08-14 18:29 ` Joseph Myers
2017-08-14 19:26 ` Martin Sebor
2017-08-14 20:41 ` Joseph Myers
2017-08-14 20:44 ` Martin Sebor
2017-08-15 3:03 ` Joseph Myers
2017-11-10 23:03 ` Marc Glisse
2017-11-11 21:10 ` Martin Sebor
2017-08-06 20:07 ` [PATCH 1/4] " Martin Sebor
2017-08-10 5:02 ` Jeff Law
2017-08-14 19:21 ` Martin Sebor
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=752c529c-7bb2-fae3-bdd6-e0ff842f4f75@gmail.com \
--to=msebor@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).