[UBSan] Fix incorrect alignment reported when global new returns an offset pointer

[belkiss]

Summary

I found that ubsan will report an incorrect alignment for a type in case it is allocated with the global operator new (without alignment), if we have it return an offset ptr.

I wrote a small repro: https://godbolt.org/z/n8Yh8eoaE

The type is aligned on 8 bytes (verified by static_assert on its alignof), but ubsan reports: "constructor call on misaligned address 0x000002af8fd8 for type 'Param', which requires 16 byte alignment".

Now I suppose changing the ptr returned by new that way breaks the __STDCPP_DEFAULT_NEW_ALIGNMENT__, but in the specs in [basic.stc.dynamic.allocation] it says for the non-aligned, non array new:

Otherwise, the storage is aligned for any object that does not have new-extended alignment and is of the requested size

which is pretty vague.

I would either expect to get an error message to indicate that break, or nothing, because in the end the pointer returned by new is 8 bytes aligned, and matches the 8 bytes alignment requirement of the type.

I think the issue comes from this line:

https://github.com/llvm/llvm-project/blob/4f7fb13f87e10bd2cd89ccf2be70b026032237a7/clang/lib/CodeGen/CGExprCXX.cpp#L1737

Instead of the allocator alignment result.getAlignment(), it should be the type alignment allocAlign. I've tried it, and ran the tests, the error goes away and the tests pass.

Fixes https://github.com/llvm/llvm-project/issues/51035

[belkiss]

Disable the test in ubsan-msan and ubsan-tsan since they also override global new, causing link error.

[belkiss]

Squash commits, for some reason update dropped the first commit (first time using arcanist...).

[belkiss]

Clang-format

[belkiss]

Use static_cast/reinterpret_cast instead of C-casts
This will also retry the CI, apparently fuzzer-finalstats.test is flaky

[aganea]

+@hans @rnk Since we discussed this in one of the past Windows/COFF calls.

[hans]

I'm not familiar enough with the code to review it (though it sounds right to me). Looks like it was written by Serge originally.

[rnk]

I think the main thing here is that the diagnostic is incorrect: It claims that the type requires __STDC_DEFAULT_NEW_ALIGNMENT__ (16) byte alignment, but it doesn't, it only requires 8. That's confusing.

The next question is, does LLVM exploit __STDCPP_DEFAULT_NEW_ALIGNMENT__ for optimization purposes? IMO, UBSan should agree with LLVM: it is supposed to aid in the early detection of UB, rather than having to figure that out later after optimizations have run. If LLVM doesn't exploit it, I don't see a need to report an error. After some experimentation, I have convinced myself that LLVM doesn't know or exploit allocator alignment:
https://godbolt.org/z/oo6rzKn3c

Finally, we should ask ourselves, is it worth adding a special diagnostic to check that all operator new calls return aligned pointers? I think the answer is no. Users are more likely to heed sanitizer errors when they point to actual, practical issues rather than hypothetical ones about more highly aligned types allocated at other call sites.

Let me ask @rjmccall for a second opinion, since he wrote the Address alignment-tracking overhaul code.

compiler-rt/test/ubsan/TestCases/TypeCheck/global-new-alignment.cpp

34

I suggest simplifying this to CHECK-NOT: runtime error, since no errors are expected. This is mostly to improve test debuggability anyway, the test will fail if it exits non-zero.

[belkiss]

compiler-rt/test/ubsan/TestCases/TypeCheck/global-new-alignment.cpp

34

I'll remove it altogether from here, since I pass --implicit-check-not="runtime error" to the FileCheck at the top of the file

[belkiss]

Simplify test, remove the inline specific CHECK-NOT since we have the implicit-check-not in FileCheck args checking that no errors have occured in the whole file

[belkiss]

And thanks @aganea, @hans & @rnk!

[rjmccall]

