[Bug ada/55485] New: stack-buffer-overflow in sem_ch8.adb

[Bug ada/55485] New: stack-buffer-overflow in sem_ch8.adb

[hjl.tools at gmail dot com]

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

             Bug #: 55485
           Summary: stack-buffer-overflow in sem_ch8.adb
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ada
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: hjl.tools@gmail.com


On Linux/x86-64, hjl/asan branch gives:

/export/build/gnu/gcc-asan/build-x86_64-linux/./gcc/xgcc
-B/export/build/gnu/gcc-asan/build-x86_64-linux/./gcc/
-B/usr/local/x86_64-unknown-linux-gnu/bin/
-B/usr/local/x86_64-unknown-linux-gnu/lib/ -isystem
/usr/local/x86_64-unknown-linux-gnu/include -isystem
/usr/local/x86_64-unknown-linux-gnu/sys-include    -c -g -O2  -fpic  -W -Wall
-gnatpg -nostdinc   s-auxdec.adb -o s-auxdec.o
==2916== ERROR: AddressSanitizer: stack-buffer-overflow on address
0x7fff47f1b588 at pc 0xb6e8f4 bp 0x7fff47f1b4e0 sp 0x7fff47f1b4d8
WRITE of size 4 at 0x7fff47f1b588 thread T0
    #0 0xb6e8f3
(/export/build/gnu/gcc-asan/build-x86_64-linux/gcc/gnat1+0xb6e8f3)
Address 0x7fff47f1b588 is located at offset 72 in frame
<ada__exceptions__raise_current_excep> of T0's stack:
  This frame has 1 object(s):
    [32, 40) 'id'
HINT: this may be a false positive if your program uses some custom stack
unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
Shadow byte and word:
  0x1fffe8fe36b1: f3
  0x1fffe8fe36b0: f3 f3 f3 f3 00 00 00 00
More shadow bytes:
  0x1fffe8fe3690: 00 00 00 00 00 00 00 00
  0x1fffe8fe3698: 00 00 00 00 00 00 00 00
  0x1fffe8fe36a0: 00 00 00 00 00 00 00 00
  0x1fffe8fe36a8: f1 f1 f1 f1 00 f4 f4 f4
=>0x1fffe8fe36b0: f3 f3 f3 f3 00 00 00 00
  0x1fffe8fe36b8: 00 00 00 00 00 00 00 00
  0x1fffe8fe36c0: 00 00 00 00 00 00 00 00
  0x1fffe8fe36c8: 00 00 00 00 00 00 00 00
  0x1fffe8fe36d0: 00 00 00 00 00 00 00 00
Stats: 4M malloced (2M for red zones) by 2930 calls
Stats: 0M realloced by 258 calls
Stats: 0M freed by 567 calls
Stats: 0M really freed by 0 calls
Stats: 9M (2443 full pages) mmaped in 16 calls
  mmaps   by size class: 7:4095; 8:2047; 9:1023; 10:511; 11:255; 12:128; 13:64;
14:32; 15:16; 16:8; 17:4; 18:6; 19:1; 21:1;
  mallocs by size class: 7:1785; 8:688; 9:53; 10:88; 11:226; 12:35; 13:17;
14:14; 15:6; 16:7; 17:3; 18:6; 19:1; 21:1;
  frees   by size class: 7:267; 8:52; 9:32; 10:67; 11:131; 12:16; 13:1; 14:1;
  rfrees  by size class:
Stats: malloc large: 24 small slow: 49
==2916== ABORTING
make[9]: *** [s-auxdec.o] Error 1
[hjl@gnu-mic-2 ~]$ addr2line -e
/export/build/gnu/gcc-asan/build-x86_64-linux/gcc/gnat1 0xb6e8f3
/export/gnu/import/git/gcc/gcc/ada/sem_ch8.adb:4038
[hjl@gnu-mic-2 ~]$

[Bug sanitizer/55485] probable false positive on __builtin_setjmp/__builtin_longjmp

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-11-27
          Component|ada                         |sanitizer
                 CC|                            |dodji at gcc dot gnu.org,
                   |                            |dvyukov at gcc dot gnu.org,
                   |                            |ebotcazou at gcc dot
                   |                            |gnu.org, jakub at gcc dot
                   |                            |gnu.org, kcc at gcc dot
                   |                            |gnu.org
     Ever Confirmed|0                           |1
            Summary|stack-buffer-overflow in    |probable false positive on
                   |sem_ch8.adb                 |__builtin_setjmp/__builtin_
                   |                            |longjmp

--- Comment #1 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-11-27 16:12:57 UTC ---
It looks rather like AddressSanitizer is confused by the __builtin_setjmp based
exception handling scheme, as hinted at by:

Address 0x7fff47f1b588 is located at offset 72 in frame
<ada__exceptions__raise_current_excep> of T0's stack:
  This frame has 1 object(s):
    [32, 40) 'id'
HINT: this may be a false positive if your program uses some custom stack
unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)

