On 12/05/2016 11:21 AM, Jeff Law wrote:
On 12/04/2016 04:55 PM, Martin Sebor wrote:
Bug 78519 points out that while the -Wformat warning flags a small
subset of sprintf calls with a null pointer argument to a %s directive
(those where the pointer is a constant) it misses the much bigger
set where the pointer is not a constant but instead is determined
to be null as a result of optimization. This is because -Wformat
runs too early, before any of the optimization passes that make it
possible to detect that non-constant pointers are null.
With the -Wformat-length warning running much later than -Wformat,
it's trivial to detect and diagnose these types of bugs with it.
The attached patch adds this warning, along with the ability to
detect a null destination pointer when it's required to be non-null
(this is in all of the {v,}sprintf functions and in {v,}snprintf
when the size argument is not zero).
Ultimately, the destination pointer argument (but not the format
string) to the {v,}sprintf functions needs to be declared nonnull
(pursuant to bug 78673) and the null-checking moved elsewhere.
I'm testing a follow-on patch that does just that but I post this
fix in the meantime since its main focus is the null %s argument.
Martin
gcc-78519.diff
PR middle-end/78519 - missing warning for sprintf %s with null pointer
gcc/ChangeLog:
PR middle-end/78519
* gimple-ssa-sprintf.c (format_string): Handle null pointers.
(format_directive): Diagnose null pointer arguments.
(pass_sprintf_length::handle_gimple_call): Diagnose null destination
pointers. Correct location of null format string in diagnostics.
gcc/testsuite/ChangeLog:
PR middle-end/78519
* gcc.dg/tree-ssa/builtin-sprintf-warn-7.c: New test.
diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index e86c4dc..7004f09 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -433,7 +433,7 @@ struct result_range
struct fmtresult
{
fmtresult ()
- : argmin (), argmax (), knownrange (), bounded (), constant ()
+ : argmin (), argmax (), knownrange (), bounded (), constant (),
nullp ()
{
range.min = range.max = HOST_WIDE_INT_MAX;
}
@@ -461,6 +461,9 @@ struct fmtresult
are also constant (such as determined by constant propagation,
though not value range propagation). */
bool constant;
+
+ /* True when the argument is a null pointer. */
+ bool nullp;
};
/* Description of a conversion specification. */
@@ -1624,6 +1627,20 @@ format_string (const conversion_spec &spec,
tree arg)
res.range.min = 0;
}
}
+ else if (arg && integer_zerop (arg))
+ {
+ /* Handle null pointer argument. */
+
+ fmtresult res;
+ /* Set the range based on Glibc "(null)" output but leave
+ all other members at default to indicate that the range
+ isn't trustworthy. This allows the rest of the format
+ string to be checked for problems. */
By not trustworthy, I guess you mean it's only used to issue "may be"
style warnings, right? What benefit do you gain by encoding the
glib-ism vs using HOST_WIDE_INT_MAX? Presumably once you use
HOST_WIDE_INT_MAX nothing else is going to be checked?
By not trustworthy I mean that the result can't be used for any
sort of optimization, including setting the range on the return
value. That's the case whenever the pass finds anything wrong,
including unspecified or implementation-defined behavior.
I suppose setting a range seemed better than giving up. Then again,
since with this patch GCC will warn on null %s pointers there may
not be much point in trying to see if there's also some other
problem after that, except perhaps in code that deliberately relies
on the Glibc feature. I'd be fine with just stopping at this point
if you prefer.
Martin
