[UBSAN] add null and alignment checks for aggregates #164548

vasu-the-sharma · 2025-10-22T05:16:02Z

Description

This PR adds sanitizer checks for null pointers and misalignment during aggregate copy operations in EmitAggregateCopy.

Current State

When copying aggregates (structs), the compiler does not validate that source and destination pointers are non-null or properly aligned when sanitizers are enabled. This allows undefined behavior from invalid pointers to go undetected at runtime.

Solution

When null or alignment sanitizers are active, this change:

Retrieves raw pointer addresses from both source and destination LValues
Performs EmitTypeCheck on both pointers to verify they are non-null and meet alignment requirements
Records these checks in the SanitizerSet for runtime emission

This ensures memory safety violations are caught early, preventing crashes and corruption from invalid aggregate copies.

Testing

Added comprehensive test cases covering:

Null source pointer detection
Null destination pointer detection
Misaligned source pointer detection
Misaligned destination pointer detection
Normal copy operations (no false positives)

Screenshots

github-actions · 2025-10-22T05:16:27Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-10-22T05:17:07Z

@llvm/pr-subscribers-compiler-rt-sanitizer
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: VASU SHARMA (vasu-the-sharma)

Changes

This PR adds null and alignment checks for aggregates like scalars

Full diff: https://github.com/llvm/llvm-project/pull/164548.diff

1 Files Affected:

(modified) clang/lib/CodeGen/CGExprAgg.cpp (+15)

diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index eee397f1f3d19..de6d80a273dbd 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -2249,6 +2249,21 @@ void CodeGenFunction::EmitAggregateCopy(LValue Dest, LValue Src, QualType Ty,
                                         bool isVolatile) {
   assert(!Ty->isAnyComplexType() && "Shouldn't happen for complex");
 
+  if (SanOpts.hasOneOf(SanitizerKind::Null | SanitizerKind::Alignment)) {
+    Address SrcAddr = Src.getAddress();
+    Address DestAddr = Dest.getAddress();
+
+    // Check source pointer for null and alignment violations
+    EmitTypeCheck(TCK_Load, SourceLocation(),
+                  SrcAddr.emitRawPointer(*this), Ty, SrcAddr.getAlignment(),
+                  SanitizerSet());
+
+    // Check destination pointer for null and alignment violations
+    EmitTypeCheck(TCK_Store, SourceLocation(),
+                  DestAddr.emitRawPointer(*this), Ty, DestAddr.getAlignment(),
+                  SanitizerSet());
+  }
+
   Address DestPtr = Dest.getAddress();
   Address SrcPtr = Src.getAddress();

…gates

github-actions · 2025-10-22T07:10:24Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions cpp -- clang/test/CodeGen/ubsan-aggregate-null-align.cpp clang/lib/CodeGen/CGExprAgg.cpp --diff_from_common_commit

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.

diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index cf215da08..df15be955 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -2251,19 +2251,18 @@ void CodeGenFunction::EmitAggregateCopy(LValue Dest, LValue Src, QualType Ty,
 
   // Sanitizer checks to verify source and destination pointers are
   // non-null and properly aligned before copying.
-  // Without these checks, undefined behavior from invalid pointers goes undetected.
+  // Without these checks, undefined behavior from invalid pointers goes
+  // undetected.
   Address SrcAddr = Src.getAddress();
   Address DestAddr = Dest.getAddress();
 
   // Check source pointer for null and alignment violations
-  EmitTypeCheck(TCK_Load, SourceLocation(),
-                SrcAddr.emitRawPointer(*this), Ty, SrcAddr.getAlignment(),
-                SanitizerSet());
+  EmitTypeCheck(TCK_Load, SourceLocation(), SrcAddr.emitRawPointer(*this), Ty,
+                SrcAddr.getAlignment(), SanitizerSet());
 
   // Check destination pointer for null and alignment violations
-  EmitTypeCheck(TCK_Store, SourceLocation(),
-                DestAddr.emitRawPointer(*this), Ty, DestAddr.getAlignment(),
-                SanitizerSet());
+  EmitTypeCheck(TCK_Store, SourceLocation(), DestAddr.emitRawPointer(*this), Ty,
+                DestAddr.getAlignment(), SanitizerSet());
 
   Address DestPtr = Dest.getAddress();
   Address SrcPtr = Src.getAddress();

tonykuttai

Please add a test support this change

clang/lib/CodeGen/CGExprAgg.cpp

…gates

hubert-reinterpretcast

I believe there should be a Clang CodeGen test for this.

hubert-reinterpretcast · 2025-10-23T18:46:45Z

clang/lib/CodeGen/CGExprAgg.cpp

+    Address DestAddr = Dest.getAddress();
+
+    // Check source pointer for null and alignment violations
+    EmitTypeCheck(TCK_Load, SourceLocation(),


I think the scope should be expanded to other cases covered by EmitCheckedLValue.

Thanks for the suggestion @hubert-reinterpretcast
I've reviewed the two EmitCheckedLValue usage sites in CGExprAgg.cpp:

Line 802 (VisitCastExpr): Uses EmitCheckedLValue with TCK_Load for dynamic_cast operations
Line 1313 (VisitBinAssign): Uses EmitCheckedLValue with TCK_Store, then calls EmitCopy which delegates to EmitAggregateCopy

Both cases are already covered:
EmitCheckedLValue performs type checking on the LValue expression itself
My changes to EmitAggregateCopy add sanitizer checks at the actual copy operation (the memcpy call)

These checks are complementary rather than redundant:
EmitCheckedLValue: Validates the expression evaluation produces a valid LValue
EmitAggregateCopy: Validates the source and destination pointers during the memory copy operation

The EmitAggregateCopy checks catch cases where pointers might become invalid between LValue emission and the actual copy (like array indexing or pointer arithmetic).
Do you see other specific cases in EmitCheckedLValue usage that would benefit from additional instrumentation?

EmitCheckedLvalue will consider the expression to be accessed (and therefore flag past-the-end array elements).

Consider:

#if !SCALAR typedef struct A { int x; } A; #else typedef int A; #endif A arr[1]; A *ap = arr; int main(void) { A a = { 0 }; arr[1] = a; }

It looks like EmitAggregateCopy may be too "low-level" for all the necessary information to be available.

Consider changes like those from vasu-the-sharma#1.
I have not updated and checked the tests in detail though (and some additional test coverage will probably be needed).

I suggest adding the following test matrix (potentially using macros to increase reuse of CHECK lines; refer to the use of --check-prefixes in the aforementioned PR).

Note that there is, at this time, a general issue to how EmitCheckedLValue works such that it does not "see through" enough AST nodes to identify the array access case in C++ mode.

Type:

plain

(for C only) atomic LHS

(for C only) volatile LHS

Operand form:

arr[idx]

*ap

Operation:

assignment

lhs = rhs

(C++ only) lhs.operator=(rhs)

(C++ only) ap->operator=(rhs)

initialization

A a = rhs;

(C++ only) A a( rhs );

(C++ only) A a{ rhs };

(C++ only) A a = { rhs };

(C++ only) new A( rhs )

(C++ only) new A{ rhs }

(C++ only) A( rhs )

(C++ only) A{ rhs }

(C++ only) static_cast<A>( rhs )

(C++ only) (A)rhs

A a[] = { rhs };

A a[] = { [0] = rhs }; (C extension in C++ mode)

(C++ only) struct AA { A a; }; (void)new AA{ rhs };

(C++ only) struct AA : virtual A { AA() : A( rhs ) { } } aa;

lvalue-to-rvalue conversion

(C only) rhs;

extern void f(int, ...); f(0, *ap);

For ease of review, it may make sense to pre-commit the test before this PR (so that the change in this PR will simply be the addition of the sanitizer checks).

clang/lib/CodeGen/CGExprAgg.cpp

vasu-the-sharma · 2025-11-13T12:27:06Z

ping

hubert-reinterpretcast · 2025-11-15T00:09:15Z