So, does AddressSanitizer support __builtin_setjmp/__builtin_longjmp?

[Bug sanitizer/55485] probable false positive on __builtin_setjmp/__builtin_longjmp

[konstantin.s.serebryany at gmail dot com]

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

Konstantin Serebryany <konstantin.s.serebryany at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |konstantin.s.serebryany at
                   |                            |gmail dot com

--- Comment #2 from Konstantin Serebryany <konstantin.s.serebryany at gmail dot com> 2012-11-27 16:48:48 UTC ---
>> So, does AddressSanitizer support __builtin_setjmp/__builtin_longjmp?
We've never seen those, so I guess not. 
Can they be lowered to regular setjmp/longjmp calls? 
If yes, then the run-time library interceptor should take care of them.

[Bug sanitizer/55485] probable false positive on __builtin_setjmp/__builtin_longjmp

[ebotcazou at gcc dot gnu.org]

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

--- Comment #3 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-11-27 17:44:06 UTC ---
> Can they be lowered to regular setjmp/longjmp calls? 
> If yes, then the run-time library interceptor should take care of them.

The purpose of __builtin_setjmp/__builtin_longjmp is precisely to avoid using
setjmp/longjmp so I don't think that would be very desirable.

[Bug sanitizer/55485] probable false positive on __builtin_setjmp/__builtin_longjmp

[konstantin.s.serebryany at gmail dot com]

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

--- Comment #4 from Konstantin Serebryany <konstantin.s.serebryany at gmail dot com> 2012-11-27 17:52:02 UTC ---
For what purpose would any one avoid longjmp call, other than for performance? 
Under asan, performance already drops by 2x, so using calls will not hurt much. 
Of course, we could instrument __builtin_longjmp call in the compiler module, 
but is it worth it? 
If yes, all we need is to call __asan_handle_no_return before __builtin_longjmp

[Bug sanitizer/55485] probable false positive on __builtin_setjmp/__builtin_longjmp

[ebotcazou at gcc dot gnu.org]

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

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-11-27 18:06:58 UTC ---
> For what purpose would any one avoid longjmp call, other than for performance? 
> Under asan, performance already drops by 2x, so using calls will not hurt much. 
> Of course, we could instrument __builtin_longjmp call in the compiler module, 
> but is it worth it?

You won't be instrumenting the original executable if you don't do it.

> If yes, all we need is to call __asan_handle_no_return before __builtin_longjmp

Thanks for the tip, I'll give it a try.

[Bug sanitizer/55485] probable false positive on __builtin_setjmp/__builtin_longjmp

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |howarth at nitro dot
                   |                            |med.uc.edu

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-28 08:11:16 UTC ---
*** Bug 55502 has been marked as a duplicate of this bug. ***

[Bug sanitizer/55485] probable false positive on __builtin_setjmp/__builtin_longjmp

[kcc at gcc dot gnu.org]

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

--- Comment #7 from Kostya Serebryany <kcc at gcc dot gnu.org> 2012-11-28 13:37:02 UTC ---
Note that the LLVM implementation inserts a call to __asan_handle_no_return
before every "no-return" call instruction.

[Bug sanitizer/55485] probable false positive on __builtin_setjmp/__builtin_longjmp

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-28 13:47:41 UTC ---
If I understand it right, that clears all shadow memory corresponding to
current thread's stack, rather than trying to figure out into which function it
longjmps and clearing only everything up to that frame, right?  Might then lead
to not reporting failures afterwards.  But sure, we could do that (but I'd
prefer to do it only after the asan/tsan builtins patch is reviewed).  Do you
do that just for noreturn calls?  What about say __builtin_trap () or
__builtin_unreachable ()?
Though in the asan pass they are likely still represented as noreturn calls and
can be handled the same way.

[Bug sanitizer/55485] probable false positive on __builtin_setjmp/__builtin_longjmp

[kcc at gcc dot gnu.org]

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

--- Comment #9 from Kostya Serebryany <kcc at gcc dot gnu.org> 2012-11-28 14:00:53 UTC ---
Correct. 
__asan_handle_no_return may loose some of the stack-buffer overflows. 
It is also used to handle clone case, where the entire stack should be
unpoisoned. 
http://code.google.com/p/address-sanitizer/issues/detail?id=37&can=1&q=clone


>> rather than trying to figure out into which function it
>> longjmps and clearing only everything up to that frame, right
I am not sure how to do it w/o going too deep inside the longjmp
implementation. 
The code we care about almost never uses longjmp (and C++ exceptions) so we
didn't bother. But yes, we have this case of "false negative".

>>  Do you do that just for noreturn calls? 

Yes, we just rely on LLVM telling us that a call is noreturn.
(haha, there is actually a minor problem in our LLVM pass.
http://code.google.com/p/address-sanitizer/issues/detail?id=129
)

>> What about say __builtin_trap () or __builtin_unreachable ()?

__builtin_trap is not asan-hostile in this manner.
Today we don't prepend it with __asan_handle_no_return