[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)

[Vedant Kumar via Phabricator via cfe-commits]

vsk created this revision.

UBSan's alignment check does not detect unaligned loads and stores from
extern struct declarations. Example:

  struct S { int i; };
  extern struct S g_S; // &g_S may not be aligned properly.
  
  int main() {
    return g_S.i; // No alignment check for &g_S.i is emitted.
  }

The frontend skips the alignment check for &g_S.i because the check is
constant-folded to 'NOT NULL' by the IR builder.

If we drop the alignment information from extern struct declarations,
the IR builder would no longer be able to constant-fold the checks away.
That would make the alignment check more useful.

Note: it would still not be able to catch the following case --

  extern int i; // &i is not aligned properly.

PR: https://bugs.llvm.org/show_bug.cgi?id=32630

Testing: check-clang, check-ubsan.


https://reviews.llvm.org/D32456

Files:
  lib/CodeGen/CodeGenModule.cpp
  test/CodeGenCXX/ubsan-global-alignment.cpp


Index: test/CodeGenCXX/ubsan-global-alignment.cpp
===================================================================
--- test/CodeGenCXX/ubsan-global-alignment.cpp
+++ test/CodeGenCXX/ubsan-global-alignment.cpp
@@ -5,18 +5,27 @@
 };
 
 extern S g_S;
+__private_extern__ S pe_S;
 extern S array_S[];
 
-// CHECK-LABEL: define i32 @_Z18load_extern_global
-int load_extern_global() {
-  // FIXME: The IR builder constant-folds the alignment check away to 'true'
-  // here, so we never call the diagnostic. This is PR32630.
-  // CHECK-NOT: ptrtoint i32* {{.*}} to i32, !nosanitize
+// CHECK-LABEL: define i32 @_Z19load_extern_global1
+int load_extern_global1() {
+  // CHECK: br i1 icmp eq (i64 and (i64 ptrtoint (%struct.S* @g_S to i64), i64 3), i64 0), {{.*}}, !nosanitize
+  // CHECK: call void @__ubsan_handle_type_mismatch
   // CHECK: [[I:%.*]] = load i32, i32* getelementptr inbounds (%struct.S, %struct.S* @g_S, i32 0, i32 0), align 4
   // CHECK-NEXT: ret i32 [[I]]
   return g_S.I;
 }
 
+// CHECK-LABEL: define i32 @_Z19load_extern_global2
+int load_extern_global2() {
+  // CHECK: br i1 icmp eq (i64 and (i64 ptrtoint (%struct.S* @pe_S to i64), i64 3), i64 0), {{.*}}, !nosanitize
+  // CHECK: call void @__ubsan_handle_type_mismatch
+  // CHECK: [[I:%.*]] = load i32, i32* getelementptr inbounds (%struct.S, %struct.S* @pe_S, i32 0, i32 0), align 4
+  // CHECK-NEXT: ret i32 [[I]]
+  return pe_S.I;
+}
+
 // CHECK-LABEL: define i32 @_Z22load_from_extern_array
 int load_from_extern_array(int I) {
   // CHECK: [[I:%.*]] = getelementptr inbounds %struct.S, %struct.S* {{.*}}, i32 0, i32 0
Index: lib/CodeGen/CodeGenModule.cpp
===================================================================
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -2312,7 +2312,14 @@
     // handling.
     GV->setConstant(isTypeConstant(D->getType(), false));
 
-    GV->setAlignment(getContext().getDeclAlign(D).getQuantity());
+    if (LangOpts.Sanitize.has(SanitizerKind::Alignment) && !IsForDefinition &&
+        (D->getStorageClass() == SC_Extern ||
+         D->getStorageClass() == SC_PrivateExtern)) {
+      // The alignment sanitizer can catch more bugs if we don't attach
+      // alignment info to extern globals.
+    } else {
+      GV->setAlignment(getContext().getDeclAlign(D).getQuantity());
+    }
 
     setLinkageAndVisibilityForGV(GV, D);
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D32456.96480.patch
Type: text/x-patch
Size: 2376 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170424/4a8254c0/attachment.bin>