| Message ID | 8279f735-1d61-8eec-ba9b-c477481c2a54@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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? Jeff
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
On 12/05/2016 08:56 PM, Martin Sebor wrote: > 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. OK. > > 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. I think I'd rather just stop at this point. I don't think we're gaining much, if anything and encoding the glibc-ism in here seems like a mistake. Jeff
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. */ + res.range.min = 0; + res.range.max = 6; + res.nullp = true; + return res; + } else { /* For a '%s' and '%ls' directive with a non-constant string, @@ -1765,6 +1782,18 @@ format_directive (const pass_sprintf_length::call_info &info, } } + bool warned = false; + + if (fmtres.nullp) + { + warned = fmtwarn (dirloc, pargrange, NULL, + OPT_Wformat_length_, + "%<%.*s%> directive argument is null", + (int)cvtlen, cvtbeg); + + /* Proceed below. */ + } + /* Compute the number of available bytes in the destination. There must always be at least one byte of space for the terminating NUL that's appended after the format string has been processed. */ @@ -1775,8 +1804,6 @@ format_directive (const pass_sprintf_length::call_info &info, /* The result is a range (i.e., it's inexact). */ if (!res->warned) { - bool warned = false; - if (navail < fmtres.range.min) { /* The minimum directive output is longer than there is @@ -2726,6 +2753,9 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi) return; } + /* The first argument is a pointer to the destination. */ + tree dstptr = gimple_call_arg (info.callstmt, 0); + info.format = gimple_call_arg (info.callstmt, idx_format); if (idx_dstsize == HOST_WIDE_INT_M1U) @@ -2733,7 +2763,7 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi) /* For non-bounded functions like sprintf, determine the size of the destination from the object or pointer passed to it as the first argument. */ - dstsize = get_destination_size (gimple_call_arg (info.callstmt, 0)); + dstsize = get_destination_size (dstptr); } else if (tree size = gimple_call_arg (info.callstmt, idx_dstsize)) { @@ -2795,6 +2825,20 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi) } else { + /* For calls to non-bounded functions or to those of bounded + functions with a non-zero size, warn if the destination + pointer is null. */ + if (integer_zerop (dstptr)) + { + /* This is diagnosed with -Wformat only when the null is a constant + pointer. The warning here diagnoses instances where the pointer + is not constant. */ + location_t loc = gimple_location (info.callstmt); + warning_at (EXPR_LOC_OR_LOC (dstptr, loc), + OPT_Wformat_length_, "null destination pointer"); + return; + } + /* Set the object size to the smaller of the two arguments of both have been specified and they're not equal. */ info.objsize = dstsize < objsize ? dstsize : objsize; @@ -2813,7 +2857,8 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi) /* This is diagnosed with -Wformat only when the null is a constant pointer. The warning here diagnoses instances where the pointer is not constant. */ - warning_at (EXPR_LOC_OR_LOC (info.format, input_location), + location_t loc = gimple_location (info.callstmt); + warning_at (EXPR_LOC_OR_LOC (info.format, loc), OPT_Wformat_length_, "null format string"); return; } diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-7.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-7.c new file mode 100644 index 0000000..ee4b04e --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-7.c @@ -0,0 +1,94 @@ +/* PR middle-end/78519 - missing warning for sprintf %s with null pointer + Also exercises null destination pointer and null format string. + { dg-do compile } + { dg-options "-O1 -Wformat -Wformat-length -ftrack-macro-expansion=0" } */ + +typedef __builtin_va_list va_list; + +#define sprintf __builtin_sprintf +#define snprintf __builtin_snprintf +#define vsprintf __builtin_vsprintf +#define vsnprintf __builtin_vsnprintf + + +static char* null (void) +{ + return 0; +} + + +void sink (int); +#define T sink + + +/* Verify that calls with a null destination pointer are diagnosed. */ + +void test_null_dest (va_list va) +{ + char *p = null (); + T (sprintf (null (), "%i", 0)); /* { dg-warning "null destination pointer" } */ + T (sprintf (p, "%i", 0)); /* { dg-warning "null destination pointer" } */ + T (sprintf (p, "%i abc", 0)); /* { dg-warning "null destination pointer" } */ + + T (snprintf (null (), 1, "%i", 0)); /* { dg-warning "null destination pointer" } */ + T (snprintf (p, 2, "%i", 0)); /* { dg-warning "null destination pointer" } */ + T (snprintf (p, 3, "%i abc", 0)); /* { dg-warning "null destination pointer" } */ + + /* Snprintf with a null pointer and a zero size is a special request + to determine the size of output without writing any. Such calls + are valid must not be diagnosed. */ + T (snprintf (p, 0, "%i", 0)); + + T (vsprintf (null (), "%i", va)); /* { dg-warning "null destination pointer" } */ + T (vsprintf (p, "%i", va)); /* { dg-warning "null destination pointer" } */ + + T (vsnprintf (null (), 1, "%i", va)); /* { dg-warning "null destination pointer" } */ + T (vsnprintf (p, 2, "%i", va)); /* { dg-warning "null destination pointer" } */ + T (vsnprintf (p, 0, "%i", va)); +} + +void test_null_format (char *d, va_list va) +{ + char *fmt = null (); + + T (sprintf (d, null ())); /* { dg-warning "null format string" } */ + T (sprintf (d, fmt)); /* { dg-warning "null format string" } */ + + T (snprintf (d, 0, null ())); /* { dg-warning "null format string" } */ + T (snprintf (d, 1, fmt)); /* { dg-warning "null format string" } */ + + T (vsprintf (d, null (), va)); /* { dg-warning "null format string" } */ + T (vsprintf (d, fmt, va)); /* { dg-warning "null format string" } */ + + T (vsnprintf (d, 0, null (), va)); /* { dg-warning "null format string" } */ + T (vsnprintf (d, 1, fmt, va)); /* { dg-warning "null format string" } */ +} + +void test_null_arg (char *d, const char *s) +{ + char *p = null (); + + T (sprintf (d, "%-s", null ())); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%-s", p)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%s %s", p, s)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%s %s", s, p)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%s %i", p, 1)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%i %s", 1, p)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%.0s", p)); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%1.0s", p)); /* { dg-warning "directive argument is null" } */ + + T (snprintf (d, 0, "%-s", null ())); /* { dg-warning "directive argument is null" } */ + T (snprintf (d, 1, "%-s", p)); /* { dg-warning "directive argument is null" } */ + /* { dg-warning "output may be truncated writing between 0 and 6 bytes" "(null)" { target *-*-* } .-1 } */ + + T (sprintf (d, "%i %s", 1, null ())); /* { dg-warning "directive argument is null" } */ + T (sprintf (d, "%i %s", 2, p)); /* { dg-warning "directive argument is null" } */ + + T (snprintf (d, 0, "%i %s", 1, null ())); /* { dg-warning "directive argument is null" } */ + T (snprintf (d, 9, "%i %s", 2, p)); /* { dg-warning "directive argument is null" } */ + + /* A sanity check that the %p directive doesn't emit a warning + with a null pointer. */ + T (sprintf (d, "%p", null ())); + T (sprintf (d, "%p", p)); +}
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