[Bug c/54582] New: gap in FORTIFY checking of buffer lengths

[Bug c/54582] New: gap in FORTIFY checking of buffer lengths

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

             Bug #: 54582
           Summary: gap in FORTIFY checking of buffer lengths
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: minor
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: dcb314@hotmail.com


Consider the following code

# include <stdio.h>

void f(int n)
{
    char buf[2];

    sprintf(buf, "ab%d", n);
    printf("%s\n", buf);

    sprintf(buf, "ab");
    printf("%s\n", buf);
}

int
main()
{
    f(2);

    return 0;
}

compiled as

[dcb@zippy Alphasrc]$ ~/gcc/trunk/results/bin/gcc -g -O2 -Wall
-D_FORTIFY_SOURCE=2 -c sep14a.c
In file included from /usr/include/stdio.h:936:0,
                 from sep14a.c:2:
In function ‘sprintf’,
    inlined from ‘f’ at sep14a.c:11:9:
/usr/include/bits/stdio2.h:34:3: warning: call to __builtin___sprintf_chk will
always overflow destination buffer [enabled by default]
   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
   ^

so gcc can find the problem in the 2nd sprintf, but not the first.

All the numeric specifiers (%d, %u etc) all produce at least one 
character, so gcc could take account of this in checking buffer lengths.

Here is cppcheck finding the problem

Checking sep14a.c...
[sep14a.c:8]: (error) Buffer is accessed out of bounds.
[sep14a.c:11]: (error) Buffer is accessed out of bounds.

[Bug c/54582] gap in FORTIFY checking of buffer lengths

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-09-17
     Ever Confirmed|0                           |1

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-17 08:51:29 UTC ---
Confirmed.

[Bug c/54582] gap in FORTIFY checking of buffer lengths

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

--- Comment #2 from David Binderman <dcb314 at hotmail dot com> 2013-02-06 10:58:22 UTC ---
I went through the source code of Fedora Linux rawhide
and there are about 220 occurrences of this problem.

So it appears far more frequently than I had suspected.

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c                           |middle-end
           Severity|minor                       |enhancement

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-06 12:18:21 UTC ---
Code is (maybe_emit_sprintf_chk_warning):

  /* If the format doesn't contain % args or %%, we know its size.  */
  if (strchr (fmt_str, target_percent) == 0)
    len = build_int_cstu (size_type_node, strlen (fmt_str));
  /* If the format is "%s" and first ... argument is a string literal,
     we know it too.  */
  else if (fcode == BUILT_IN_SPRINTF_CHK
           && strcmp (fmt_str, target_percent_s) == 0)
    ...
  else
    return;

