[clang][UBSan] Passing underaligned pointer as a function argument is undefined behaviour

[lebedev.ri]

Summary

This is somewhat related to the following RFC by @rjmccall:
https://lists.llvm.org/pipermail/llvm-dev/2016-January/094012.html

C 6.3.2.3p7 (N1548) says:

A pointer to an object type may be converted to a pointer to a
different object type. If the resulting pointer is not correctly
aligned) for the referenced type, the behavior is undefined.

C++ [expr.reinterpret.cast]p7 (N4527) defines pointer conversions in terms
of conversions from void*:

An object pointer can be explicitly converted to an object pointer
of a different type. When a prvalue v of object pointer type is
converted to the object pointer type “pointer to cv T”, the result
is static_cast<cv T*>(static_cast<cv void*>(v)).

C++ [expr.static.cast]p13 says of conversions from void*:

A prvalue of type “pointer to cv1 void” can be converted to a
prvalue of type “pointer to cv2 T” .... If the original pointer value
represents the address A of a byte in memory and A satisfies the alignment
requirement of T, then the resulting pointer value represents the same
address as the original pointer value, that is, A. The result of any
other such pointer conversion is unspecified.

Currently clang does not optimize based on the fact that the pointers
passed into the function must be appropriately aligned for their pointee type.
We now have a motivation to change that. Namely, it was estabilished that
not doing so prevents LICM, and it is likely not the only penalized transform.
See D99249.

Now, somehow i don't think we can just start optimizing on that.
Let's first give users tools to weed out the cases where the pointer
passed into function's argument is underaligned.

I've yet to actually see just how much mayhem this causes,
at least on the LLVM stage-2 build.

[lebedev.ri]

Fix usage of clang function argument -> llvm function argument mapping reading.

[rsmith]

This is somewhat related to the following RFC by @rjmccall:
https://lists.llvm.org/pipermail/llvm-dev/2016-January/094012.html

It looks to me like this direction is in conflict with that RFC. In particular, it says:

It is not undefined behavior to create a pointer that is less aligned
than its pointee type. Instead, it is only undefined behavior to
access memory through a pointer that is less aligned than its pointee
type.

... and gives as an example that passing an underaligned pointer to a function that doesn't perform an underaligned access is OK under the proposed rule. Putting this behind a -fstrict-* flag (as this patch does) seems OK to me, but I think enabling it by default on any target would be inconsistent with the RFC and would need broader discussion and a change in policy.

I think it is reasonable to add stricter UBSan checks for underaligned pointers, but that should be available regardless of what properties our implementation happens to treat as defined; part of the point of UBSan is to check for portability issues and to check whether problems would arise as a prerequisite for enabling the corresponding -fstrict flag. So I do not think the checks should be gated behind the -fstrict flag. I don't think this is analogous to -fwrapv, which actually opts into a different set of language rules where integer overflow is defined; this is more like -fstrict-enums, for which we will still sanitize even in cases where we happen to choose to not optimize, or the sanitizer for floating-point division by zero, for which we still sanitize (because it's formally UB) but don't include in -fsanitize=undefined (because our implementation defines it). Generally-speaking, things controlled by -fstrict-* flags are checked by UBSan and in particular by -fsanitize=undefined regardless of whether the -fstrict-* flag is specified, and I think there are some cases where -fsanitize=undefined already diagnoses misalignment that is valid under John's RFC, so adding the new checks to the -fsanitize=undefined sanitizer group (regardless of -fstrict) seems consistent to me.

If we want to add a sanitizer that checks for misaligned pointers existing, instead of checking (as our current alignment sanitizer does) for misaligned accesses, then I think it would be more precise (and add a lot fewer checks) to enforce the language rule as written, that is, check for pointer and reference casts to a more-aligned type, rather than checking function arguments. I assume your intent here is to eventually add LLVM parameter attributes indicating the function argument is correctly-aligned, which is why you're checking on call boundaries?

[nickdesaulniers]

