[Bug middle-end/50460] New: [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[Bug middle-end/50460] New: [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[hjl.tools at gmail dot com]

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

             Bug #: 50460
           Summary: [4.7 Regression]
                    __builtin___strcpy_chk/__builtin_object_size don't
                    work
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: hjl.tools@gmail.com


On Linux/x86, revision 178028 gave

[hjl@gnu-34 gcc]$ cat ~/tmp/foo.c
const char *str1 = "JIHGFEDCBA";
extern __inline __attribute__ ((__always_inline__)) __attribute__
((__artificial__)) char *
__attribute__ ((__nothrow__)) strcpy (char *__restrict __dest, __const char
*__restrict __src)
{
  return __builtin___strcpy_chk (__dest, __src, __builtin_object_size (__dest,
2 > 1));
}
int
main ()
{
  struct A { char buf1[9]; char buf2[1]; } a;
  strcpy (a.buf1 + (0 + 4), str1 + 5);
  return 0;
}
[hjl@gnu-34 gcc]$ ./xgcc -B./ -O2  ~/tmp/foo.c
[hjl@gnu-34 gcc]$ ./a.out 
[hjl@gnu-34 gcc]$ 

instead of

[hjl@gnu-34 gcc]$ ./a.out 
*** buffer overflow detected ***: ./a.out terminated
======= Backtrace: =========
/lib64/libc.so.6(__fortify_fail+0x37)[0x3e56103ca7]
/lib64/libc.so.6[0x3e56101c20]
./a.out[0x40041e]
/lib64/libc.so.6(__libc_start_main+0xed)[0x3e560215bd]
./a.out[0x400451]
======= Memory map: ========
00400000-00401000 r-xp 00000000 08:11 52433950                          
/export/gnu/import/svn/gcc-test-ia32corei7/bld/gcc/a.out
00600000-00601000 rw-p 00000000 08:11 52433950                          
/export/gnu/import/svn/gcc-test-ia32corei7/bld/gcc/a.out
00a33000-00a54000 rw-p 00000000 00:00 0                                  [heap]
3e55c00000-3e55c22000 r-xp 00000000 08:06 392457                        
/lib64/ld-2.14.90.so
3e55e21000-3e55e22000 r--p 00021000 08:06 392457                        
/lib64/ld-2.14.90.so
3e55e22000-3e55e23000 rw-p 00022000 08:06 392457                        
/lib64/ld-2.14.90.so
3e55e23000-3e55e24000 rw-p 00000000 00:00 0 
3e56000000-3e561a6000 r-xp 00000000 08:06 392458                        
/lib64/libc-2.14.90.so
3e561a6000-3e563a5000 ---p 001a6000 08:06 392458                        
/lib64/libc-2.14.90.so
3e563a5000-3e563a9000 r--p 001a5000 08:06 392458                        
/lib64/libc-2.14.90.so
3e563a9000-3e563aa000 rw-p 001a9000 08:06 392458                        
/lib64/libc-2.14.90.so
3e563aa000-3e563b0000 rw-p 00000000 00:00 0 
3e57800000-3e57815000 r-xp 00000000 08:06 392602                        
/lib64/libgcc_s-4.6.0-20110603.so.1
3e57815000-3e57a14000 ---p 00015000 08:06 392602                        
/lib64/libgcc_s-4.6.0-20110603.so.1
3e57a14000-3e57a15000 rw-p 00014000 08:06 392602                        
/lib64/libgcc_s-4.6.0-20110603.so.1
7fb292f73000-7fb292f76000 rw-p 00000000 00:00 0 
7fb292f93000-7fb292f95000 rw-p 00000000 00:00 0 
7fff0d60a000-7fff0d62b000 rw-p 00000000 00:00 0                         
[stack]
7fff0d68a000-7fff0d68b000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                 
[vsyscall]
Aborted (core dumped)
[hjl@gnu-34 gcc]$ 

Revision 177983 is OK.

[Bug middle-end/50460] [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-09-20 09:59:49 UTC ---
Seems to be caused by PR48571, we throw away the important info (that the
access was through a.buf1 rather than &a), which is essential for
-D_FORTIFY_SOURCE=2.
The change happens already during gimplification:
-  strcpy (&a.buf1[4], D.2732);
+  strcpy (&MEM[(void *)&a + 4B], D.2732);
while in *.original it was
  strcpy ((char *) &a.buf1 + 4, str1 + 5);
Not reconstrucing the array ref is fine, but before *.objsz pass we really
shouldn't throw away the buf1 from it, so it should be tmp = &a.buf1 + 4;
-D_FORTIFY_SOURCE=2 cares whether the user wrote
  strcpy ((char *) &a + 4, ...); (in which case it allows to overwrite the
whole object) or strcpy ((char *) &a.buf1 + 4, ...); (in which case it is
allowed to overwrite just the buf1 field).

Richard, can you please have a look at this?

[Bug middle-end/50460] [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

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

--- Comment #2 from H.J. Lu <hjl.tools at gmail dot com> 2011-09-20 13:34:15 UTC ---
It is caused by revision 178312:

http://gcc.gnu.org/ml/gcc-cvs/2011-08/msg01330.html

[Bug middle-end/50460] [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2011-09-22
         AssignedTo|unassigned at gcc dot       |rguenth at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

--- Comment #3 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-09-22 21:39:11 UTC ---
(In reply to comment #1)
> Seems to be caused by PR48571, we throw away the important info (that the
> access was through a.buf1 rather than &a), which is essential for
> -D_FORTIFY_SOURCE=2.
> The change happens already during gimplification:
> -  strcpy (&a.buf1[4], D.2732);
> +  strcpy (&MEM[(void *)&a + 4B], D.2732);
> while in *.original it was
>   strcpy ((char *) &a.buf1 + 4, str1 + 5);
> Not reconstrucing the array ref is fine, but before *.objsz pass we really
> shouldn't throw away the buf1 from it, so it should be tmp = &a.buf1 + 4;
> -D_FORTIFY_SOURCE=2 cares whether the user wrote
>   strcpy ((char *) &a + 4, ...); (in which case it allows to overwrite the
> whole object) or strcpy ((char *) &a.buf1 + 4, ...); (in which case it is
> allowed to overwrite just the buf1 field).
> 
> Richard, can you please have a look at this?

Sure.  Avoiding this during gimplification should be possible - but
preserving &a.buf1 + 4 in some form until objsz will be impossible
I fear.  It looks like objsz relies on something that GIMPLE does not
provide.

I'm sure we don't guarantee that any obfuscation of a.buf1 + 4 doesn't
trip on fortify=2, do we?

I suppose the error is in &MEM[(void *)&a + 4B] objsz handling (again).

[Bug middle-end/50460] [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-09-23 07:26:10 UTC ---
Looking at:
const char *str1 = "JIHGFEDCBA";
#define strcpy(x,y) __builtin___strcpy_chk (x, y, __builtin_object_size (x, 1))

int
f1 (void)
{
  struct A { char buf1[9]; char buf2[1]; } a;
  strcpy (a.buf1 + (0 + 4), str1 + 5);
  return 0;
}

int
f2 (void)
{
  struct A { char buf1[9]; char buf2[1]; } a;
  strcpy ((char *) &a + (0 + 4), str1 + 5);
  return 0;
}

int
f3 (void)
{
  struct A { char buf1[9]; char buf2[1]; } a;
  char *p = (char *) &a;
  strcpy (p + (0 + 4), str1 + 5);
  return 0;
}

int
f4 (void)
{
  struct A { char buf0; char buf1[9]; char buf2[1]; } a;
  char *p = (char *) &a;
  strcpy (p + (0 + 5), str1 + 5);
  return 0;
}

int
f5 (void)
{
  struct A { char buf0; char buf1[9]; char buf2[1]; } a;
  strcpy ((char *) &a + (0 + 5), str1 + 5);
  return 0;
}

with GCC 4.4, seems we have always reconstructed it into &a.buf1[4].
So likely we want to reconstruct it from the MEM_REF in the *.objsz pass then.
If there is union involved, we probably want to reconstruct it to the
alternative with the largest possible __builtin_object_size (X, 1) resp.
smallest possible __builtin_object_size (X, 3).

[Bug middle-end/50460] [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-09-25 11:11:21 UTC ---
(In reply to comment #4)
> Looking at:
> const char *str1 = "JIHGFEDCBA";
> #define strcpy(x,y) __builtin___strcpy_chk (x, y, __builtin_object_size (x, 1))
> 
> int
> f1 (void)
> {
>   struct A { char buf1[9]; char buf2[1]; } a;
>   strcpy (a.buf1 + (0 + 4), str1 + 5);
>   return 0;
> }
> 
> int
> f2 (void)
> {
>   struct A { char buf1[9]; char buf2[1]; } a;
>   strcpy ((char *) &a + (0 + 4), str1 + 5);
>   return 0;
> }
> 
> int
> f3 (void)
> {
>   struct A { char buf1[9]; char buf2[1]; } a;
>   char *p = (char *) &a;
>   strcpy (p + (0 + 4), str1 + 5);
>   return 0;
> }
> 
> int
> f4 (void)
> {
>   struct A { char buf0; char buf1[9]; char buf2[1]; } a;
>   char *p = (char *) &a;
>   strcpy (p + (0 + 5), str1 + 5);
>   return 0;
> }
> 
> int
> f5 (void)
> {
>   struct A { char buf0; char buf1[9]; char buf2[1]; } a;
>   strcpy ((char *) &a + (0 + 5), str1 + 5);
>   return 0;
> }
> 
> with GCC 4.4, seems we have always reconstructed it into &a.buf1[4].
> So likely we want to reconstruct it from the MEM_REF in the *.objsz pass then.
> If there is union involved, we probably want to reconstruct it to the
> alternative with the largest possible __builtin_object_size (X, 1) resp.
> smallest possible __builtin_object_size (X, 3).

I'm not sure.  What's the C / fortify difference of a.buf1 + 9 vs. a.buf2?
Both would be MEM[&a, 9].  I suppose we didn't re-construct array-refs in
4.4 from

 void *p = a.buf1;
 char *q = p + 4;

so, did we fail with 4.4 here, too?

[Bug middle-end/50460] [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-09-26 09:08:36 UTC ---
#define strcpy(x,y) __builtin___strcpy_chk (x, y, __builtin_object_size (x, 1))

int
f1 (void)
{
  struct A { char buf1[9]; char buf2[4]; } a;
  strcpy (a.buf1 + 9, "a");
  return 0;
}

int
f2 (void)
{
  struct A { char buf1[9]; char buf2[4]; } a;
  strcpy (a.buf2 + 0, "a");
  return 0;
}

int
f3 (void)
{
  struct A { char buf1[9]; char buf2[4]; } a;
  strcpy (a.buf1 + 10, "a");
  return 0;
}

int
f4 (void)
{
  struct A { char buf1[9]; char buf2[4]; } a;
  strcpy (a.buf2 - 1, "a");
  return 0;
}

int
f5 (void)
{
  struct A { char buf1[9]; char buf2[4]; } a;
  strcpy ((char *) &a + 10, "a");
  return 0;
}

int
f6 (void)
{
  struct A { char buf1[9]; char buf2[4]; } a;
  strcpy ((char *) a.buf2 - 1, "a");
  return 0;
}

used to warn in f{1,3,4,6} (and fail at runtime) and not in f{2,5} in
4.{1,2,3,4,6} (haven't checked 4.5), doesn't warn nor fail on the trunk.
So yes, 4.0+ clearly did some reconstruction, but only in limited cases (e.g.
when the &a is offsetted).  Some field + offset remained COMPONENT_REF +
offset.

[Bug middle-end/50460] [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic

--- Comment #7 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-09-28 12:23:15 UTC ---
Btw, this is equivalent to a missing diagnostic, it's correctly not trapping
similar as to if it didn't know anything about the object that is refered to.

Indeed when I disable the folding during gimplification CCP comes along
and does

 <bb 2>:
   str1.0_1 = str1;
   D.2732_2 = str1.0_1 + 5;
-  D.2733_3 = &a.buf1 + 4;
-  __dest_7 = (char * restrict) D.2733_3;
   __src_8 = (const char * restrict) D.2732_2;
-  D.2747_9 = __builtin_object_size (__dest_7, 1);
-  D.2746_10 = __builtin___strcpy_chk (__dest_7, __src_8, D.2747_9);
-  D.2746_12 = D.2746_10;
-  D.2734_4 = 0;
-  return D.2734_4;
+  D.2746_10 = __builtin___strcpy_chk (&MEM[(void *)&a + 4B], __src_8, 6);
+  return 0;

which is good, as the address is invariant.

So, short of moving the objsize pass way earlier (which I'm sure we don't
want to do), I don't see a good way to recover this diagnostic.

One possibility is to make sure try_move_mult_to_index handles the case
of &a.buf1 + 4, instead of just &a.buf1[0] + 4.  I have a patch for that.

[Bug middle-end/50460] [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[rguenth at gcc dot gnu.org]

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

--- Comment #8 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-09-28 13:47:16 UTC ---
Author: rguenth
Date: Wed Sep 28 13:47:12 2011
New Revision: 179313

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=179313
Log:
2011-09-28  Richard Guenther  <rguenther@suse.de>

    PR middle-end/50460
    * fold-const.c (try_move_mult_to_index): Handle &a.array the
    same as &a.array[0].

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/fold-const.c

[Bug middle-end/50460] [4.7 Regression] __builtin___strcpy_chk/__builtin_object_size don't work

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #9 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-09-28 13:47:57 UTC ---
Fixed^WWorked around.