so it lacks a way to compute an upper bound for the format which I guess
we can always compute (just not account all %'s at all?).

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> 2013-02-06 12:21:12 UTC ---
(In reply to comment #3)
> Code is (maybe_emit_sprintf_chk_warning):
> 
>   /* If the format doesn't contain % args or %%, we know its size.  */
>   if (strchr (fmt_str, target_percent) == 0)
>     len = build_int_cstu (size_type_node, strlen (fmt_str));
>   /* If the format is "%s" and first ... argument is a string literal,
>      we know it too.  */
>   else if (fcode == BUILT_IN_SPRINTF_CHK
>            && strcmp (fmt_str, target_percent_s) == 0)
>     ...
>   else
>     return;
> 
> so it lacks a way to compute an upper bound for the format which I guess
> we can always compute (just not account all %'s at all?).

lower bound of course

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-06 12:41:09 UTC ---
It isn't that easy.  For %'s you really have to parse all the characters after
% and figure out where the format specifier ends.  Users can have printf hooks
installed, so it certainly needs to give up any time it sees something it
doesn't fully understand.  In that case I guess it could safely just assume the
lower bound as if the string ended on the % after which it doesn't understand
the letters.  Note, that this is just about the compile time warning, the code
will fail at runtime the same way in the first as in the second case.

So, if we are going to do something about this, either we could do something
very simple, like strchr (str, '%') - str as low bound guess, or reuse the
c-format tables somehow (but they are in the FE, while this is in middle-end),
or write a simple parse of few most common formatting specifiers and give up on
anything else.

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |manu at gcc dot gnu.org

--- Comment #6 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2013-02-06 13:13:52 UTC ---
(In reply to comment #5)
> So, if we are going to do something about this, either we could do something
> very simple, like strchr (str, '%') - str as low bound guess, or reuse the
> c-format tables somehow (but they are in the FE, while this is in middle-end),

And why is this check in the middle-end?

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-06 13:25:29 UTC ---
Because object sizes are finalized only during the objsz pass, after lots of
optimization passes.  Note, as I said earlier, what matters most is that the
check is performed at runtime in that case and thus the source code bug can't
be exploited.  The warning is just to let the user know earlier than at
runtime, when easily possible.

-D_FORTIFY_SOURCE{,=2} is done using inline functions, so the FE pretty much
never knows the object size, you need inlining and various propagations (plus
for many cases also the objsz pass that propagates the object size properties
through the IL).  In the FE you could do it only if all the fortification
functions were preprocessor macros, and handle only the most simple cases.

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

--- Comment #8 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2013-02-06 13:39:32 UTC ---
(In reply to comment #7)
> Because object sizes are finalized only during the objsz pass, after lots of
> optimization passes.  Note, as I said earlier, what matters most is that the
> check is performed at runtime in that case and thus the source code bug can't
> be exploited.  The warning is just to let the user know earlier than at
> runtime, when easily possible.

Maybe we are talking about different things. cppcheck seems to be able to give
the warning without any optimizations. The FE doesn't know that buf[2] is
length 2 and "ab" is length 3?

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-06 13:46:05 UTC ---
1) this is -D_FORTIFY_SOURCE warning, you can invent other warnings elsewhere
2) with -D_FORTIFY_SOURCE, e.g. sprintf is an inline function, so the FE sees
it as a call to an inline function with some argument, you need to inline it,
figure out what the inline does, then fold the builtins used in the inline.
Also consider
  char buf[2];
  char *p;
  p = buf;
  sprintf(buf, "ab%d", n);
Unless you move the optimization passes into the FE, you aren't going to warn
about this properly in the FE.  Insisting on a FE warning in this case is just
dumb.

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[fweimer at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com

--- Comment #10 from Florian Weimer <fweimer at redhat dot com> 2013-02-06 14:28:35 UTC ---
(In reply to comment #9)
> 1) this is -D_FORTIFY_SOURCE warning, you can invent other warnings elsewhere
> 2) with -D_FORTIFY_SOURCE, e.g. sprintf is an inline function, so the FE sees
> it as a call to an inline function with some argument, you need to inline it,
> figure out what the inline does, then fold the builtins used in the inline.
> Also consider
>   char buf[2];
>   char *p;
>   p = buf;
>   sprintf(buf, "ab%d", n);

I think you mean sprintf(p, ...).

> Unless you move the optimization passes into the FE, you aren't going to warn
> about this properly in the FE.  Insisting on a FE warning in this case is just
> dumb.

We could duplicate optimizations in the front end (like others do).

More seriously, I think this is a case where a layering violation makes perfect
sense.

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

--- Comment #11 from David Binderman <dcb314 at hotmail dot com> 2013-02-06 18:47:37 UTC ---
>It isn't that easy.  For %'s you really have to parse all the characters after
>% and figure out where the format specifier ends.

Agreed. But it doesn't have to be perfect, it just has to be
better than the previous solution. This is only a warning we are discussing.
It can be ignored. 

The Werror folks are, IMHO, far too keen for their own good.

I wrote:
>All the numeric specifiers (%d, %u etc) all produce at least one 
>character, so gcc could take account of this in checking buffer lengths.

In computing a lower bound, all % specifiers could be assumed to 
produce at least one byte of output.

Parsing the % specifier doesn't have to be perfect, it just has
to understand the common cases (%s, %d, %u etc) and punt on the rest.

More understanding can be added in later versions.

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

--- Comment #12 from David Binderman <dcb314 at hotmail dot com> 2013-02-07 21:19:15 UTC ---
I coded up some of my ideas into the attached C++ source code.

Then I modified builtins.c and did a bootstrap.
My small patch seems to be working well.

I'm sure someone could convert my C++ code into something
that follows the compiler coding conventions, add test cases etc.

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

--- Comment #13 from David Binderman <dcb314 at hotmail dot com> 2013-02-07 21:21:56 UTC ---
Created attachment 29391
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29391
C++ source code

This code doesn't understand all sprintf % specifiers.

It is future work to make it understand more specifiers.

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

--- Comment #14 from David Binderman <dcb314 at hotmail dot com> 2013-02-14 19:06:54 UTC ---
Created attachment 29453
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29453
C++ source code

Old version understood about a dozen formats, this 
later version understands 34 formats.

Next version will understand some field widths.

[Bug middle-end/54582] gap in FORTIFY checking of buffer lengths

[dcb314 at hotmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54582

--- Comment #15 from David Binderman <dcb314 at hotmail dot com> 2013-03-08 08:48:47 UTC ---
Created attachment 29615
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29615
C++ source code

This one now understands 40 common formats and can
properly interpret field widths.

The next version will understand the NUM1.NUM2 format.