enhance overflow and truncation detection in strncpy and strncat (PR 81117)

> So I think the fixes exposed by the new warning are OK to go in as-is
> immediately if you wish to do so.  Minor questions on the actual
> improved warnings inline.
>

Sure, thanks.

>
>> -static inline tree
>> +static tree
>>  compute_objsize (tree dest, int ostype)
>>  {
...
>> +  type = TYPE_MAIN_VARIANT (type);
>> +
>> +  if (TREE_CODE (type) == ARRAY_TYPE)
>> +    return TYPE_SIZE_UNIT (type);
> So I *think* TYPE_SIZE_UNIT isn't necessarily guaranteed to be a
> INTEGER_CST, it could be a non-constant expression for the size.  Are
> the callers of compute_objsize prepared to handle that?  Just to be
> clear, I'd prefer to return TYPE_SIZE_UNIT even if it's not an
> INTEGER_CST, I'm just worried about the callers ability to handle that
> correctly.

They should be prepared for it.  If not, it's a bug.  I've added
a few more test cases though I'm not sure the case you're concerned
about actually arises (VLA sizes are represented as gimple calls to
__builtin_alloca_with_align so the code doesn't get this far).

>
>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index d0b9050..e4208d9 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -5170,6 +5170,57 @@ whether to issue a warning.  Similarly to @option{-Wstringop-overflow=3} this
>>  setting of the option may result in warnings for benign code.
>>  @end table
>>
>> +@item -Wstringop-truncation
>> +@opindex Wstringop-overflow
>> +@opindex Wno-stringop-overflow

Looks like I have a couple of typos up there.  The option name
should be the same in all three entries.  Let me fix that.

>> +Warn for calls to bounded string manipulation functions such as @code{strncat},
>> +@code{strncpy}, and @code{stpncpy} that may either truncate the copied string
>> +or leave the destination unchanged.
>> +
>> +In the following example, the call to @code{strncat} specifies the length
>> +of the source string as the bound.  If the destination contains a non-empty
>> +string the copy of the source will be truncated.  Therefore, the call is
>> +diagnosed.  To avoid the warning use @code{strlen (d) - 4)} as the bound;
>> +
>> +@smallexample
>> +void append (char *d)
>> +@{
>> +  strncat (d, ".txt", 4);
>> +@}
>> +@end smallexample
> Sorry.  I don't follow this particular example.  Where's the truncation
> when strlen (SRC) == N for strncat?  In that case aren't we going to
> append SRC without the nul terminator, then nul terminate the result?
> Isn't that the same as appending SRC with its nul terminator?  Am I
> missing something here?

You're right that there is no truncation and the effect is
the same but only in the unlikely case when the destination
is empty.  Otherwise the result is truncated.

(Although well defined, calling strncat with an empty destination
and with the bound as big as the source is a misuse of the API.
The expected way to copy a string is to call one of the copy
functions and the warning relies on this as the basis for the
diagnostic.)

>
> Also your advice for avoiding the warning seems wrong.  Isn't it going
> to produce a huge value if strlen (d) < 4?

Right, that advice is wrong.  I had fixed this in my local tree
before posting the patch but must not have updated the patch.
Let me refresh the patch.

>>  @item -Wsizeof-array-argument
>>  @opindex Wsizeof-array-argument
>> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
>> index d94dc9c..5d021f9 100644
>> --- a/gcc/gimple-fold.c
>> +++ b/gcc/gimple-fold.c
>> @@ -1856,21 +1893,68 @@ gimple_fold_builtin_strncat (gimple_stmt_iterator *gsi)
>>        return true;
>>      }
>>
>> +      if (!nowarn && cmpsrc <= 0)
>> +	{
>> +	  /* To avoid certain truncation the specified bound should also
>> +	     not be equal to (or less than) the length of the source.  */
>> +	  location_t loc = gimple_location (stmt);
>> +	  if (warning_at (loc, OPT_Wstringop_truncation,
>> +			  cmpsrc == 0
>> +			  ? G_("%qD specified bound %E equals source length")
>> +			  : G_("%qD specified bound %E is less than source "
>> +			       "length %u"),
>> +			  gimple_call_fndecl (stmt), len, srclen))
>> +	    gimple_set_no_warning (stmt, true);
>> +	}
> So I think this corresponds to the example above I asked about.  Where's
> the truncation when the bound is equal to the source length?

Yes.  Hopefully the above answers the question.

> NIT: s/potentiall/potentially/
>

Sure. Fixed along with the other typo above.

>
>> @@ -2512,6 +2651,19 @@ strlen_optimize_stmt (gimple_stmt_iterator *gsi)
>>  	  case BUILT_IN_STPCPY_CHK_CHKP:
>>  	    handle_builtin_strcpy (DECL_FUNCTION_CODE (callee), gsi);
>>  	    break;
>> +
>> +	  case BUILT_IN_STRNCAT:
>> +	  case BUILT_IN_STRNCAT_CHK:
>> +	    check_builtin_strncat (DECL_FUNCTION_CODE (callee), gsi);
>> +	    break;
>> +
>> +	  case BUILT_IN_STPNCPY:
>> +	  case BUILT_IN_STPNCPY_CHK:
>> +	  case BUILT_IN_STRNCPY:
>> +	  case BUILT_IN_STRNCPY_CHK:
>> +	    check_builtin_stxncpy (DECL_FUNCTION_CODE (callee), gsi);
>> +	    break;
>> +
> So we've got calls to check the arguments, but not optimize here.  But
> the containing function is "strlen_optimize_stmt".
>
> Would it make sense to first call strlen_optimize_stmt to handle the
> optimization cases, then to call a new separate function to handle
> warnings for the strn* family?

tree-ssa-strlen doesn't handle strncat or strncpy (for the latter
I'm tracking the enhancement in bug 81433).  When the handling is
added I expect check_builtin_{strncat,stxncpy} will be renamed to
handle_builtin_{strncat,stxncpy} to match the existing functions,
and the optimization done there.

Or did you have something else in mind and I missed it?

Attached is an updated revision of the patch with the tweaks above.

Martin

enhance overflow and truncation detection in strncpy and strncat (PR 81117)

Commit Message

Comments

Patch