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

[Martin Sebor <msebor at gmail dot com>]

On 09/08/2016 01:45 PM, David Malcolm wrote:
> On Thu, 2016-09-08 at 13:03 -0600, Martin Sebor wrote:
> > Attached is another update to the patch to address the last round
> > of comments and suggestions, most notably to:
> >
> >    *  implement the checking of the implementation limit of 4,095 on
> >       the output of a single directive to allow for the Glibc failure
> >       due to ENOMEM (the patch issues a warning and disables the
> >       optimization when this happens)
> >    *  implement checking for exceeding INT_MAX bytes (warn and disable
> >       optimization)
> >    *  call set_range_info when the return value optimization is not
> >       possible
> >    *  remove code to work around tree-optimization/71831 (now on
> >       trunk)
> >
> > The -fprintf-return value optimization is still disabled.  GCC
> > successfully bootstraps with it and most tests pass but there's
> > a failure in the Fortran libgomp tests that I am yet to figure
> > out.
> >
> > I'm hoping to get the patch reviewed and hopefully approved while
> > I debug the libgomp failure.
> >
> > Martin
>
> I see that you also integrated the substring_loc and format_warning API
> into this revision - thanks.

Yes, I forgot to mention it among the highlights.  Thanks for making
the API available to the middle-end!

> The patch has a lot of macro-based testcases, presumably for exercising
> all of the format codes and boundary conditions, but it seems to be
> lacking what I'd call a "usability test case" - a test case that shows
> a representative example of idiomatic but buggy code, along with the
> full output of the warning, with carets and underlining, so that we can
> easily see what the user experience is.  (sorry if there is one and I
> didn't see it).

There is a simple test (in the test_sprintf_note() function in
builtin-sprintf-warn-1.c) that exercises the notes but there's always
room for more and more robust test cases :)  I'll see about adding
a few.

FWIW, the challenge here is not in adding them but rather in knowing
when to stop and what level of detail to go to.  With too many test
cases (exercising this level of detail) it can get very tedious and
time consuming to then change the diagnostics or add more detail.
This is not an excuse for not having enough tests.  But striking
the right balance between the level of detail in them is something
I had to grapple with on this project.

>  From a marketing point-of-view, I think any new diagnostics like this
> deserve a screenshot on the website's gcc-7/changes.html page, showing
> a simple example of the above that makes a casual reader think "gcc 7
> looks neat; I've definitely made that mistake; I wonder if that's going
> to find bugs in my code; I'd better try it at some point".
>
> So please can you add a test case that demonstrates such a screenshot
> -worthy example, using:
>
>    /* { dg-options "-fdiagnostics-show-caret" } */
>
> and you can use:
>
>    /* { dg-begin-multiline-output "" }
> copy&paste the source, underlines and carets here, omitting trailing dg
> directives.
>       { dg-end-multiline-output "" } */
>
> (we'd strip away all the dg- directives when making the screenshots for
> the website).
>
> The act of creating such an example sometimes suggests tweaks e.g. to
> the exact wording of the warning.
>
> Sorry if this seems like I'm picking on you Martin; I just wanted to
> share some thoughts that I'm trying to crystallize into general
> guidelines on writing diagnostics.

Not at all!  Thanks for the review and for the suggestion to make
use of the multiline DejaGnu directives.  I'll add more tests in
the next revision of the patch (as I have been in each iteration).

Martin