If we want to assume a weaker alignment, we should change the calculation of allocationAlign rather than just changing this point downstream of that computation. But replacements of the standard operator new are restricted and cannot simply choose to return less-aligned memory; at the very least, the test program is not portable.

[rnk]

clang/lib/CodeGen/CGExprCXX.cpp

1658–1659

I believe John's proposal is to remove this max here.

[rjmccall]

clang/lib/CodeGen/CGExprCXX.cpp

1658–1659

Or to at least use something more conservative, like the alignment of a pointer. Or even potentially to fix getNewAlign() so that we're only aggressive about it on targets (like Darwin) that have more explicitly authorized us to be so. But in any case, we should make the change up here instead of only in the UBSan check.

clang/lib/CodeGen/CGExprCXX.cpp

if (allocator->isReservedGlobalPlacementOperator()) {
     // not take an alignment argument, the allocator is known to produce
     // storage that's suitably aligned for any object that fits, up to a known
     // threshold. Otherwise assume it's suitably aligned for the allocated type.
     CharUnits allocationAlign = allocAlign;
     if (!E->passAlignment() &&
         allocator->isReplaceableGlobalAllocationFunction()) {
       unsigned AllocatorAlign = llvm::PowerOf2Floor(std::min<uint64_t>(
           Target.getNewAlign(), getContext().getTypeSize(allocType)));
       allocationAlign = std::max(
           allocationAlign, getContext().toCharUnitsFromBits(AllocatorAlign));
     }
 
     allocation = Address(RV.getScalarVal(), Int8Ty, allocationAlign);
   }
 
   // Emit a null check on the allocation result if the allocation
   // function is allowed to return null (because it has a non-throwing
   // exception spec or is the reserved placement new) and we have an
llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) {
   // array it was checked only once and not at each constructor call. We may
   // have already checked that the pointer is non-null.
   // FIXME: If we have an array cookie and a potentially-throwing allocator,
   // we'll null check the wrong pointer here.
   SanitizerSet SkippedChecks;
   SkippedChecks.set(SanitizerKind::Null, nullCheck);
   EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall,
                 E->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(),
-                result.getPointer(), allocType, result.getAlignment(),
-                SkippedChecks, numElements);
+                result.getPointer(), allocType, allocAlign, SkippedChecks,
+                numElements);
 
   EmitNewInitializer(*this, E, allocType, elementTy, result, numElements,
                      allocSizeWithoutCookie);
   if (E->isArray()) {
     // NewPtr is a pointer to the base element type.  If we're
     // allocating an array of arrays, we'll need to cast back to the
     // array pointer type.
     llvm::Type *resultType = ConvertTypeForMem(E->getType());

compiler-rt/test/ubsan/TestCases/TypeCheck/global-new-alignment.cpp

+// RUN: %clangxx -fsanitize=alignment %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --implicit-check-not="runtime error" -allow-empty
+// Disable with msan and tsan because they also override global new
+// UNSUPPORTED: ubsan-msan, ubsan-tsan
+
+#include <cassert>
+#include <cstddef>
+#include <cstdlib>
+
+void *operator new(std::size_t count) {
+  constexpr const size_t offset = 8;
+
+  // allocate a bit more so we can safely offset it
+  void *ptr = std::malloc(count + offset);
+
+  // verify malloc returned 16 bytes aligned mem
+  static_assert(__STDCPP_DEFAULT_NEW_ALIGNMENT__ == 16,
+                "Global new doesn't return 16 bytes aligned memory!");
+  assert((reinterpret_cast<std::ptrdiff_t>(ptr) &
+          (__STDCPP_DEFAULT_NEW_ALIGNMENT__ - 1)) == 0);
+
+  return static_cast<char *>(ptr) + offset;
+}
+
+struct Param {
+  void *_cookie1;
+  void *_cookie2;
+};
+
+static_assert(alignof(Param) == 8, "Param struct alignment must be 8 bytes!");
+
+int main() { Param *p = new Param; }