[PING 2] [PATCH 3/4] enhance overflow and truncation detection in strncpy and strncat (PR 81117)

[Martin Sebor]

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
>