[asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[rsundahl]

Summary

Hooks into the address sanitizer that support array cookie poisoning and
validation were being generated for x86_64 but not for ARM. (amended)

In addition to the ItaniumCXXABI array cookie of a single size_t element
containing the number of elements in the allocated array, the ARMCXXABI adds
a second size_t element containing the sizeof(element). This difference in
cookie size created the need to override the methods ::InitializeArrayCookie()
and ::readArrayCookieImpl(). Later, in support of ASAN poison array cookies,
calls to asan_poison_cxx_array_cookie() and asan_load_cxx_array_cookie()
were added to each method respectively. However, these "hooks" were only
implemented for the ItaniumCXXABI. This commit adds the same functionality
to the overridden ARMCXXABI methods.

rdar://92765369

[rsundahl]

Adding ASAN test "new_array_cookie_with_new_from_class" (rdar://92884511)

This test "new_array_cookie_with_new_from_class " was dependent on
the cookie size on x86_64 and was either unsupported or expected to
fail on arm. Those restrictions can be removed with this revision.

[delcypher]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2253

Why is there a restriction on the address space?

2257

This comment is confusing. What's returning 0? __asan_load_cxx_array_cookie doesn't do that and AFAICT neither does this code.

2258

I also don't understand what you mean by the comment. Could you elaborate?

[rsundahl]

Adding Vitaly Buka and Kostya Serebryany (sanitizer maintainers).

[yln]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2263

This code is very similar to what we have in ItaniumCXXABI::InitializeArrayCookie(). Can we try to extract a local handleASanArrayCookiePoisoning() helper?

2264

Same here: extract common helper for the ASan stuff to be shared by ARM and ItaniumCXXABI.

compiler-rt/lib/asan/asan_poisoning.cpp

262–265

Nitpicking extreme:

• I think the code inside #if shouldn't have extra indent (because preprocessor sections don't establish code). If in doubt, let clang-format do it for you.
• Let's move the comment inside the #if, just above before the line of code. If you ever read the pre-processed source-code, then the comment "lives and dies" with the line of code it relates too, i.e, on x86, currently there would be a comment without the code.

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp

3

❤ The boy scouts rule:

Always leave the campground cleaner than you found it.

19–20

Can we try to simulate a 1-byte buffer-underflow (and leave the definition of struct C as-is)? This would show that ASan still reports the smallest possible infraction.

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp

19–20

Can we try to simulate a 1-byte buffer-underflow (and leave the definition of struct C as-is)? This would show that ASan still reports the smallest possible infraction.

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp

5

sanitizer -> ASan

16–17

Avoid NOT in FileCheck prefix names, because -NOT is the directive to do a "negative match", that is, "make sure this text does not appear".

How about:
--check-prefix=COOKIE for the cookie case and just skip --check-prefix for the "no cookie" (it's the default after all)

35

Why remove this debug output? It might be useful...

43–47

[yln]

compiler-rt/lib/asan/asan_poisoning.cpp

262

[yln]

compiler-rt/lib/asan/asan_poisoning.cpp

262

I find this a bit confusing

• x86_64: cookie is 1 word and passed p points to it
• arm64: cookie is 2 words and passed p points to second half of it

Would it be worth to take the extra care in CodeGen to always pass the "beginning of the cookie" to __asan_poison_cxx_array_cookie() and then have something like that:

size_t shadow_cookie_size = SANITIZER_ARM64 ? 2 : 1:
internal_memset(s, kAsanArrayCookieMagic, shadow_cookie_size);

[rsundahl]

Adding dialog to comments made by reviewers.

clang/lib/CodeGen/ItaniumCXXABI.cpp

2253

It's only there because it's also there in ItaniumCXXABI::InitializeArrayCookie() method and I have no reason to think that the ARMCXXABI version would be different. That said, this is the revision that introduced the notion originally: https://reviews.llvm.org/D5111

2257

(Same answer) It's only there because it's also there in ItaniumCXXABI::InitializeArrayCookie() method and I have no reason to think that the ARMCXXABI version would be different. That said, this is the revision that introduced the notion originally: https://reviews.llvm.org/D5111

2258

(Same answer) It's only there because it's also there in ItaniumCXXABI::InitializeArrayCookie() method and I have no reason to think that the ARMCXXABI version would be different. That said, this is the revision that introduced the notion originally: https://reviews.llvm.org/D5111

2264

Was limiting my impact but yes, I agree with you and thanks for expecting it. FYI and FWIW: the same concern that @delcypher has in the preceding comments will still exist because the factored code will come from the existing Itanium methods and those concerns come from that existing code that was introduced by: https://reviews.llvm.org/D5111.

compiler-rt/lib/asan/asan_poisoning.cpp

262

I don't think so for a collection of reasons. When the ItaniumCXXABI defines the "array cookie" it states: "The cookie will have size sizeof(size_t)" and it then describes how there may be padding preceding the cookie but makes no mention of a second element anywhere. (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#array-cookies). Now, while the ARMCXXABI(32) specification defines the cookie as two 4-byte (int) values (sizeof(element),numElements), the ARMCXXABI(64) (https://github.com/ARM-software/abi-aa/blob/320a56971fdcba282b7001cf4b84abb4fd993131/cppabi64/cppabi64.rst#the-generic-c-abi) does no such similar thing and defers to the Itanium specification of a single, type size_t (8 bytes) cookie which contains only (numElements). The initial commit creating the ARMCXXABI(64) mimicking the ARMCXXABI(32) "pair" of values occurred here: https://marc.info/?l=cfe-commits&m=135915714232578&w=2 and is then (unfortunately) this interpretation was reiterated here: https://developer.apple.com/documentation/xcode/writing-arm64-code-for-apple-platforms
which (imo incorrectly) states:
"The ABI provides a fixed layout of two size_t words for array cookies, with no extra alignment requirements. This behavior matches the ARM 32-bit C++ ABI." However... there is no supporting documentation for this two-element cookie in the ARMCXXABI(64). Furthermore, throughout the llvm source code, including sanitizer, the term "array cookie" ALWAYS means the (size_t) bytes immediately preceding the array pointer that is returned by the allocator. For these reasons, I have chosen to treat the "array cookie" uniformly as a size_t singleton and the additional sizeof(element) as the padding that it is, but with the additional information. (There is no use in llvm of the second element either in the Itanium nor the ARM ABIs including the 32 bit ARM ABI. I'm confident that we should support only the defined array cookie as a single value but also fill the padding with sizeof(element) and deprecate the whole idea eventually.)

262–265

Good stuff. Please nit-pick all you like. Will update.

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp

3

I pick up litter on my walks too. ;-)

19–20

That's a good idea, but it's not about leaving the struct the same because it's definitely wrong in that type "int" has nothing to do with the cookie and it only happens to be true that (buffer[-2].x) gets to the beginning of the cookie since two "int"s happen to fit inside of a "size_t". (This may not always be true as in most 64-on-32 type arrangements) It's better to treat the cookie as [-1] of an array of size_t elements so that when indexing [-1] you get all of the cookie at once (see: https://itanium-cxx-abi.github.io/cxx-abi/abi.html#array-cookies). The reason I write to the *whole* cookie at once is because of endian-ness. The existing code which only writes to the first half of the cookie would reach a different half of the cookie on a big-endian machine so this covers both. But you have a good point, we should see if the minimum underflow is caught so I'll think of a way to do that in an endian-agnostic way...

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp

19–20

(Same answer as above)

[rsundahl]

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp

19–20

I believe that because the existing code:

buffer[-1].x = 10

uses a negative address from an array of int, I took that as something unchangeable and if it were, my arguments for making the size of the elements size_t have some context, But...now I see that unjustified and merely clever in some way and your approach explicitly separating the type of the elements of the allocation from the overflow is better in at least two ways. First, it removes any such "cleverness" from the test (which is really more distracting than it is useful) and second, it allows us to test the smallest possible ingress into the cookie area. Sorry that was a hard sell but appreciate the insight.

buffer[-1].x = 10

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp

19–20

In this situation the UAF isn't caught until the corrupted cookie has called delete on all 42 (supposed) elements of the array. The test depends on seeing all 42 members' being deleted so the write must go the low-byte. If you place the 42 (or anything non-zero) in the high byte, the cookie becomes > 2^54 with a corresponding number of calls to delete each member. If all the deletes are not allowed to execute, the actual UAF detection does not get executed. Therefore I'm going to leave this one as it is and leave the "minimum-underflow" to new_array_cookie_test. (The test explicitly disables sanitizing the underflow to the cookie but what is not clear to me is why access to the cookie by the second "delete" after the free doesn't trap the read of the poisoned cookie before deleting the (supposed) members. We might want to instrument ASan to check for a poisoned cookie earlier.)

[rsundahl]

Implement suggestions from reviews. (Incremental update.)

[rsundahl]

Revert ItaniumCXXABI.cpp for now (unintentional push)

[rsundahl]

Update diff back to dc7e02d4b4dc30d44ddebd832719a6e63396e718

[yln]

compiler-rt/lib/asan/asan_poisoning.cpp

263

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp

19–20

C *buffer = new C[argc];

Although this looks weird, I think this is to prevent the compiler from reasoning about the array size.
I know that we had tests in the past that were "silently passing", because the compiler optimized away parts of the test.

I am not sure if it applies here, but let's keep it just in case ...

20

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp

4–15

Thanks for improving the test like this! This is now better than "just a test", it's essentially "executable documentation"! :)

36–38

Is the reason that we had to remove this assert (and change how we overwrite the cookie) that on arm the cookie is 2 words?

Can we do the following?

int cooke_words = <preprocessor for "is ARM?"> : 2 : 1;
assert(reinterpret_cast<uintptr_t>(foo) ==
         reinterpret_cast<uintptr_t>(Foo::allocated) + cookie_words * sizeof(void*));

[rsundahl]

• Refactor InitializeArrayCookie and readArrayCookieImpl.

This update to the differential implements the final suggestions
for refactoring ItaniumCXXABI to remove duplicated code in the
function InitializeArrayCookie() and readArrayCookieImpl().

[rsundahl]

The update completes the suggested changes. The generated code is slightly different around initialization of the array cookie due to choosing one implementation over another when I "folded" ARMCXXABI::InitializeArrayCookie() into ItaniumCXXABI::InitializeArrayCookie(). The generated code LGTM but I would appreciate added scrutiny for the ItaniumCXXABI.cpp changes. check-sanitizer and check-asan completed successfully on x86_64, arm64 and arm64e.

clang/lib/CodeGen/ItaniumCXXABI.cpp

2257

I'm not sure where to find a definitive spec for __asan_load_cxx_array_cookie() so I'm conservatively leaving it as it currently is.

[yln]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2180

Variable names should start with uppercase:
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Please also be conservative with "rename cleanups" in reviews. The smaller your patch is, the easier it is to review. You can do these in a follow-up NFC commit.

compiler-rt/lib/asan/asan_poisoning.cpp

266

I just realized that we are poisoning the cookie, but we are not doing anything on the detection side.
Is this actually required to make detection work/the tests pass?

If this bit isn't needed to make the existing tests pass, then I would like to suggest 1 of 2 things:

• Remove the additional poisoning (recommended), -or-
• Add another (ARM-specific?) test that requires poisoning of the second cookie word. I think this would require adaptation of __asan_load_cxx_array_cookie as well.

[rsundahl]

This update corrects merge conflicts in Build #104091

[rsundahl]

clang/lib/CodeGen/ItaniumCXXABI.cpp

2190

NumElementsPtr.getRawPointer(CGF) had to be changed to CGF.Builder.CreateCall(F, NumElementsPtr.getPointer() but likely will have to be switched back when it appears downstream.

2227

NumElementsPtr.getRawPointer(CGF) had to be changed to CGF.Builder.CreateCall(F, NumElementsPtr.getPointer() but likely will have to be switched back when it appears downstream.

[rsundahl]

Fix (and cleanup) for failure of CodeGen arm.c check in ci/cd pipeline.

[vitalybuka]

Is this still relevant?
If so, I would recommend to split ItaniumCXXABI from asan changes.

[rsundahl]

In D125195#3979478, @vitalybuka wrote:

Is this still relevant?
If so, I would recommend to split ItaniumCXXABI from asan changes.

I haven't proceeded with this because this would be ABI breaking. I intend to support the (I believe) wrong ARM64 array cookie implementation and add the poisoning and tests re-enabled separately. Thanks for your input @vitalybuka.

clang/lib/CodeGen/ItaniumCXXABI.cpp

public:
   CharUnits getArrayCookieSizeImpl(QualType elementType) override;
   Address InitializeArrayCookie(CodeGenFunction &CGF,
                                 Address NewPtr,
                                 llvm::Value *NumElements,
                                 const CXXNewExpr *expr,
                                 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,
                        bool PerformInit) override;
   void registerGlobalDtor(CodeGenFunction &CGF, const VarDecl &D,
                           llvm::FunctionCallee dtor,
                           llvm::Constant *addr) override;
 
void EmitReturnFromThunk(CodeGenFunction &CGF, RValue RV,
                            QualType ResTy) override;
 
   CharUnits getArrayCookieSizeImpl(QualType elementType) override;
   Address InitializeArrayCookie(CodeGenFunction &CGF,
                                 Address NewPtr,
                                 llvm::Value *NumElements,
                                 const CXXNewExpr *expr,
                                 QualType ElementType) override;
-  llvm::Value *readArrayCookieImpl(CodeGenFunction &CGF, Address allocPtr,
-                                   CharUnits cookieSize) override;
 };
 
 class AppleARM64CXXABI : public ARMCXXABI {
 public:
   AppleARM64CXXABI(CodeGen::CodeGenModule &CGM) : ARMCXXABI(CGM) {
     Use32BitVTableOffsetABI = true;
   }
 

 
 Address ItaniumCXXABI::InitializeArrayCookie(CodeGenFunction &CGF,
                                              Address NewPtr,
                                              llvm::Value *NumElements,
                                              const CXXNewExpr *expr,
                                              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 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);
-
-  // 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 &&
+  // 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);
+
+  // Store the number of elements in the cookie
+  llvm::Instruction *SI = CGF.Builder.CreateStore(NumElements, cookiePtr);
+
+  //  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) {
   // ARM says that the cookie is always:
   //   struct array_cookie {
   //     std::size_t element_size; // element_size != 0
   //     std::size_t element_count;
   //   };
   // But the base ABI doesn't give anything an alignment greater than
   // 8, so we can dismiss this as typical ABI-author blindness to
   // actual language complexity and round up to the element alignment.
   return std::max(CharUnits::fromQuantity(2 * CGM.SizeSizeInBytes),
                   CGM.getContext().getTypeAlignInChars(elementType));
 }
 
 Address ARMCXXABI::InitializeArrayCookie(CodeGenFunction &CGF,
                                          Address newPtr,
                                          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);
-
-  // 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());
+  // 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,
+  CGF.Builder.CreateStore(elementSize, xCookiePtr);
 
-  numElementsPtr = CGF.Builder.CreateElementBitCast(numElementsPtr, CGF.SizeTy);
-  return CGF.Builder.CreateLoad(numElementsPtr);
+  // Return just the numElements portion of the cookie as expected.
+  return cookiePtr;
 }
 
 /*********************** Static local initialization **************************/
 
 static llvm::FunctionCallee getGuardAcquireFn(CodeGenModule &CGM,
                                               llvm::PointerType *GuardPtrTy) {
   // int __cxa_guard_acquire(__guard *guard_object);
   llvm::FunctionType *FTy =

clang/test/CodeGenCXX/arm.cpp

namespace test3 {
   struct A {
     int x;
     ~A();
   };
 
   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];
   }
 
   void b(int n) {
     // CHECK-LABEL: define{{.*}} void @_ZN5test31bEi(
     // CHECK: [[N:%.*]] = load i32, i32*
     // CHECK: @llvm.umul.with.overflow.i32(i32 [[N]], i32 4)
     // CHECK: @llvm.uadd.with.overflow.i32(i32 {{.*}}, i32 8)
     // 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];
   }
 
   void d(int n) {
     // CHECK-LABEL: define{{.*}} void @_ZN5test31dEi(
     // CHECK: [[N:%.*]] = load i32, i32*
     // CHECK: @llvm.umul.with.overflow.i32(i32 [[N]], i32 80)
     // CHECK: [[NE:%.*]] = mul i32 [[N]], 20
     // 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];
   }
 
   void e(A *x) {
     // CHECK-LABEL: define{{.*}} void @_ZN5test31eEPNS_1AE(
     // CHECK: icmp eq {{.*}}, null
     // CHECK: getelementptr {{.*}}, i32 -8
     // CHECK: getelementptr {{.*}}, i32 4
namespace test4 {
   struct A {
     int x;
     void operator delete[](void *, size_t sz);
   };
 
   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];
   }
 
   void b(int n) {
     // CHECK-LABEL: define{{.*}} void @_ZN5test41bEi(
     // CHECK: [[N:%.*]] = load i32, i32*
     // CHECK: @llvm.umul.with.overflow.i32(i32 [[N]], i32 4)
     // 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];
   }
 
   void d(int n) {
     // CHECK-LABEL: define{{.*}} void @_ZN5test41dEi(
     // CHECK: [[N:%.*]] = load i32, i32*
     // CHECK: @llvm.umul.with.overflow.i32(i32 [[N]], i32 80)
     // CHECK: [[NE:%.*]] = mul i32 [[N]], 20
     // 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];
   }
 
   void e(A *x) {
     // CHECK-LABEL: define{{.*}} void @_ZN5test41eEPNS_1AE(
     // CHECK: [[ALLOC:%.*]] = getelementptr inbounds {{.*}}, i32 -8
     // CHECK: getelementptr inbounds {{.*}}, i32 4
     // CHECK: bitcast

 // CHECK-NEXT: [[O0:%.*]] = extractvalue { i32, i1 } [[T0]], 1
 // CHECK-NEXT: [[T1:%.*]] = extractvalue { i32, i1 } [[T0]], 0
 // CHECK-NEXT: [[T2:%.*]] = call { i32, i1 } @llvm.uadd.with.overflow.i32(i32 [[T1]], i32 16)
 // CHECK-NEXT: [[O1:%.*]] = extractvalue { i32, i1 } [[T2]], 1
 // CHECK-NEXT: [[OVERFLOW:%.*]] = or i1 [[O0]], [[O1]]
 // 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) {
     delete[] array;
   }
 // CHECK-LABEL:    define{{.*}} void @_ZN5test910testDeleteEPNS_1AE(
 // CHECK:      [[BEGIN:%.*]] = load [[TEST9]]*, [[TEST9]]**
 // CHECK-NEXT: [[T0:%.*]] = icmp eq [[TEST9]]* [[BEGIN]], null
 // 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]]
 // CHECK-NEXT: [[T0:%.*]] = icmp eq [[TEST9]]* [[BEGIN]], [[END]]
 // CHECK-NEXT: br i1 [[T0]],
 //   Array deallocation follows.
 }
 
   // CHECK: define linkonce_odr [[C:%.*]]* @_ZTv0_n12_N5test21CD1Ev(
   // CHECK:   call [[C]]* @_ZN5test21CD1Ev(
   // CHECK:   ret [[C]]* undef
 
   // CHECK-LABEL: define linkonce_odr void @_ZTv0_n12_N5test21CD0Ev(
   // CHECK:   call void @_ZN5test21CD0Ev(
   // CHECK:   ret void

compiler-rt/lib/asan/asan_poisoning.cpp

 }
 
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE
 void __asan_poison_cxx_array_cookie(uptr p) {
   if (SANITIZER_WORDSIZE != 64) return;
   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
 uptr __asan_load_cxx_array_cookie(uptr *p) {
   if (SANITIZER_WORDSIZE != 64) return *p;
   if (!flags()->poison_array_cookie) return *p;
   uptr s = MEM_TO_SHADOW(reinterpret_cast<uptr>(p));
   u8 sval = *reinterpret_cast<u8*>(s);
   if (sval == kAsanArrayCookieMagic) return *p;

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp

 // 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() {
     fprintf(stderr, "ZZZZZZZZ\n");
     exit(1);
   }
 };
 
 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;
 }

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp

 // REQUIRES: asan-64-bits
 // RUN: %clangxx_asan -O3 %s -o %t
 // 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() {
     dtor_counter++;
     fprintf(stderr, "DTOR %d\n", dtor_counter);
   }
 };
 
 __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;
 // COOKIE: DTOR 1
 // COOKIE-NOT: DTOR 2
 // COOKIE: AddressSanitizer: loaded array cookie from free-d memory
 // COOKIE: AddressSanitizer: attempting double-free
 // NO_COOKIE: DTOR 1
 // NO_COOKIE: DTOR 43
 // NO_COOKIE-NOT: DTOR 44
 // NO_COOKIE-NOT: AddressSanitizer: loaded array cookie from free-d memory
 // NO_COOKIE: AddressSanitizer: attempting double-free
 
 }

compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp

-// 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); }
   ~Foo();
   static void *allocated;
   static void *Allocate(size_t s) {
     assert(!allocated);
     return allocated = ::new char[s];
   }
 };
 
-Foo::~Foo() {}
 void *Foo::allocated;
 
 Foo *getFoo(size_t n) {
   return new Foo[n];
 }
 
 int main() {
   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;
 }