Let's first give users tools to weed out the cases where the pointer
passed into function's argument is underaligned.

Isn't there already -Walign-mismatch in clang? We definitely have 1 unresolved case of that affecting the Linux kernel ATM: https://github.com/ClangBuiltLinux/linux/issues/1328

clang/docs/ReleaseNotes.rst

52

s/was//

[lebedev.ri]

In D100037#2674610, @rsmith wrote:

This is somewhat related to the following RFC by @rjmccall:
https://lists.llvm.org/pipermail/llvm-dev/2016-January/094012.html

It looks to me like this direction is in conflict with that RFC. In particular, it says:

It is not undefined behavior to create a pointer that is less aligned
than its pointee type. Instead, it is only undefined behavior to
access memory through a pointer that is less aligned than its pointee
type.

... and gives as an example that passing an underaligned pointer to a function that doesn't perform an underaligned access is OK under the proposed rule.

Yes. We now have a motivational reason to do so, it wouldn't be "just expose the UB for the sake of miscompiling the program".

Putting this behind a -fstrict-* flag (as this patch does) seems OK to me, but I think enabling it by default on any target would be inconsistent with the RFC and would need broader discussion and a change in policy.

I suppose so.

I think it is reasonable to add stricter UBSan checks for underaligned pointers, but that should be available regardless of what properties our implementation happens to treat as defined; part of the point of UBSan is to check for portability issues and to check whether problems would arise as a prerequisite for enabling the corresponding -fstrict flag. So I do not think the checks should be gated behind the -fstrict flag. I don't think this is analogous to -fwrapv, which actually opts into a different set of language rules where integer overflow is defined; this is more like -fstrict-enums, for which we will still sanitize even in cases where we happen to choose to not optimize, or the sanitizer for floating-point division by zero, for which we still sanitize (because it's formally UB) but don't include in -fsanitize=undefined (because our implementation defines it). Generally-speaking, things controlled by -fstrict-* flags are checked by UBSan and in particular by -fsanitize=undefined regardless of whether the -fstrict-* flag is specified, and I think there are some cases where -fsanitize=undefined already diagnoses misalignment that is valid under John's RFC, so adding the new checks to the -fsanitize=undefined sanitizer group (regardless of -fstrict) seems consistent to me.

Aha. I was also very uneasy about doing the check behind the -fstrict.
Will change it to be unconditional!

If we want to add a sanitizer that checks for misaligned pointers existing, instead of checking (as our current alignment sanitizer does) for misaligned accesses, then I think it would be more precise (and add a lot fewer checks) to enforce the language rule as written, that is, check for pointer and reference casts to a more-aligned type, rather than checking function arguments. I assume your intent here is to eventually add LLVM parameter attributes indicating the function argument is correctly-aligned, which is why you're checking on call boundaries?

It's complicated. I would like this to be more strict, and catch any misaligned pointers, yes.
But, two things:

1. it's going to be much more noisy than only dealing with function arguments :)
2. even if we are okay with that, handling this *just* on the callers side (when the pointer is created) is a wrong solution here. we still need to do this on the callee side. because we can't be sure that the caller will be compiled with sanitizer, but we *know* that the callee will be micompiled by us.

So at most i could handle it in more cases, but this one should stay..
Does this make sense?

In D100037#2674675, @nickdesaulniers wrote:

Let's first give users tools to weed out the cases where the pointer
passed into function's argument is underaligned.

Isn't there already -Walign-mismatch in clang? We definitely have 1 unresolved case of that affecting the Linux kernel ATM: https://github.com/ClangBuiltLinux/linux/issues/1328

Front-end warnings won't really help here.

[lebedev.ri]

Unguard the UBSan check under -fstrict*.

In D100037#2674610, @rsmith wrote:

I assume your intent here is to eventually add LLVM parameter attributes indicating the function argument is correctly-aligned, which is why you're checking on call boundaries?

Yep, D99791.

[rjmccall]

