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

[Richard Biener <rguenther at suse dot de>]

On Fri, 1 Jul 2016, Martin Sebor wrote:

> The attached patch enhances compile-time checking for buffer overflow
> and output truncation in non-trivial calls to the sprintf family of
> functions under a new option -Wformat-length=[12].  This initial
> patch handles printf directives with string, integer, and simple
> floating arguments but eventually I'd like to extend it all other
> functions and directives for which it makes sense.
> 
> I made some choices in the implementation that resulted in trade-offs
> in the quality of the diagnostics.  I would be grateful for comments
> and suggestions how to improve them.  Besides the list I include
> Jakub who already gave me some feedback (thanks), Joseph who as
> I understand has deep knowledge of the c-format.c code, and Richard
> for his input on the LTO concern below.
> 
> 1) Making use of -Wformat machinery in c-family/c-format.c.  This
>    seemed preferable to duplicating some of the same code elsewhere
>    (I initially started implementing it in expand_builtin in
>    builtins.c).  It makes the implementation readily extensible
>    to all the same formats as those already handled for -Wformat.
>    One drawback is that unlike in expand_builtin, calls to these
>    functions cannot readily be folded.  Another drawback pointed

folded?  You mean this -W option changes code generation?

>    out by Jakub is that since the code is only available in the
>    C and C++ compilers, it apparently may not be available with
>    an LTO compiler (I don't completely understand this problem
>    but I mention it in the interest of full disclosure). In light
>    of the dependency in (2) below, I don't see a way to avoid it
>    (moving c-format.c to the middle end was suggested but seemed
>    like too much of a change to me).

Yes, lto1 is not linked with C_COMMON_OBJS (that could be changed
of course at the expense of dragging in some dead code).  Moving
all the format stuff to the middle-end (or separated better so
the overhead in lto1 is lower) would be possible as well.

That said, a langhook as you add it highlights the issue with LTO.

Richard.

> 2) Optimization.
>    In keeping with the other -Wformat options, the checking is
>    enabled without optimization.  Especially at level 2, the
>    warnings can be useful even without it.  But to make buffer
>    sizes and non-constant argument values available in calls to
>    functions like sprintf (via __builtin_object_size) better
>    results are obtained with optimization.
> 
> 3) Truncation warnings.
>    Although calls to bounded functions like snprintf aren't subject
>    to buffer overflow, they can be subject to accidental truncation
>    when the destination buffer isn't sized appropriately.  With the
>    patch, such calls are diagnosed under the same option, but I
>    wonder if have a separate warning option for them might be
>    preferable (e.g., -Wformat-trunc=[01] or something like that).
>    Independently, it might be useful to differentiate between
>    truncating calls that check the return value and those that
>    don't.
> 
> Besides the usual testing I compiled several packages with the
> warning.  If found a few bugs in boundary cases in Binutils that
> are being fixed.
> 
> Thanks
> Martin
> 
> PS There are a few FIXME notes in the patch that I will either
> fix or remove, depending on feedback, before committing the
> patch.
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)