[UBSan] Fix incorrect alignment reported when global new returns an o…

clang/lib/CodeGen/CGExprCXX.cpp

@@ -18,6 +18,9 @@
 #include "ConstantEmitter.h"
 #include "TargetInfo.h"
 #include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/Sanitizers.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "llvm/IR/Intrinsics.h"
 
@@ -1736,17 +1739,29 @@ llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) {
       allocator->isReservedGlobalPlacementOperator())
     result = Builder.CreateLaunderInvariantGroup(result);
 
+  // Check what type of constructor call the sanitizer is checking
+  // Different UB can occour with custom overloads of operator new
+  TypeCheckKind checkKind = CodeGenFunction::TCK_ConstructorCall;
+  const TargetInfo &TI = getContext().getTargetInfo();
+  unsigned DefaultTargetAlignment = TI.getNewAlign() / TI.getCharWidth();
+  SourceManager &SM = getContext().getSourceManager();
+  SourceLocation Loc = E->getOperatorNew()->getLocation();
+  bool IsCustomOverload = !SM.isInSystemHeader(Loc);
+  if (SanOpts.has(SanitizerKind::Alignment) && IsCustomOverload &&
+      (DefaultTargetAlignment >
+       CGM.getContext().getTypeAlignInChars(allocType).getQuantity()))
+    checkKind = CodeGenFunction::TCK_ConstructorCallOverloadedNew;
+
   // Emit sanitizer checks for pointer value now, so that in the case of an
   // 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, allocType, result.getAlignment(), SkippedChecks,
-                numElements);
+  EmitTypeCheck(
+      checkKind, E->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(),
+      result, allocType, result.getAlignment(), SkippedChecks, numElements);
 
   EmitNewInitializer(*this, E, allocType, elementTy, result, numElements,
                      allocSizeWithoutCookie);

clang/lib/CodeGen/CodeGenFunction.h

@@ -3292,7 +3292,10 @@ class CodeGenFunction : public CodeGenTypeCache {
     TCK_NonnullAssign,
     /// Checking the operand of a dynamic_cast or a typeid expression.  Must be
     /// null or an object within its lifetime.
-    TCK_DynamicOperation
+    TCK_DynamicOperation,
+    /// Checking the 'this' poiner for a constructor call, including that the
+    /// alignment is greater or equal to the targets minimum alignment
+    TCK_ConstructorCallOverloadedNew
   };
 
   /// Determine whether the pointer type check \p TCK permits null pointers.

compiler-rt/lib/ubsan/ubsan_checks.inc

@@ -28,6 +28,7 @@ UBSAN_CHECK(NullptrAfterNonZeroOffset, "nullptr-after-nonzero-offset",
 UBSAN_CHECK(PointerOverflow, "pointer-overflow", "pointer-overflow")
 UBSAN_CHECK(MisalignedPointerUse, "misaligned-pointer-use", "alignment")
 UBSAN_CHECK(AlignmentAssumption, "alignment-assumption", "alignment")
+UBSAN_CHECK(AlignmentOnOverloadedNew, "alignment-new", "alignment")
 UBSAN_CHECK(InsufficientObjectSize, "insufficient-object-size", "object-size")
 UBSAN_CHECK(SignedIntegerOverflow, "signed-integer-overflow",
             "signed-integer-overflow")

compiler-rt/lib/ubsan/ubsan_handlers.cpp

@@ -73,14 +73,26 @@ enum TypeCheckKind {
   TCK_NonnullAssign,
   /// Checking the operand of a dynamic_cast or a typeid expression.  Must be
   /// null or an object within its lifetime.
-  TCK_DynamicOperation
+  TCK_DynamicOperation,
+  /// Checking the 'this' poiner for a constructor call, including that the
+  /// alignment is greater or equal to the targets minimum alignment
+  TCK_ConstructorCallOverloadedNew
 };
 
 extern const char *const TypeCheckKinds[] = {
-    "load of", "store to", "reference binding to", "member access within",
-    "member call on", "constructor call on", "downcast of", "downcast of",
-    "upcast of", "cast to virtual base of", "_Nonnull binding to",
-    "dynamic operation on"};
+    "load of",
+    "store to",
+    "reference binding to",
+    "member access within",
+    "member call on",
+    "constructor call on",
+    "downcast of",
+    "downcast of",
+    "upcast of",
+    "cast to virtual base of",
+    "_Nonnull binding to",
+    "dynamic operation on",
+    "constructor call with pointer from overloaded operator new on"};
 }
 
 static void handleTypeMismatchImpl(TypeMismatchData *Data, ValueHandle Pointer,
@@ -94,7 +106,9 @@ static void handleTypeMismatchImpl(TypeMismatchData *Data, ValueHandle Pointer,
              ? ErrorType::NullPointerUseWithNullability
              : ErrorType::NullPointerUse;
   else if (Pointer & (Alignment - 1))
-    ET = ErrorType::MisalignedPointerUse;
+    ET = (Data->TypeCheckKind == TCK_ConstructorCallOverloadedNew)
+             ? ErrorType::AlignmentOnOverloadedNew
+             : ErrorType::MisalignedPointerUse;
   else
     ET = ErrorType::InsufficientObjectSize;
 
@@ -117,6 +131,13 @@ static void handleTypeMismatchImpl(TypeMismatchData *Data, ValueHandle Pointer,
     Diag(Loc, DL_Error, ET, "%0 null pointer of type %1")
         << TypeCheckKinds[Data->TypeCheckKind] << Data->Type;
     break;
+  case ErrorType::AlignmentOnOverloadedNew:
+    Diag(Loc, DL_Error, ET,
+         "%0 misaligned address %1 for type %2, "
+         "which requires target minimum assumed alignment of %3")
+        << TypeCheckKinds[Data->TypeCheckKind] << (void *)Pointer << Data->Type
+        << Alignment;
+    break;
   case ErrorType::MisalignedPointerUse:
     Diag(Loc, DL_Error, ET, "%0 misaligned address %1 for type %3, "
                         "which requires %2 byte alignment")

compiler-rt/test/ubsan/TestCases/TypeCheck/minimum-alignment.cpp

@@ -0,0 +1,36 @@
+// RUN: %clangxx %gmlt -fsanitize=alignment %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+// UNSUPPORTED: i386
+// UNSUPPORTED: armv7l
+
+// These sanitizers already overload the new operator so won't compile this test
+// UNSUPPORTED: ubsan-msan
+// UNSUPPORTED: ubsan-tsan
+
+#include <cassert>
+#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);
+  assert(((std::ptrdiff_t)ptr & (__STDCPP_DEFAULT_NEW_ALIGNMENT__ - 1)) == 0);
+
+  return (char *)ptr + offset;
+}
+
+struct Foo {
+  void *_cookie1, *_cookie2;
+};
+
+static_assert(alignof(Foo) == 8);
+int main() {
+  // CHECK: runtime error: constructor call with pointer from overloaded operator new on misaligned address 0x{{.*}} for type 'Foo', which requires target minimum assumed alignment of 16
+  Foo *f = new Foo;
+  return 0;
+}