This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Tue, 2016-07-12 at 08:45 -0600, Martin Sebor wrote:
> On 07/12/2016 04:01 AM, Florian Weimer wrote:
> > On 07/12/2016 11:54 AM, Jakub Jelinek wrote:
> > > On Tue, Jul 12, 2016 at 11:51:50AM +0200, Florian Weimer wrote:
> > > > On 07/01/2016 08:15 PM, 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 tried your patch with the following code, which is close to a
> > > > real-world
> > > > example:
> > > > 
> > > > #include <stdio.h>
> > > > 
> > > > void print (const char *);
> > > > 
> > > > void
> > > > format_1 (unsigned address)
> > > > {
> > > >   unsigned char a = address >> 24;
> > > >   unsigned char b = address >> 16;
> > > >   unsigned char c = address >> 8;
> > > >   unsigned char d = address;
> > > >   char buf[15];
> > > >   sprintf ("%u.%u.%u.%u", buf, a, b, c, d);
> > > 
> > > Are you sure this is real-world code?  sprintf's first argument
> > > is the
> > > buffer and second the format string, so if this doesn't warn at
> > > compile
> > > time, it will surely crash at runtime when trying to store into
> > > .rodata.
> > 
> > Argh!  You are right, I swapped the arguments.
> > 
> > And further attempts showed that I was missing -D_FORTIFY_SOURCE=2.
> > With
> > it, I get a nice diagnostic.  Wow!

Does it warn for the code that Florian actually posted?  I tried it
with a recent (unpatched) build of trunk and got no warning (with -O2 
-Wall -D_FORTIFY_SOURCE=2), but it strikes me that we ought to warn if
someone passes about the above code (for the uninitialized format
string, at least; I don't know if it's legal to pass a string literal
as the destination).

Should I file a PR for this?

> Thanks for giving it a try!  Based on the feedback I received
> I've since updated the patch and will post the latest version
> for review soon.  In simple cases like this one it warns even
> without _FORTIFY_SOURCE or optimization (though without the
> latter it doesn't benefit from VRP information).  Let me see
> about adding a warning to detect and warn when the arguments
> are transposed.
> 
> $ /build/gcc-49905/gcc/xgcc -B /build/gcc-49905/gcc -S -Wall -Wextra 
> -Wpedantic -Wformat-length=2 xyz.c
> 
> xyz.c: In function ‘format_1’:
> xyz.c:13:29: warning: may write a terminating nul past the end of the
> destination [-Wformat-length=]
>     sprintf (buf, "%u.%u.%u.%u", a, b, c, d);
>                               ^
> xyz.c:13:3: note: destination size is ‘15’ bytes, output size between
> ‘8’ and ‘16’
>     sprintf (buf, "%u.%u.%u.%u", a, b, c, d);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Style question: should the numbers in these diagnostic messages be in
quotes?  That looks a bit strange to me.  It seems clearer to me to
have:

xyz.c:13:3: note: destination size is 15 bytes, output size between 8
and 16

> xyz.c: In function ‘format_2’:
> xyz.c:21:3: warning: ‘%u’ directive writing between ‘1’ and ‘10’
> bytes 
> into a region of size ‘4’ [-Wformat-length=]
>     sprintf (buf, "%u.%u.%u.%u",
>     ^
> xyz.c:21:3: note: using the range [‘1u’, ‘2147483648u’] for directive
> argument
> xyz.c:21:3: note: destination size is ‘15’ bytes, output size between
> ‘4’ and ‘22’
>     sprintf (buf, "%u.%u.%u.%u",
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>              address >> 24,
>              ~~~~~~~~~~~~~~
>              (address >> 16) & 0xff,
>              ~~~~~~~~~~~~~~~~~~~~~~~
>              (address >> 8) & 0xff,
>              ~~~~~~~~~~~~~~~~~~~~~~
>              address & 0xff);
>              ~~~~~~~~~~~~~~~
> xyz.c:21:3: note: destination size is ‘15’ bytes, output size between
> ‘6’ and ‘33’
> 
> Martin


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]