[Bug sanitizer/61293] New: asan can not find left buffer overflow of new[]-allocated buffer, frontend help needed

[Bug sanitizer/61293] New: asan can not find left buffer overflow of new[]-allocated buffer, frontend help needed

[kcc at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61293

            Bug ID: 61293
           Summary: asan can not find left buffer overflow of
                    new[]-allocated buffer, frontend help needed
           Product: gcc
           Version: 4.10.0
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: kcc at gcc dot gnu.org
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org,
                    timurrrr at google dot com

asan does not detect the following case: 

TypeWithDtor *a = new TypeWithDtor[N];
a[-1] = ... 
https://code.google.com/p/address-sanitizer/issues/detail?id=314

That's because when we have new[] for a type with DTORs, 
the actual allocated size is greater.
The code looks something like this:
  extra = max(sizeof(long), alignment_of(TypeWithDtor));
  ptr = malloc(N + extra);
  *(long*)(ptr+extra-sizeof(long)) = N;
  return ptr + extra;  // must be properly aligned for TypeWithDtor

As the result, we will not detect overwrites of new[] cookie -- scary! 

I don't see how we can implement this w/o help from FE. 

First, we need to ensure alignment 8 even on 32-bits: 
  extra = max(8, alignment_of(TypeWithDtor));  

Second, we need to poison the first extra bytes.

Lastly, we need to not instrument the legitimate loads/stores of the cookie
generated by the frontend. 

All of this has to be done with the help from FE

[Bug sanitizer/61293] asan can not find left buffer overflow of new[]-allocated buffer, frontend help needed

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61293

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

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
IMNSHO you can't change the value of extra, that is an ABI issue,
and -fsanitize=address shouldn't be an ABI changing option.  Consider:
struct S { S (); ~S (); };
S *foo (int n) { return new S[n]; }
void bar (S *p) { delete [] p; }
int main () { bar (foo (5)); }
where bar is defined in a different compilation unit/library and something is
built with -fsanitize=address, something is not.

If the padding before structure is at least 64-bit, sure, instrumenting the FE
to put there an __asan_poison_memory_region call after the size is stored there
and in delete[] again to __asan_unpoison_memory_region before reading the size
should not be that hard.

For 32-bit code if the type doesn't need at least 64-bit alignment (again,
alignment of the type is an ABI thing), you are out of luck I'm afraid.
Thus, e.g. tests for this will need to be conditionalized.

[Bug sanitizer/61293] asan can not find left buffer overflow of new[]-allocated buffer, frontend help needed

[kcc at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61293

--- Comment #2 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #1)
> IMNSHO you can't change the value of extra, that is an ABI issue,
> and -fsanitize=address shouldn't be an ABI changing option.  Consider:
> struct S { S (); ~S (); };
> S *foo (int n) { return new S[n]; }
> void bar (S *p) { delete [] p; }
> int main () { bar (foo (5)); }
> where bar is defined in a different compilation unit/library and something
> is built with -fsanitize=address, something is not.
> 
> If the padding before structure is at least 64-bit, sure, instrumenting the
> FE to put there an __asan_poison_memory_region call after the size is stored

yep

> there
> and in delete[] again to __asan_unpoison_memory_region before reading the
> size should not be that hard.

Yes, but a bit more preferable is to ignore the instructions
reading the size instead of calling __asan_unpoison_memory_region. 
Consider a case where the DTORs are accessing the array itself out of bounds.
(We've seen similar things!!)
That's a bit harder to implement though. 

> 
> For 32-bit code if the type doesn't need at least 64-bit alignment (again,
> alignment of the type is an ABI thing), you are out of luck I'm afraid.
Yea... We can theoretically request operator new to 
return memory that is == 4 mod 8 for these cases. 
That's a bit complicated too...



> Thus, e.g. tests for this will need to be conditionalized.

[Bug sanitizer/61293] asan can not find left buffer overflow of new[]-allocated buffer, frontend help needed

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61293

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Kostya Serebryany from comment #2)
> (In reply to Jakub Jelinek from comment #1)
> > IMNSHO you can't change the value of extra, that is an ABI issue,
> > and -fsanitize=address shouldn't be an ABI changing option.  Consider:
> > struct S { S (); ~S (); };
> > S *foo (int n) { return new S[n]; }
> > void bar (S *p) { delete [] p; }
> > int main () { bar (foo (5)); }
> > where bar is defined in a different compilation unit/library and something
> > is built with -fsanitize=address, something is not.
> > 
> > If the padding before structure is at least 64-bit, sure, instrumenting the
> > FE to put there an __asan_poison_memory_region call after the size is stored
> 
> yep
> 
> > there
> > and in delete[] again to __asan_unpoison_memory_region before reading the
> > size should not be that hard.
> 
> Yes, but a bit more preferable is to ignore the instructions
> reading the size instead of calling __asan_unpoison_memory_region. 
> Consider a case where the DTORs are accessing the array itself out of bounds.
> (We've seen similar things!!)
> That's a bit harder to implement though. 

Yeah, that makes it uglier, supposedly easiest (in GCC) would be to replace
the memory store to the area before the returned pointer with some magic
builtin user can't normally use, ditto for the load and add another builtin
right before the actual operator delete call, assign a new 0xeN? shadow value
for "the area before operator new", and in asan pass just expand those calls to
more code (the first one to a normal 0 check of shadow plus memory store,
followed by manually poisioning the shadow with the new magic constant, the
second one which would check if the shadow at that point is either 0 or the
0xeN? magic value, otherwise die, then load, and finally the last builtin that
would unpoison it.

> > For 32-bit code if the type doesn't need at least 64-bit alignment (again,
> > alignment of the type is an ABI thing), you are out of luck I'm afraid.
> Yea... We can theoretically request operator new to 
> return memory that is == 4 mod 8 for these cases. 

No, because you don't know when some type that will actually need 64-bit
alignment is used.  Because if you misalign the returned value, then such types
would not be aligned properly.

[Bug sanitizer/61293] asan can not find left buffer overflow of new[]-allocated buffer, frontend help needed

[kcc at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61293

--- Comment #4 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
FTR, this has been implemented in Clang (Itanium ABI only for now): 
clang: http://llvm.org/viewvc/llvm-project?view=revision&revision=216434
asan: http://llvm.org/viewvc/llvm-project?rev=214711&view=rev
test:  http://llvm.org/viewvc/llvm-project?rev=216494&view=rev