Ubsan minimum assumed alignment #166755
Open
Ubsan minimum assumed alignment #166755
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…new returns incorrectly aligned memory
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-compiler-rt-sanitizer @llvm/pr-subscribers-clang Author: Matthew Nagy (gbMattN) ChangesSecond attempt at landing the error message change Full diff: https://github.com/llvm/llvm-project/pull/166755.diff 7 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index 14d8db32bafc6..9acf3a1817c82 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -17,7 +17,11 @@
#include "CodeGenFunction.h"
#include "ConstantEmitter.h"
#include "TargetInfo.h"
+#include "clang/AST/CharUnits.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"
@@ -1749,6 +1753,20 @@ llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) {
allocator->isReservedGlobalPlacementOperator())
result = Builder.CreateLaunderInvariantGroup(result);
+ // Check the default alignment of the type and why. Users may incorrectly
+ // return misaligned memory from a replaced operator new without knowing
+ // about default alignment.
+ TypeCheckKind checkKind = CodeGenFunction::TCK_ConstructorCall;
+ CharUnits checkAlignment = result.getAlignment();
+ const TargetInfo &TI = getContext().getTargetInfo();
+ unsigned DefaultTargetAlignment = TI.getNewAlign() / TI.getCharWidth();
+ if (SanOpts.has(SanitizerKind::Alignment) &&
+ (DefaultTargetAlignment >
+ CGM.getContext().getTypeAlignInChars(allocType).getQuantity())){
+ checkKind = CodeGenFunction::TCK_ConstructorCallMinimumAlign;
+ checkAlignment = CharUnits::fromQuantity(DefaultTargetAlignment);
+ }
+
// 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.
@@ -1756,10 +1774,9 @@ llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) {
// 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, checkAlignment, SkippedChecks, numElements);
EmitNewInitializer(*this, E, allocType, elementTy, result, numElements,
allocSizeWithoutCookie);
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 8c4c1c8c2dc95..047ca844c79de 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -3296,7 +3296,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_ConstructorCallMinimumAlign
};
/// Determine whether the pointer type check \p TCK permits null pointers.
diff --git a/clang/test/CodeGenCXX/ubsan-new-checks.cpp b/clang/test/CodeGenCXX/ubsan-new-checks.cpp
index 60edd323648ab..c8a20ecfed3b4 100644
--- a/clang/test/CodeGenCXX/ubsan-new-checks.cpp
+++ b/clang/test/CodeGenCXX/ubsan-new-checks.cpp
@@ -75,7 +75,7 @@ S3 *func_07() {
// CHECK-LABEL: define {{.*}} @_Z7func_07v
// CHECK: and i64 %{{.*}}, 31, !nosanitize
// CHECK: icmp eq i64 %{{.*}}, 0, !nosanitize
- // CHECK: and i64 %{{.*}}, 3, !nosanitize
+ // CHECK: and i64 %{{.*}}, 15, !nosanitize
// CHECK: icmp eq i64 %{{.*}}, 0, !nosanitize
// CHECK: ret ptr
return new S3;
@@ -85,7 +85,7 @@ S3 *func_08() {
// CHECK-LABEL: define {{.*}} @_Z7func_08v
// CHECK: and i64 %{{.*}}, 31, !nosanitize
// CHECK: icmp eq i64 %{{.*}}, 0, !nosanitize
- // CHECK: and i64 %{{.*}}, 3, !nosanitize
+ // CHECK: and i64 %{{.*}}, 15, !nosanitize
// CHECK: icmp eq i64 %{{.*}}, 0, !nosanitize
// CHECK: ret ptr
return new S3[10];
diff --git a/compiler-rt/lib/ubsan/ubsan_checks.inc b/compiler-rt/lib/ubsan/ubsan_checks.inc
index b1d09a9024e7e..f8757d781afb8 100644
--- a/compiler-rt/lib/ubsan/ubsan_checks.inc
+++ b/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(MinumumAssumedAlignment, "minimum-assumed-alignment", "alignment")
UBSAN_CHECK(InsufficientObjectSize, "insufficient-object-size", "object-size")
UBSAN_CHECK(SignedIntegerOverflow, "signed-integer-overflow",
"signed-integer-overflow")
diff --git a/compiler-rt/lib/ubsan/ubsan_handlers.cpp b/compiler-rt/lib/ubsan/ubsan_handlers.cpp
index 63319f46734a4..fc6063af4562b 100644
--- a/compiler-rt/lib/ubsan/ubsan_handlers.cpp
+++ b/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_ConstructorCallMinimumAlign
};
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 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_ConstructorCallMinimumAlign)
+ ? ErrorType::MinumumAssumedAlignment
+ : 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::MinumumAssumedAlignment:
+ Diag(Loc, DL_Error, ET,
+ "%0 misaligned address %1 for type %2, "
+ "which requires target minimum assumed %3 byte alignment")
+ << 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")
diff --git a/compiler-rt/test/ubsan/TestCases/TypeCheck/minimum-alignment.cpp b/compiler-rt/test/ubsan/TestCases/TypeCheck/minimum-alignment.cpp
new file mode 100644
index 0000000000000..96e1939ea4363
--- /dev/null
+++ b/compiler-rt/test/ubsan/TestCases/TypeCheck/minimum-alignment.cpp
@@ -0,0 +1,37 @@
+// RUN: %clangxx %gmlt -std=c++17 -m64 -fsanitize=alignment %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+// UNSUPPORTED: i386, i686
+// 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>
+#include <cstddef>
+
+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 operator new on misaligned address 0x{{.*}} for type 'Foo', which requires target minimum assumed 16 byte alignment
+ Foo *f = new Foo;
+ return 0;
+}
diff --git a/compiler-rt/test/ubsan/TestCases/TypeCheck/misaligned.cpp b/compiler-rt/test/ubsan/TestCases/TypeCheck/misaligned.cpp
index e39a0ab4e6589..74d5c9a35754c 100644
--- a/compiler-rt/test/ubsan/TestCases/TypeCheck/misaligned.cpp
+++ b/compiler-rt/test/ubsan/TestCases/TypeCheck/misaligned.cpp
@@ -101,7 +101,7 @@ int main(int, char **argv) {
return s->f() && 0;
case 'n':
- // CHECK-NEW: misaligned.cpp:[[@LINE+4]]{{(:21)?}}: runtime error: constructor call on misaligned address [[PTR:0x[0-9a-f]*]] for type 'S', which requires 4 byte alignment
+ // CHECK-NEW: misaligned.cpp:[[@LINE+4]]{{(:21)?}}: runtime error: constructor call{{( with pointer from operator new)?}} on misaligned address [[PTR:0x[0-9a-f]*]] for type 'S', which requires {{(4|(target minimum assumed (8|(16))))}} byte alignment
// CHECK-NEW-NEXT: [[PTR]]: note: pointer points here
// CHECK-NEW-NEXT: {{^ 00 00 00 01 02 03 04 05}}
// CHECK-NEW-NEXT: {{^ \^}}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
gbMattN
commented
Nov 6, 2025
clang/lib/CodeGen/CGExprCXX.cpp
Outdated
| #include "clang/AST/CharUnits.h" | ||
| #include "clang/Basic/CodeGenOptions.h" | ||
| #include "clang/Basic/Sanitizers.h" | ||
| #include "clang/Basic/SourceLocation.h" |
Contributor
Author
There was a problem hiding this comment.
Aha, some of these headers can probably go now
gbMattN
commented
Nov 6, 2025
Contributor
Author
|
This is the diff from the last patch It looks like the windows tests are failing due to a python type error, which I don't entirely understand. I will make sure its either fixed or confirmed to be a different upstream problem before I merge |
| if (SanOpts.has(SanitizerKind::Alignment) && | ||
| (DefaultTargetAlignment > | ||
| CGM.getContext().getTypeAlignInChars(allocType).getQuantity()) && | ||
| !result.getAlignment().isOne() && |
Collaborator
There was a problem hiding this comment.
Why do we need to check !result.getAlignment().isOne()?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Second attempt at landing the error message change
The previous one had to be reverted because of some test failures when run with -m32 on some build bots, and because I was passing the wrong value as the alignment. Commit 4e32b7b has my changes from the previous patch.