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

[Martin Sebor]

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 part --------------
A non-text attachment was scrubbed...
Name: gcc-81117-3.diff
Type: text/x-patch
Size: 72718 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20170815/a33604ab/attachment.bin>