Re: [PATCH] assume sprintf formatting of wide characters may fail (PR 86853)

[Jeff Law <law at redhat dot com>]

On 08/04/2018 12:46 PM, Martin Sebor wrote:
> The sprintf handling of wide characters neglects to consider
> that calling the function may fail due to a conversion error
> (when the wide character is invalid or not representable in
> the current locale).  The handling also misinterprets
> the POSIX %S wide string directive as a plain narrow %s and
> doesn't include %C (the POSIX equivalent of %lc).  The attached
> patch corrects these oversights by extending the data structures
> to indicate when a directive may fail, and extending the UNDER4K
> member of the format_result structure to also encode calls with
> such directives.
> 
> Tested on x86_64-linux.
> 
> Besides the trunk, since this bug can affect code correctness
> I'd like to backport this patch to both release branches (7
> and 8).
> 
> Martin
> 
> gcc-86853.diff
> 
> 
> PR tree-optimization/86853 - sprintf optimization for wide strings doesn't account for conversion failure
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/86853
> 	* gimple-ssa-sprintf.c (struct format_result): Rename member.
> 	(struct fmtresult): Add member and initialize it in ctors.
> 	(format_character): Handle %C.  Extend range to NUL.  Set MAYFAIL.
> 	(format_string): Handle %S the same as %ls.  Set MAYFAIL.
> 	(format_directive): Set POSUNDER4K when MAYFAIL is set.
> 	(parse_directive): Handle %C same as %c.
> 	(sprintf_dom_walker::compute_format_length): Adjust.
> 	(is_call_safe): Adjust.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/86853
> 	* gcc.dg/tree-ssa/builtin-sprintf-10.c: New test.
> 	* gcc.dg/tree-ssa/builtin-sprintf-11.c: New test.
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-18.c: Adjust.
Mostly OK.  One nit noted below and one question.  If you're
sufficiently confident that the charmap test is OK, then you just need
to address the nit in the comment and you're good to go.

> 
> Index: gcc/gimple-ssa-sprintf.c
> ===================================================================
> --- gcc/gimple-ssa-sprintf.c	(revision 263295)
> +++ gcc/gimple-ssa-sprintf.c	(working copy)
> @@ -2217,13 +2224,20 @@ format_character (const directive &dir, tree arg,
>  	      res.range.likely = 0;
>  	      res.range.unlikely = 0;
>  	    }
> -	  else if (min > 0 && min < 128)
> +	  else if (min >= 0 && min < 128)
>  	    {
> +	      /* Be conservative if the target execution character set
> +		 is not a 1-to-1 mapping to the source character set or
> +		 if the source set is not ASCII.  */
> +	      bool one_2_one_ascii
> +		= (target_to_host_charmap[0] == 1 && target_to_host ('a') == 97);
Hmm.  Is this really sufficient?    I have nowhere near enough knowledge
of the potential target character sets to know if this is sufficiently
tight.

> @@ -2302,6 +2320,10 @@ format_string (const directive &dir, tree arg, vr_
>  	  /* Even a non-empty wide character string need not convert into
>  	     any bytes.  */
>  	  res.range.min = 0;
> +
> +	  /* A non-emtpy wide character conversion may fail.  */
s/emtpy/empty/

Jeff