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

[Richard Biener <rguenther at suse dot de>]

On Mon, 4 Jul 2016, Martin Sebor wrote:

> On 07/04/2016 04:59 AM, Richard Biener wrote:
> > 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?
> 
> No, it doesn't.  What I meant is that the same code, when added
> in builtins.c instead, could readily be extended to fold into
> strings expressions like
> 
>   sprintf (buf, "%i", 123);
> 
> > 
> > >     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.
> 
> Thanks for the clarification.  IIUC, there are at least three
> possibilities for how to proceed: leave it as is (no checking
> with LTO), link LTO with C_COMMON_OBJS, or move the c-format.c
> code into the middle end.  Do you have a preference for one of
> these?  Or is there another solution that I missed?

Another solution is to implement this somewhen before LTO
bytecode is streamed out thus at the end of early optimizations.

I'm not sure linking with C_COMMON_OBJS does even work (you can try).
Likewise c-format.c may be too entangled with the FEs (maybe just
linking with c-format.o is enough?).

Richard.

> 
> FWIW, I would expect a good number of other warnings to benefit
> from optimization and having a general solution for this problem
> to be helpful.  I also suspect this isn't the first time this
> issue has come up.  I'm wondering what solutions have already
> been considered and with what pros and cons (naively, I would
> think that factoring the relevant code out of cc1 into a shared
> library that lto1 could load should work).
> 
> Martin
> 
> 

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