Yes. We now have a motivational reason to do so, it wouldn't be "just expose the UB for the sake of miscompiling the program".

Nobody thinks you're proposing this just for the sake of miscompiling. We understand that there are optimization benefits to having this information. We just don't think you get to have this information in general, because of the de facto rules of the language. Knowing that argument pointers don't alias would also be valuable for optimization, but C doesn't guarantee that; this is not that different, it's just that the language rule is a self-imposed rule of our implementation instead of being written up in a standard.

Are you certain that knowing argument pointer alignment is the only way of achieving this optimization? That's surprising to me because I would expect that most optimizations would also need dereferenceability, which of course you can't get. And if you can see (and hoist) a load to infer dereferenceability then you can also infer alignment from that.

clang/docs/ReleaseNotes.rst

 Some of the major new features and improvements to Clang are listed
 here. Generic improvements to Clang as a whole or to its underlying
 infrastructure are described first, followed by language-specific
 sections with improvements to Clang's support for those languages.
 
 Major New Features
 ------------------
 
-- ...
+- Passing underaligned pointers as function arguments is undefined behaviour.
+  Previously, clang was not making any optimizations based on that,
+  however, there's interest in doing so now.
+  UBSan's ``-fsanitize=alignment`` was enhanced to catch the cases where
+  an underaligned pointer was passed as a function argument,
+  to allow codebases to be cleaned up in preparation for this optimization,
+  to avoid miscompiles.
+
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - ...
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------

 
 - ...
 
 .. _release-notes-ubsan:
 
 Undefined Behavior Sanitizer (UBSan)
 ------------------------------------
 
+- ``-fsanitize=alignment`` was enhanced to catch the cases where
+  an underaligned pointer was passed as a function argument.
+  In the future clang will optimize based on that UB.
+
+
 Core Analysis Improvements
 ==========================
 
 - ...
 
 New Issues Found
 ================
 

clang/lib/CodeGen/CGExpr.cpp

static llvm::Value *emitHash16Bytes(CGBuilderTy &Builder, llvm::Value *Low,
   llvm::Value *A1 = Builder.CreateXor(Builder.CreateLShr(A0, K47), A0);
   llvm::Value *B0 = Builder.CreateMul(Builder.CreateXor(High, A1), KMul);
   llvm::Value *B1 = Builder.CreateXor(Builder.CreateLShr(B0, K47), B0);
   return Builder.CreateMul(B1, KMul);
 }
 
 bool CodeGenFunction::isNullPointerAllowed(TypeCheckKind TCK) {
   return TCK == TCK_DowncastPointer || TCK == TCK_Upcast ||
-         TCK == TCK_UpcastToVirtualBase || TCK == TCK_DynamicOperation;
+         TCK == TCK_UpcastToVirtualBase || TCK == TCK_DynamicOperation ||
+         TCK == TCK_FunctionPointerArgument;
 }
 
 bool CodeGenFunction::isVptrCheckRequired(TypeCheckKind TCK, QualType Ty) {
   CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
   return (RD && RD->hasDefinition() && RD->isDynamicClass()) &&
          (TCK == TCK_MemberAccess || TCK == TCK_MemberCall ||
           TCK == TCK_DowncastPointer || TCK == TCK_DowncastReference ||
           TCK == TCK_UpcastToVirtualBase || TCK == TCK_DynamicOperation);

clang/lib/CodeGen/CodeGenFunction.h

enum TypeCheckKind {
     TCK_Upcast,
     /// Checking the operand of a cast to a virtual base object. Must be an
     /// object within its lifetime.
     TCK_UpcastToVirtualBase,
     /// Checking the value assigned to a _Nonnull pointer. Must not be null.
     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 alignment of the function's pointer argument.
+    TCK_FunctionPointerArgument
   };
 
   /// Determine whether the pointer type check \p TCK permits null pointers.
   static bool isNullPointerAllowed(TypeCheckKind TCK);
 
   /// Determine whether the pointer type check \p TCK requires a vptr check.
   static bool isVptrCheckRequired(TypeCheckKind TCK, QualType Ty);
 

clang/lib/CodeGen/CodeGenFunction.cpp

if (CXXABIThisValue) {
         SkippedChecks.set(SanitizerKind::Null, true);
 
       EmitTypeCheck(
           isa<CXXConstructorDecl>(MD) ? TCK_ConstructorCall : TCK_MemberCall,
           Loc, CXXABIThisValue, ThisTy, CXXABIThisAlignment, SkippedChecks);
     }
   }
 
