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
