rsundahl updated this revision to Diff 430461.
rsundahl added a comment.
Fix (and cleanup) for failure of CodeGen arm.c check in ci/cd pipeline.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125195/new/
https://reviews.llvm.org/D125195
Files:
clang/lib/CodeGen/ItaniumCXXABI.cpp
clang/test/CodeGenCXX/arm.cpp
compiler-rt/lib/asan/asan_poisoning.cpp
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
===================================================================
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp
+++ 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 <new>
-#include <stdlib.h>
-#include <stdint.h>
-#include <stdio.h>
#include <assert.h>
+#include <stdio.h>
+
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<uintptr_t>(foo) ==
- reinterpret_cast<uintptr_t>(Foo::allocated) + sizeof(void*));
- *reinterpret_cast<uintptr_t*>(Foo::allocated) = 42;
+ reinterpret_cast<char*>(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;
}
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
===================================================================
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp
@@ -3,12 +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
-
-#include <stdio.h>
-#include <stdlib.h>
#include <assert.h>
-int dtor_counter;
+#include <stdio.h>
+
+int dtor_counter=0;
struct C {
int x;
~C() {
@@ -19,12 +17,11 @@
__attribute__((noinline)) void Delete(C *c) { delete[] c; }
__attribute__((no_sanitize_address)) void Write42ToCookie(C *c) {
- long *p = reinterpret_cast<long*>(c);
- p[-1] = 42;
+ reinterpret_cast<size_t*>(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;
Index: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
===================================================================
--- compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
+++ compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp
@@ -1,13 +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
-
#include <stdio.h>
#include <stdlib.h>
+
struct C {
int x;
~C() {
@@ -17,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<char*>(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;
}
Index: compiler-rt/lib/asan/asan_poisoning.cpp
===================================================================
--- compiler-rt/lib/asan/asan_poisoning.cpp
+++ 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<u8*>(s) = kAsanArrayCookieMagic;
+#if SANITIZER_ARM64
+ // The ARM64 cookie has a second "size_t" entry so poison it as well
+ *(reinterpret_cast<u8*>(s)-1) = kAsanArrayCookieMagic;
+#endif
}
extern "C" SANITIZER_INTERFACE_ATTRIBUTE
Index: clang/test/CodeGenCXX/arm.cpp
===================================================================
--- clang/test/CodeGenCXX/arm.cpp
+++ clang/test/CodeGenCXX/arm.cpp
@@ -111,8 +111,8 @@
void a() {
// CHECK-LABEL: define{{.*}} void @_ZN5test31aEv()
// CHECK: call noalias nonnull i8* @_Znam(i32 48)
- // CHECK: store i32 4
// CHECK: store i32 10
+ // CHECK: store i32 4
A *x = new A[10];
}
@@ -124,16 +124,16 @@
// CHECK: [[OR:%.*]] = or i1
// CHECK: [[SZ:%.*]] = select i1 [[OR]]
// CHECK: call noalias nonnull i8* @_Znam(i32 [[SZ]])
- // CHECK: store i32 4
// CHECK: store i32 [[N]]
+ // CHECK: store i32 4
A *x = new A[n];
}
void c() {
// CHECK-LABEL: define{{.*}} void @_ZN5test31cEv()
// CHECK: call noalias nonnull i8* @_Znam(i32 808)
- // CHECK: store i32 4
// CHECK: store i32 200
+ // CHECK: store i32 4
A (*x)[20] = new A[10][20];
}
@@ -145,8 +145,8 @@
// CHECK: @llvm.uadd.with.overflow.i32(i32 {{.*}}, i32 8)
// CHECK: [[SZ:%.*]] = select
// CHECK: call noalias nonnull i8* @_Znam(i32 [[SZ]])
- // CHECK: store i32 4
// CHECK: store i32 [[NE]]
+ // CHECK: store i32 4
A (*x)[20] = new A[n][20];
}
@@ -186,8 +186,8 @@
void a() {
// CHECK-LABEL: define{{.*}} void @_ZN5test41aEv()
// CHECK: call noalias nonnull i8* @_Znam(i32 48)
- // CHECK: store i32 4
// CHECK: store i32 10
+ // CHECK: store i32 4
A *x = new A[10];
}
@@ -198,16 +198,16 @@
// CHECK: @llvm.uadd.with.overflow.i32(i32 {{.*}}, i32 8)
// CHECK: [[SZ:%.*]] = select
// CHECK: call noalias nonnull i8* @_Znam(i32 [[SZ]])
- // CHECK: store i32 4
// CHECK: store i32 [[N]]
+ // CHECK: store i32 4
A *x = new A[n];
}
void c() {
// CHECK-LABEL: define{{.*}} void @_ZN5test41cEv()
// CHECK: call noalias nonnull i8* @_Znam(i32 808)
- // CHECK: store i32 4
// CHECK: store i32 200
+ // CHECK: store i32 4
A (*x)[20] = new A[10][20];
}
@@ -219,8 +219,8 @@
// CHECK: @llvm.uadd.with.overflow.i32(i32 {{.*}}, i32 8)
// CHECK: [[SZ:%.*]] = select
// CHECK: call noalias nonnull i8* @_Znam(i32 [[SZ]])
- // CHECK: store i32 4
// CHECK: store i32 [[NE]]
+ // CHECK: store i32 4
A (*x)[20] = new A[n][20];
}
@@ -387,12 +387,14 @@
// CHECK-NEXT: [[T3:%.*]] = extractvalue { i32, i1 } [[T2]], 0
// CHECK-NEXT: [[T4:%.*]] = select i1 [[OVERFLOW]], i32 -1, i32 [[T3]]
// CHECK-NEXT: [[ALLOC:%.*]] = call noalias nonnull i8* @_Znam(i32 [[T4]])
-// CHECK-NEXT: [[T0:%.*]] = bitcast i8* [[ALLOC]] to i32*
-// CHECK-NEXT: store i32 16, i32* [[T0]]
-// CHECK-NEXT: [[T1:%.*]] = getelementptr inbounds i32, i32* [[T0]], i32 1
-// CHECK-NEXT: store i32 [[N]], i32* [[T1]]
-// CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds i8, i8* [[ALLOC]], i32 16
-// CHECK-NEXT: bitcast i8* [[T0]] to [[TEST9]]*
+// CHECK-NEXT: [[BPTR:%.*]] = getelementptr inbounds i8, i8* [[ALLOC]], i32 12
+// CHECK-NEXT: [[NPTR:%.*]] = bitcast i8* [[BPTR]] to i32*
+// CHECK-NEXT: store i32 [[N]], i32* [[NPTR]]
+// CHECK-NEXT: [[BPTR:%.*]] = getelementptr inbounds i8, i8* [[ALLOC]], i32 16
+// CHECK-NEXT: [[WPTR:%.*]] = bitcast i8* [[BPTR]] to i32*
+// CHECK-NEXT: [[SPTR:%.*]] = getelementptr i32, i32* [[WPTR]], i32 -2
+// CHECK-NEXT: store i32 16, i32* [[SPTR]]
+// CHECK-NEXT: bitcast i8* [[BPTR]] to [[TEST9]]*
// Array allocation follows.
void testDelete(A *array) {
@@ -404,7 +406,7 @@
// CHECK-NEXT: br i1 [[T0]],
// CHECK: [[T0:%.*]] = bitcast [[TEST9]]* [[BEGIN]] to i8*
// CHECK-NEXT: [[ALLOC:%.*]] = getelementptr inbounds i8, i8* [[T0]], i32 -16
-// CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds i8, i8* [[ALLOC]], i32 4
+// CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds i8, i8* [[ALLOC]], i32 12
// CHECK-NEXT: [[T1:%.*]] = bitcast i8* [[T0]] to i32*
// CHECK-NEXT: [[N:%.*]] = load i32, i32* [[T1]]
// CHECK-NEXT: [[END:%.*]] = getelementptr inbounds [[TEST9]], [[TEST9]]* [[BEGIN]], i32 [[N]]
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===================================================================
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -317,7 +317,7 @@
QualType ElementType) override;
llvm::Value *readArrayCookieImpl(CodeGenFunction &CGF,
Address allocPtr,
- CharUnits cookieSize) override;
+ CharUnits paddedCookieSize) override;
void EmitGuardedInit(CodeGenFunction &CGF, const VarDecl &D,
llvm::GlobalVariable *DeclPtr,
@@ -448,8 +448,6 @@
llvm::Value *NumElements,
const CXXNewExpr *expr,
QualType ElementType) override;
- llvm::Value *readArrayCookieImpl(CodeGenFunction &CGF, Address allocPtr,
- CharUnits cookieSize) override;
};
class AppleARM64CXXABI : public ARMCXXABI {
@@ -2163,69 +2161,71 @@
QualType ElementType) {
assert(requiresArrayCookie(expr));
- unsigned AS = NewPtr.getAddressSpace();
+ // getArrayCookieSizeImpl() returns the size of a padded cookie
+ CharUnits paddedCookieSize = getArrayCookieSizeImpl(ElementType);
- ASTContext &Ctx = getContext();
- CharUnits SizeSize = CGF.getSizeSize();
+ // The cookie itself is right-justified in the padded cookie
+ Address cookiePtr = NewPtr;
+ CharUnits padSize = paddedCookieSize - CGF.getSizeSize();
+ if (!padSize.isZero())
+ cookiePtr =
+ CGF.Builder.CreateConstInBoundsByteGEP(cookiePtr, padSize);
+ cookiePtr = CGF.Builder.CreateElementBitCast(cookiePtr, CGF.SizeTy);
- // The size of the cookie.
- CharUnits CookieSize =
- std::max(SizeSize, Ctx.getPreferredTypeAlignInChars(ElementType));
- assert(CookieSize == getArrayCookieSizeImpl(ElementType));
+ // Store the number of elements in the cookie
+ llvm::Instruction *SI = CGF.Builder.CreateStore(NumElements, cookiePtr);
- // Compute an offset to the cookie.
- Address CookiePtr = NewPtr;
- CharUnits CookieOffset = CookieSize - SizeSize;
- 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);
- llvm::Instruction *SI = CGF.Builder.CreateStore(NumElements, NumElementsPtr);
-
- // Handle the array cookie specially in ASan.
- if (CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) && AS == 0 &&
+ // Handle ASan array cookie poisoning
+ 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);
+ llvm::FunctionType::get(CGM.VoidTy, cookiePtr.getType(), false);
llvm::FunctionCallee F =
CGM.CreateRuntimeFunction(FTy, "__asan_poison_cxx_array_cookie");
- CGF.Builder.CreateCall(F, NumElementsPtr.getPointer());
+// CGF.Builder.CreateCall(F, cookiePtr.getRawPointer(CGF));
+ CGF.Builder.CreateCall(F, cookiePtr.getPointer());
}
- // Finally, compute a pointer to the actual data buffer by skipping
- // over the cookie completely.
- return CGF.Builder.CreateConstInBoundsByteGEP(NewPtr, CookieSize);
+ // Return the usable userdata immediately after the cookie
+ return CGF.Builder.CreateConstInBoundsByteGEP(NewPtr, paddedCookieSize);
}
llvm::Value *ItaniumCXXABI::readArrayCookieImpl(CodeGenFunction &CGF,
Address allocPtr,
- CharUnits cookieSize) {
- // The element size is right-justified in the cookie.
- Address numElementsPtr = allocPtr;
- CharUnits numElementsOffset = cookieSize - CGF.getSizeSize();
- if (!numElementsOffset.isZero())
- numElementsPtr =
- CGF.Builder.CreateConstInBoundsByteGEP(numElementsPtr, numElementsOffset);
-
- unsigned AS = allocPtr.getAddressSpace();
- numElementsPtr = CGF.Builder.CreateElementBitCast(numElementsPtr, CGF.SizeTy);
- if (!CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) || AS != 0)
- return CGF.Builder.CreateLoad(numElementsPtr);
- // In asan mode emit a function call instead of a regular load and let the
- // run-time deal with it: if the shadow is properly poisoned return the
- // cookie, otherwise return 0 to avoid an infinite loop calling DTORs.
- // We can't simply ignore this load using nosanitize metadata because
- // the metadata may be lost.
+ CharUnits paddedCookieSize) {
+ // The cookie is right-justified in the padded cookie
+ Address cookiePtr = allocPtr;
+ CharUnits padSize = paddedCookieSize - CGF.getSizeSize();
+ if (!padSize.isZero())
+ cookiePtr =
+ CGF.Builder.CreateConstInBoundsByteGEP(cookiePtr, padSize);
+ cookiePtr = CGF.Builder.CreateElementBitCast(cookiePtr, CGF.SizeTy);
+
+ // Return the cookie unless ASan will handle it
+ if (!CGM.getLangOpts().Sanitize.has(SanitizerKind::Address) ||
+ allocPtr.getAddressSpace() != 0)
+ return CGF.Builder.CreateLoad(cookiePtr);
+
+ // In ASan mode we emit a function call to __asan_load_cxx_array_cookie()
+ // rather than doing a regular load. The ASan runtime will check the
+ // validity of the cookie using the shadow memory. If ASan determines that
+ // the cookie is invalid, it will log the invalid load of the the cookie
+ // and then either abort or return 0 depending on the ASan setting. If the
+ // cookie is indeed invalid, it is critical to NOT load it because it will
+ // then be used by delete[] to call the DTORs for each element of the array.
+ // This would result in DTORs being called for invalid elements and create
+ // a severe security issue. Returning 0 effectively avoids invalid DTORs.
llvm::FunctionType *FTy =
llvm::FunctionType::get(CGF.SizeTy, CGF.SizeTy->getPointerTo(0), false);
llvm::FunctionCallee F =
CGM.CreateRuntimeFunction(FTy, "__asan_load_cxx_array_cookie");
- return CGF.Builder.CreateCall(F, numElementsPtr.getPointer());
+//return CGF.Builder.CreateCall(F, cookiePtr.getRawPointer(CGF));
+ return CGF.Builder.CreateCall(F, cookiePtr.getPointer());
}
CharUnits ARMCXXABI::getArrayCookieSizeImpl(QualType elementType) {
@@ -2248,35 +2248,31 @@
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);
-
- // The second element is the element count.
- cookie = CGF.Builder.CreateConstInBoundsGEP(cookie, 1);
- CGF.Builder.CreateStore(numElements, cookie);
-
- // 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);
-}
-
-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);
+ // Do everything Itanium first
+ Address cookiePtr = ItaniumCXXABI::InitializeArrayCookie(CGF, newPtr,
+ numElements,
+ expr, elementType);
+
+ // ARM32 stores a second value *before* numElements which is sizeof(element)
+ // ARM64 doesn't seem to support this extra information but we've been doing
+ // it since ARMCXXABI. We've also maintained a minimum alignment of 16 bytes
+ // from the allocator so there is room for it. (NB: x86)64 doesn't return
+ // 16 byte aligned allocations when there are array cookies like ARM32 does)
+
+ // The padded cookie had better be at least 2*size_t in size
+ assert(ARMCXXABI::getArrayCookieSizeImpl(elementType) >=
+ CharUnits::fromQuantity(2*CGM.SizeSizeInBytes));
+
+ // Add the "extra" sizeof(element) information behind the regular cookie.
+ Address xCookiePtr = CGF.Builder.CreateElementBitCast(cookiePtr, CGF.SizeTy);
+ xCookiePtr = CGF.Builder.CreateConstGEP(xCookiePtr, -2);
+ llvm::Value *elementSize =
+ llvm::ConstantInt::get(CGF.SizeTy,
+ getContext().getTypeSizeInChars(elementType).getQuantity());
+ CGF.Builder.CreateStore(elementSize, xCookiePtr);
+
+ // Return just the numElements portion of the cookie as expected.
+ return cookiePtr;
}
/*********************** Static local initialization **************************/
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