+  if (SanOpts.has(SanitizerKind::Alignment)) {
+    // If we are optimizing based on the strict pointer alignment rules,
+    // check that each pointer argument is aligned appropriately.
+    for (const VarDecl *VD : Args) {
+      QualType Ty = VD->getType();
+      const auto *PtrTy = Ty->getAs<PointerType>();
+      if (!PtrTy)
+        continue;
+
+      QualType PTy = PtrTy->getPointeeType();
+      if (!PTy->isObjectType())
+        continue;
+
+      SanitizerSet SkippedChecks;
+      SkippedChecks.set(SanitizerKind::Null, true);
+      SkippedChecks.set(SanitizerKind::ObjectSize, true);
+      SkippedChecks.set(SanitizerKind::Vptr, true);
+
+      Address AddrOfArg = LocalDeclMap.find(VD)->second;
+      llvm::Value *Arg = EmitLoadOfScalar(AddrOfArg, /*Volatile=*/false, Ty,
+                                          VD->getLocation());
+      EmitTypeCheck(TCK_FunctionPointerArgument, VD->getLocation(), Arg, PTy,
+                    /*Alignment=*/CharUnits(), SkippedChecks);
+    }
+  }
+
   // If any of the arguments have a variably modified type, make sure to
   // emit the type size.
   for (FunctionArgList::const_iterator i = Args.begin(), e = Args.end();
        i != e; ++i) {
     const VarDecl *VD = *i;
 
     // Dig out the type as written from ParmVarDecls; it's unclear whether
     // the standard (C99 6.9.1p10) requires this, but we're following the

clang/test/CodeGen/catch-function-pointer-argument-alignment.cpp

+// RUN: %clang_cc1                                                       -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call"
+// RUN: %clang_cc1  -fsanitize=alignment -fno-sanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER,CHECK-SANITIZE-UNREACHABLE
+// RUN: %clang_cc1  -fsanitize=alignment -fsanitize-recover=alignment    -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1  -fsanitize=alignment -fsanitize-trap=alignment       -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP,CHECK-SANITIZE-UNREACHABLE
+
+// CHECK-SANITIZE-ANYRECOVER: @[[INT:.*]] = {{.*}} c"'int'\00" }
+// CHECK-SANITIZE-ANYRECOVER: @[[LINE_100_POINTER_ARGUMENT:.*]] = {{.*}}, i32 100, i32 16 }, {{.*}}* @[[INT]], i8 2, i8 12 }
+
+#line 100
+void func(int *data) {
+  // CHECK:                           define{{.*}} void @{{.*}}(i32* %[[DATA:.*]])
+  // CHECK-NEXT:                      [[ENTRY:.*]]:
+  // CHECK-NEXT:                        %[[DATA_ADDR:.*]] = alloca i32*, align 8
+  // CHECK-NEXT:                        store i32* %[[DATA]], i32** %[[DATA_ADDR]], align 8
+  // CHECK-SANITIZE-NEXT:               %[[DATA_ADDR_RELOADED:.*]] = load i32*, i32** %[[DATA_ADDR]], align 8
+  // CHECK-SANITIZE-NEXT:               %[[PTRINT:.*]] = ptrtoint i32* %[[DATA_ADDR_RELOADED]] to i64
+  // CHECK-SANITIZE-NEXT:               %[[MASKEDPTR:.*]] = and i64 %[[PTRINT]], 3
+  // CHECK-SANITIZE-NEXT:               %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0
+  // CHECK-SANITIZE-NEXT:               br i1 %[[MASKCOND]], label %[[CONT:.*]], label %[[HANDLER_TYPE_MISMATCH:[^,]+]],{{.*}} !nosanitize
+
+  // CHECK-SANITIZE:                  [[HANDLER_TYPE_MISMATCH]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT:     call void @__ubsan_handle_type_mismatch_v1_abort(i8* bitcast ({ {{{.*}}}, {{{.*}}}*, {{.*}}, {{.*}} }* @[[LINE_100_POINTER_ARGUMENT]] to i8*), i64 %[[PTRINT]]){{.*}}, !nosanitize
+  // CHECK-SANITIZE-RECOVER-NEXT:       call void @__ubsan_handle_type_mismatch_v1(i8* bitcast ({ {{{.*}}}, {{{.*}}}*, {{.*}}, {{.*}} }* @[[LINE_100_POINTER_ARGUMENT]] to i8*), i64 %[[PTRINT]]){{.*}}, !nosanitize
+  // CHECK-SANITIZE-TRAP-NEXT:          call void @llvm.ubsantrap(i8 22){{.*}}, !nosanitize
+  // CHECK-SANITIZE-UNREACHABLE-NEXT:   unreachable, !nosanitize
+
+  // CHECK-SANITIZE:                  [[CONT]]:
+  // CHECK:                             ret void
+}

compiler-rt/lib/ubsan/ubsan_handlers.cpp

enum TypeCheckKind {
   TCK_Upcast,
   /// Checking the operand of a cast to a virtual base object. Must be an
   /// object within its lifetime.
   TCK_UpcastToVirtualBase,
   /// Checking the value assigned to a _Nonnull pointer. Must not be null.
   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 alignment of the function's pointer argument.
+  TCK_FunctionPointerArgument
 };
 
-const char *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"};
-}
+const char *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",
+                                "function pointer argument has"};
+} // namespace __ubsan
 
 static void handleTypeMismatchImpl(TypeMismatchData *Data, ValueHandle Pointer,
                                    ReportOptions Opts) {
   Location Loc = Data->Loc.acquire();
 
   uptr Alignment = (uptr)1 << Data->LogAlignment;
   ErrorType ET;
   if (!Pointer)

compiler-rt/test/ubsan/TestCases/Pointer/function-pointer-argument.cpp

+// RUN: %clang   -x c   -fsanitize=alignment -O0 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
+// RUN: %clang   -x c   -fsanitize=alignment -O1 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
+// RUN: %clang   -x c   -fsanitize=alignment -O2 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
+// RUN: %clang   -x c   -fsanitize=alignment -O3 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
+
+// RUN: %clang   -x c++ -fsanitize=alignment -O0 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
+// RUN: %clang   -x c++ -fsanitize=alignment -O1 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
+// RUN: %clang   -x c++ -fsanitize=alignment -O2 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
+// RUN: %clang   -x c++ -fsanitize=alignment -O3 %s -o %t && %run %t 2>&1 | FileCheck %s --implicit-check-not=" assumption " --implicit-check-not="note:" --implicit-check-not="error:"
+
+#include <stdlib.h>
+
+struct S {
+  int k;
+} __attribute__((aligned(8192)));
+
+void take_pointer_to_overaligned_struct(struct S *x) {
+}
+
+int main(int argc, char *argv[]) {
+  char *ptr = (char *)malloc(2 * sizeof(struct S));
+
+  take_pointer_to_overaligned_struct((struct S *)(ptr + 1));
+  // CHECK: {{.*}}function-pointer-argument.cpp:[[@LINE-7]]:51: runtime error: function pointer argument has misaligned address {{.*}} for type 'struct S', which requires 8192 byte alignment
+  // CHECK: 0x{{.*}}: note: pointer points here
+
+  free(ptr);
+
+  return 0;
+}