diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp --- a/clang/lib/CodeGen/ItaniumCXXABI.cpp +++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp @@ -2339,32 +2339,26 @@ QualType ElementType) { assert(requiresArrayCookie(expr)); + CharUnits cookieSize = getArrayCookieSizeImpl(ElementType); unsigned AS = NewPtr.getAddressSpace(); - ASTContext &Ctx = getContext(); - CharUnits SizeSize = CGF.getSizeSize(); - - // The size of the cookie. - CharUnits CookieSize = - std::max(SizeSize, Ctx.getPreferredTypeAlignInChars(ElementType)); - assert(CookieSize == getArrayCookieSizeImpl(ElementType)); - // Compute an offset to the cookie. - Address CookiePtr = NewPtr; - CharUnits CookieOffset = CookieSize - SizeSize; - if (!CookieOffset.isZero()) - CookiePtr = CGF.Builder.CreateConstInBoundsByteGEP(CookiePtr, CookieOffset); + Address cookiePtr = NewPtr; + CharUnits cookieOffset = cookieSize - CGF.getSizeSize(); + if (!cookieOffset.isZero()) + cookiePtr = CGF.Builder.CreateConstInBoundsByteGEP(cookiePtr, cookieOffset); // Write the number of elements into the appropriate slot. Address NumElementsPtr = - CGF.Builder.CreateElementBitCast(CookiePtr, CGF.SizeTy); + CGF.Builder.CreateElementBitCast(cookiePtr, CGF.SizeTy); llvm::Instruction *SI = CGF.Builder.CreateStore(NumElements, NumElementsPtr); // Handle the array cookie specially in ASan. - if (CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) && AS == 0 && + if (CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) && + NewPtr.getAddressSpace() == 0 && (expr->getOperatorNew()->isReplaceableGlobalAllocationFunction() || CGM.getCodeGenOpts().SanitizeAddressPoisonCustomArrayCookie)) { - // The store to the CookiePtr does not need to be instrumented. + // The store to the cookiePtr does not need to be instrumented. CGM.getSanitizerMetadata()->disableSanitizerForInstruction(SI); llvm::FunctionType *FTy = llvm::FunctionType::get(CGM.VoidTy, NumElementsPtr.getType(), false); @@ -2375,7 +2369,7 @@ // Finally, compute a pointer to the actual data buffer by skipping // over the cookie completely. - return CGF.Builder.CreateConstInBoundsByteGEP(NewPtr, CookieSize); + return CGF.Builder.CreateConstInBoundsByteGEP(NewPtr, cookieSize); } llvm::Value *ItaniumCXXABI::readArrayCookieImpl(CodeGenFunction &CGF, @@ -2422,37 +2416,27 @@ llvm::Value *numElements, const CXXNewExpr *expr, QualType elementType) { - assert(requiresArrayCookie(expr)); - - // The cookie is always at the start of the buffer. - Address cookie = newPtr; - // The first element is the element size. - cookie = CGF.Builder.CreateElementBitCast(cookie, CGF.SizeTy); - llvm::Value *elementSize = llvm::ConstantInt::get(CGF.SizeTy, - getContext().getTypeSizeInChars(elementType).getQuantity()); - CGF.Builder.CreateStore(elementSize, cookie); + // getArrayCookieSizeImpl() should have left room for two elements + assert(ARMCXXABI::getArrayCookieSizeImpl(elementType) >= + CharUnits::fromQuantity(2*CGM.SizeSizeInBytes)); - // The second element is the element count. - cookie = CGF.Builder.CreateConstInBoundsGEP(cookie, 1); - CGF.Builder.CreateStore(numElements, cookie); + // Prepend the additional ARM64 cookie data (elementSize) + Address xCookie = CGF.Builder.CreateElementBitCast(newPtr, CGF.SizeTy); + llvm::Value *elementSize = + llvm::ConstantInt::get(CGF.SizeTy, + getContext().getTypeSizeInChars(elementType).getQuantity()); + CGF.Builder.CreateStore(elementSize, xCookie); - // Finally, compute a pointer to the actual data buffer by skipping - // over the cookie completely. - CharUnits cookieSize = ARMCXXABI::getArrayCookieSizeImpl(elementType); - return CGF.Builder.CreateConstInBoundsByteGEP(newPtr, cookieSize); + // The rest of the cookie handling is the same + return ItaniumCXXABI::InitializeArrayCookie(CGF,newPtr, numElements, expr, + elementType); } llvm::Value *ARMCXXABI::readArrayCookieImpl(CodeGenFunction &CGF, Address allocPtr, CharUnits cookieSize) { - // The number of elements is at offset sizeof(size_t) relative to - // the allocated pointer. - Address numElementsPtr - = CGF.Builder.CreateConstInBoundsByteGEP(allocPtr, CGF.getSizeSize()); - - numElementsPtr = CGF.Builder.CreateElementBitCast(numElementsPtr, CGF.SizeTy); - return CGF.Builder.CreateLoad(numElementsPtr); + return ItaniumCXXABI::readArrayCookieImpl(CGF, allocPtr, cookieSize); } /*********************** Static local initialization **************************/ diff --git a/compiler-rt/lib/asan/asan_poisoning.cpp b/compiler-rt/lib/asan/asan_poisoning.cpp --- a/compiler-rt/lib/asan/asan_poisoning.cpp +++ b/compiler-rt/lib/asan/asan_poisoning.cpp @@ -259,6 +259,10 @@ if (!flags()->poison_array_cookie) return; uptr s = MEM_TO_SHADOW(p); *reinterpret_cast(s) = kAsanArrayCookieMagic; +#if SANITIZER_ARM64 + // The ARM64 cookie has a second "size_t" entry so poison it as well + *(reinterpret_cast(s)-1) = kAsanArrayCookieMagic; +#endif } extern "C" SANITIZER_INTERFACE_ATTRIBUTE diff --git a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp --- a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp +++ b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp @@ -1,16 +1,12 @@ // REQUIRES: asan-64-bits // RUN: %clangxx_asan -O3 %s -o %t -// RUN: not %run %t 2>&1 | FileCheck %s +// RUN: not %run %t 2>&1 | FileCheck %s // RUN: %env_asan_opts=poison_array_cookie=1 not %run %t 2>&1 | FileCheck %s // RUN: %env_asan_opts=poison_array_cookie=0 not %run %t 2>&1 | FileCheck %s --check-prefix=NO_COOKIE -// UNSUPPORTED: ios - -// Added to allow enabling of tests but needs investigation (rdar://91448627) -// XFAIL: (darwin && (arm64-target-arch || arm64e-target-arch)) - #include #include + struct C { int x; ~C() { @@ -20,11 +16,11 @@ }; int main(int argc, char **argv) { - C *buffer = new C[argc]; - buffer[-2].x = 10; + C *buffer = new C[1]; + reinterpret_cast(buffer)[-1] = 42; // CHECK: AddressSanitizer: heap-buffer-overflow // CHECK: in main {{.*}}new_array_cookie_test.cpp:[[@LINE-2]] -// CHECK: is located 0 bytes inside of 12-byte region +// CHECK: is located {{7 bytes inside of 12|15 bytes inside of 20}}-byte region // NO_COOKIE: ZZZZZZZZ delete [] buffer; } diff --git a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp --- a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp +++ b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp @@ -3,15 +3,10 @@ // RUN: %env_asan_opts=poison_array_cookie=1 not %run %t 2>&1 | FileCheck %s --check-prefix=COOKIE // RUN: %env_asan_opts=poison_array_cookie=0 not %run %t 2>&1 | FileCheck %s --check-prefix=NO_COOKIE -// UNSUPPORTED: ios - -// Added to allow enabling of tests but needs investigation (rdar://91448627) -// XFAIL: (darwin && (arm64-target-arch || arm64e-target-arch)) - -#include -#include #include -int dtor_counter; +#include + +int dtor_counter=0; struct C { int x; ~C() { @@ -22,12 +17,11 @@ __attribute__((noinline)) void Delete(C *c) { delete[] c; } __attribute__((no_sanitize_address)) void Write42ToCookie(C *c) { - long *p = reinterpret_cast(c); - p[-1] = 42; + reinterpret_cast(c)[-1] = 42; } int main(int argc, char **argv) { - C *buffer = new C[argc]; + C *buffer = new C[1]; delete [] buffer; Write42ToCookie(buffer); delete [] buffer; diff --git a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp --- a/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp +++ b/compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp @@ -1,16 +1,22 @@ -// Test that we do not poison the array cookie if the operator new is defined -// inside the class. -// RUN: %clangxx_asan %s -o %t && %run %t +// For arrays allocated by classes that define their own custom "new", ASAN +// should NOT poison the array cookie unless the compilation unit is compiled +// with "-fsanitize-address-poison-custom-array-cookie". +// Here we test that: +// 1) w/o ASan, the array cookie location IS writable +// 2) w/ASan, the array cookie location IS writeable by default +// 3) w/ASan, the flag "-fno-sanitize-address-poison-custom-array-cookie" +// is a NOP (because this is the default) and finally, +// 4) w/ASan AND "-fsanitize-address-poison-custom-array-cookie", the +// array cookie location is NOT writable (ASAN successfully stopped it) // -// XFAIL: arm +// RUN: %clangxx %s -o %t && %run %t 2>&1 | FileCheck %s +// RUN: %clangxx_asan %s -o %t && %run %t 2>&1 | FileCheck %s +// RUN: %clangxx_asan -fno-sanitize-address-poison-custom-array-cookie %s -o %t && %run %t 2>&1 | FileCheck %s +// RUN: %clangxx_asan -fsanitize-address-poison-custom-array-cookie %s -o %t && not %run %t 2>&1 | FileCheck %s --check-prefix=COOKIE -// UNSUPPORTED: ios - -#include -#include -#include -#include #include +#include + struct Foo { void *operator new(size_t s) { return Allocate(s); } void *operator new[] (size_t s) { return Allocate(s); } @@ -22,7 +28,6 @@ } }; -Foo::~Foo() {} void *Foo::allocated; Foo *getFoo(size_t n) { @@ -33,8 +38,12 @@ Foo *foo = getFoo(10); fprintf(stderr, "foo : %p\n", foo); fprintf(stderr, "alloc: %p\n", Foo::allocated); - assert(reinterpret_cast(foo) == - reinterpret_cast(Foo::allocated) + sizeof(void*)); - *reinterpret_cast(Foo::allocated) = 42; + reinterpret_cast(foo)[-1] = 42; +// COOKIE: AddressSanitizer: heap-buffer-overflow +// COOKIE: in main {{.*}}new_array_cookie_with_new_from_class.cpp:[[@LINE-2]] +// COOKIE: is located {{7 bytes inside of 18|15 bytes inside of 26}}-byte region + printf("Unsurprisingly, we were able to write to the array cookie\n"); +// CHECK: Unsurprisingly, we were able to write to the array cookie + return 0; }