Re: [PATCH] enhance -Wrestrict for sprintf %s arguments

[Jeff Law <law at redhat dot com>]

On 07/02/2017 02:00 PM, Martin Sebor wrote:
> The attached patch enhances the -Wrestrict warning to detect more
> than just trivial instances of overlapping copying by sprintf and
> related functions.
> 
> The meat of the patch is relatively simple but because it introduces
> dependencies between existing classes in the sprintf pass I had to
> move the class definitions around.  That makes the changes look more
> extensive than they really are.
> 
> The enhancement works by first determining the base object (or
> pointer) from the destination of the sprintf call, the constant
> offset into the object, and its size.  For each %s argument, it
> then computes same information.  If it determines that overlap
> between the two is possible it stores the information for the
> directive argument (including the size of the argument) for later
> processing.  After the whole call/format string has been processed,
> the patch then iterates over the stored directives and their
> arguments and compares the size/length of the argument against
> the function's overall output.  If they overlap it issues
> a warning.
> 
> Tested on x86_64-linux.
> 
> -Wrestrict is not currently included in either -Wextra or -Wall
> and this patch doesn't change it even though there have been
> requests to add it to one of these two options.  I'd like to do
> that as a separate step.
Yea, I think separate step is wise.

> 
> As the next step I'd also like to extend a limited form of the
> -Wrestrict enhancement to other restrict-qualified built-ins (like
> strcpy), and ultimately also to user-defined functions that make
> use of restrict.  I think this might perhaps best be done in
> a separate pass where the computed pointer information can be
> cached to avoid recomputing it for each call, but I don't expect
> to be able to have the new pass (or whatever form the enhancement
> might end up taking) to also handle sprintf (at least not with
> the same accuracy it does now) because the sprintf data for each
> format directive are not available outside the sprintf pass.
Seems reasonable.  Actual implementation will tell us for sure :-)


> 
> Martin
> 
> gcc-35503.diff
> 
> 
> PR tree-optimization/35503 - Warning about restricted pointers?
> 
> gcc/c-family/ChangeLog:
> 
> 	PR tree-optimization/35503
> 	* gcc/c-family/c-common.c (check_function_restrict): Avoid diagnosing
> 	sprintf et al. unless both -Wformat-overflow and -Wformat-truncation
> 	are disabled.
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/35503
> 	* gimple-ssa-sprintf.c (format_result::alias_info): New struct.
> 	(directive::argno): New member.
> 	(format_result::aliases, format_result::alias_count): New data members.
> 	(format_result::append_alias): New member function.
> 	(fmtresult::dst_offset): New data member.
> 	(pass_sprintf_length::call_info::dst_origin): New data member.
> 	(pass_sprintf_length::call_info::dst_field, dst_offset): Same.
> 	(char_type_p, array_elt_at_offset, field_at_offset): New functions.
> 	(get_origin_and_offset): Same.
> 	(format_string): Call it.
> 	(format_directive): Call append_alias and set directive argument
> 	number.
> 	(pass_sprintf_length::compute_format_length): Diagnose arguments
> 	that overlap the destination buffer.
> 	(pass_sprintf_length::handle_gimple_call): Initialize new members.
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/35503
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-19.c: New test.
I'm OK with the general concept of enhancing the warning.  The big
question I have is whether or not we'd be better off using the alias
oracle here rather than what appears to be rolling our own data
structures and analysis routines to describe memory objects and their
potential alias relationship.

See tree-ssa-alias.h.  In particular you're looking for ao_ref.  You may
also be intersted in the points-to solutions.  Would using that
infrastructure make sense?

jeff