Page MenuHomePhabricator

[clang/asan] call __asan_poison_cxx_array_cookie after operator new[]
ClosedPublic

Authored by kcc on Aug 4 2014, 5:58 AM.

Details

Summary

PR19838
When operator new[] is called and an array cookie is created
we want asan to detect buffer overflow bugs that touch the cookie.
For that we need to

a) poison the shadow for the array cookie (call __asan_poison_cxx_array_cookie).
b) ignore the legal accesses to the cookie generated by clang (add 'nosanitize' metadata)

Diff Detail

Event Timeline

kcc updated this revision to Diff 12155.Aug 4 2014, 5:58 AM
kcc retitled this revision from to [clang/asan] call __asan_poison_cxx_array_cookie after operator new[].
kcc updated this object.
kcc edited the test plan for this revision. (Show Details)
kcc added reviewers: samsonov, rsmith.
kcc added a subscriber: Unknown Object (MLST).
kcc updated this revision to Diff 12156.Aug 4 2014, 6:06 AM

remove unnecessary file

samsonov edited edge metadata.Aug 21 2014, 10:46 AM

Sorry for delay. Looking at it now.

samsonov added inline comments.
lib/CodeGen/ItaniumCXXABI.cpp
1450

Hm, I think we'd need to ask Timur to implement similar logic for Microsoft ABI?

1482

Please start local variable names with capitals

1483

Why don't you use fixed type for __asan_poison_cxx_array_cookie (like IntptrTy)?

rsmith edited edge metadata.Aug 21 2014, 11:25 AM

Is this change correct? Suppose I do this:

char Buffer[32];
// ...
new (Buffer) int[4];
// ...
new (Buffer) int(0);

Won't we get a false positive on the last line?

I thought of one more test case we should add:

__attribute__((no_sanitize_address))
int *createArray(int n) {
  return new int[n];
}

int bad_access() {  
  int *array = createArray(4);
  return array[-1];
}

We certainly want to print an error in this case, even though we have attribute on createArray() function. I believe current code *would* handle this correctly, but let's test this behavior.

timurrrr edited edge metadata.Aug 22 2014, 5:34 AM

Am I right that skipping MicrosoftCXXABI for a while won't give us false positives?
If so, I'd wait until a change to ItaniumCXXABI lands and then I can just do that for MicrosoftCXXABI.

Sure, we can add support for this to MicrosoftCXXABI later.

kcc added a comment.Aug 25 2014, 11:20 AM
In D4774#11, @samsonov wrote:

I thought of one more test case we should add:

__attribute__((no_sanitize_address))
int *createArray(int n) {
  return new int[n];
}
 
int bad_access() {  
  int *array = createArray(4);
  return array[-1];
}

We certainly want to print an error in this case, even though we have attribute on createArray() function. I believe current code *would* handle this correctly, but let's test this behavior.

I'd prefer to add such test to compiler-rt, not to clang, as it is a run-time test.

In D4774#10, @rsmith wrote:

Is this change correct? Suppose I do this:

char Buffer[32];
// ...
new (Buffer) int[4];
// ...
new (Buffer) int(0);

Won't we get a false positive on the last line?

Not sure I understand this test.
First, with arrays of PODs you don't have cookies at all.
Second, do we have the cookie with placement new at all?
Pardon my ignorance here; this is my test case:

#include <stdio.h>
#include <new>
struct C {
  int x;
  ~C() {fprintf(stderr, "ZZZZZZZZ\n");}
};
char Buffer[100];
C *c;
int main(int argc, char **argv) { 
  c = new (Buffer) C[argc];
}

clang++ -O -fsanitize=address ~/tmp/new_ar_placement.cc -S -o - -emit-llvm

define i32 @main(i32 %argc, i8** nocapture readnone %argv) #0 {
entry:

store %struct.C* bitcast ({ [100 x i8], [60 x i8] }* @Buffer to %struct.C*), %struct.C** getelementptr inbounds ({ %struct.C*, [56 x i8] }* @c, i32 0, i32 0), align 8, !tbaa !5
ret i32 0

}

// No cookie

lib/CodeGen/ItaniumCXXABI.cpp
1450

yes, but not in this round

1482

done

1483

What's wrong with i64* ?
Having I64 here will just require one more cast

kcc updated this revision to Diff 12906.Aug 25 2014, 11:24 AM
kcc edited edge metadata.

address some of the comments

kcc updated this revision to Diff 12914.Aug 25 2014, 1:10 PM

Don't insert insrumentation for placement new, add a relevant test.

Richard, PTAL

rsmith added inline comments.Aug 25 2014, 4:10 PM
lib/CodeGen/ItaniumCXXABI.cpp
1480

Use expr->getOperatorNew()->isReplaceableGlobalAllocationFunction() here, so that you also sanitize new (std::nothrow) X[n].

kcc updated this revision to Diff 12924.Aug 25 2014, 4:24 PM

Use expr->getOperatorNew()->isReplaceableGlobalAllocationFunction()
and add a test to verify that new (std::nothrow) X[n] is instrumented.

PTAL

rsmith accepted this revision.Aug 25 2014, 6:33 PM
rsmith edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 25 2014, 6:33 PM
kcc closed this revision.Aug 25 2014, 7:39 PM