Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law <law at redhat dot com>]

On 09/11/2016 08:03 PM, Martin Sebor wrote:

> Attached is revision 8 of the patch (hopefully) adjusted as
> requested above, and with a simple test with of the multiline
> diagnostic directives suggested by David.  This revision also
> enables the -fprintf-return-value option by default.  The libgomp
> test failures I was seeing in my earlier testing must have been
> caused by an older version of GMP or MPFR that I had inadvertently
> use (normally I use in-tree versions downloaded and expanded there
> by the download_prerequisites script but that time I forgot that
> step).
Ah, my question answered.  I started looking at the multiline stuff and 
didn't read the last sentence WRT libgomp.  Sorry for the noise in my 
prior message.


> David, in the way of feedback, I spent hours debugging the simple
> multiline test I added.  It seems that DejaGnu is exquisitely
> sensitive to whitespaces in the multiline output.  I appreciate
> that spacing is essential to lining up the caret and the tildes
> with the text of the program but tests fail even when all the
> expected output is lined up right in the multiline directive but
> off by a space (or tab) with respect to the actual output.  That
> DejaGnu output doesn't make this very clear at all makes debugging
> these types of issues a pain.  I wonder if there's a better to do
> this.
dejagnu as a whole is a pain and trying to get the right whitespace, 
regexps, escaping is an exercise in pure torture.  How you and David 
have managed to do any multiline tests is amazing.


> Thanks
> Martin
>
> gcc-49905.diff
>
>
> PR middle-end/49905 - Better sanity checking on sprintf src & dest to
> 	produce warning for dodgy code
>
> gcc/ChangeLog:
> 	PR middle-end/49905
> 	* Makefile.in (OBJS): Add gimple-ssa-sprintf.o.
> 	* config/linux.h (TARGET_PRINTF_POINTER_FORMAT): Redefine.
> 	* config/linux.c (gnu_libc_printf_pointer_format): New function.
> 	* config/sol2.h (TARGET_PRINTF_POINTER_FORMAT): Same.
> 	* config/sol2.c (solaris_printf_pointer_format): New function.
> 	* doc/invoke.texi (-Wformat-length, -fprintf-return-value): New
> 	options.
> 	* doc/tm.texi.in (TARGET_PRINTF_POINTER_FORMAT): Document.
> 	* doc/tm.texi: Regenerate.
> 	* gimple-fold.h (get_range_strlen): New function.
> 	(get_maxval_strlen): Declare existing function.
> 	* gimple-fold.c (get_range_strlen): Add arguments and compute both
> 	maximum and minimum.
> 	 (get_range_strlen): Define overload.
> 	(get_maxval_strlen): Adjust.
> 	* gimple-ssa-sprintf.c: New file and pass.
> 	* passes.def (pass_sprintf_length): Add new pass.
> 	* targhooks.h (default_printf_pointer_format): Declare new function.
> 	(gnu_libc_printf_pointer_format): Same.
> 	(solaris_libc_printf_pointer_format): Same.
> 	* targhooks.c (default_printf_pointer_format): Define new function.
> 	* tree-pass.h (make_pass_sprintf_length): Declare new function.
> 	* print-tree.c: Increase buffer size.
>
> gcc/c-family/ChangeLog:
> 	PR middle-end/49905
> 	* c.opt: Add -Wformat-length and -fprintf-return-value.
>
> gcc/testsuite/ChangeLog:
> 	PR middle-end/49905
> 	* gcc.dg/builtin-stringop-chk-1.c: Adjust.
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: New test.
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-2.c: New test.
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: New test.
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-4.c: New test.
> 	* gcc.dg/tree-ssa/builtin-sprintf.c: New test.
> 	* gcc.dg/tree-ssa/builtin-sprintf-2.c: New test.

>  static bool
> -get_maxval_strlen (tree arg, tree *length, bitmap *visited, int type)
> +get_range_strlen (tree arg, tree length[2], bitmap *visited, int type,
> +		  bool fuzzy)
>  {
>    tree var, val;
>    gimple *def_stmt;
>
> +  /* The minimum and maximum length.  The MAXLEN pointer stays unchanged
> +     but MINLEN may be cleared during the execution of the function.  */
> +  tree *minlen = length;
> +  tree* const maxlen = length + 1;
Check formatting here.  I'm not sure if the formatting standards 
explicitly state how to handle the "*" when there's a qualifier between 
the type and the object.  But please check.

> +
> +	  if (warned && fmtres.argmin)
> +	    {
> +	      if (fmtres.argmin == fmtres.argmax)
> +		inform (info.fmtloc, "directive argument %qE", fmtres.argmin);
> +	      else if (fmtres.bounded)
> +		inform (info.fmtloc, "directive argument in the range [%E, %E]",
> +			fmtres.argmin, fmtres.argmax);
> +	      else
> +		inform (info.fmtloc,
> +			"using the range [%qE, %qE] for directive argument",
> +			fmtres.argmin, fmtres.argmax);
Don't you need G_ for these messages?

Can you please check the calls to fmtwarn which pass in the string as an 
argument and add G_ as needed?  I think the cases where the string is 
computed into a variable are all handled correctly, it's jut those where 
the string appears as a call argument that need double checking.  I 
think there's ~3 of them.


> +
> +/* Account for the number of bytes between BEG and END (or between
> +   BEG + strlen (BEG) when END is null) in the format string in a call
> +   to a formatted output function described by INFO.  Reflect the count
> +   in RES and issue warnings as appropriate.  */
> +
> +static void
> +add_bytes (const pass_sprintf_length::call_info &info,
> +	   const char *beg, const char *end, format_result *res)
> +{
> +      size_t len = strlen (info.fmtstr + off);
> +
> +#if 0
> +      int caret = off - !len;
> +      int beg = len ? off : 0;
> +      int end = off + len - !!len;
> +      int chr = info.fmtstr[off];
> +
> +      if (const char *env = getenv ("LOC_CARET"))
> +	caret = atoi (env);
> +      if (const char *env = getenv ("LOC_BEGIN"))
> +	beg = atoi (env);
> +      if (const char *env = getenv ("LOC_END"))
> +	end = atoi (env);
> +      if (const char *env = getenv ("CHAR"))
> +	chr = atoi (env);
> +
> +      substring_loc loc
> +	(info.fmtloc, TREE_TYPE (info.format), caret, beg, end);
> +#else
> +      substring_loc loc
> +	(info.fmtloc, TREE_TYPE (info.format), off - !len, len ? off : 0,
> +	 off + len - !!len);
> +#endif
Please remove the #if 0'd code.




> +
> +  /* Avoid the return value optimization when the behavior of the call
> +     is undefined either because any directive may have produced 4K or
> +     more of output, or the return value exceeds INT_MAX, or because
> +     the ourput overflows the destination object (but leave it enabled
s/ourput/output/


I think with the nits above fixed, this is OK for the trunk.

Thanks for your patience.

Jeff