[clang] Correct sanitizer behavior in union FAMs

Clang doesn't have the same behavior as GCC does with union flexible
array members. (Technically, union FAMs are probably not acceptable in
C99 and are an extension of GCC and Clang.)

Both Clang and GCC treat *all* arrays at the end of a structure as FAMs.
GCC does the same with unions. Clang does it for some arrays in unions
(incomplete, '0', and '1'), but not for all. Instead of having this
half-supported feature, sync Clang's behavior with GCC's.

Reviewed By: kees

Differential Revision: https://reviews.llvm.org/D135727

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 3fb780a..739851c 100644 (file)
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -210,20 +210,20 @@ bool Expr::isFlexibleArrayMemberLike(
 
   // For compatibility with existing code, we treat arrays of length 0 or
   // 1 as flexible array members.
-  if (const auto *CAT = Context.getAsConstantArrayType(getType())) {
+  const auto *CAT = Context.getAsConstantArrayType(getType());
+  if (CAT) {
     llvm::APInt Size = CAT->getSize();
 
     // GCC extension, only allowed to represent a FAM.
     if (Size == 0)
       return true;
 
-    // FIXME: While the default -fstrict-flex-arrays=0 permits Size>1 trailing
-    // arrays to be treated as flexible-array-members, we still emit diagnostics
-    // as if they are not. Pending further discussion...
     using FAMKind = LangOptions::StrictFlexArraysLevelKind;
-    if (StrictFlexArraysLevel == FAMKind::ZeroOrIncomplete || Size.uge(2))
+    if (StrictFlexArraysLevel == FAMKind::ZeroOrIncomplete && Size.uge(1))
       return false;
 
+    if (StrictFlexArraysLevel == FAMKind::OneZeroOrIncomplete && Size.uge(2))
+      return false;
   } else if (!Context.getAsIncompleteArrayType(getType()))
     return false;
 
@@ -244,8 +244,13 @@ bool Expr::isFlexibleArrayMemberLike(
   // FIXME: If the base type of the member expr is not FD->getParent(),
   // this should not be treated as a flexible array member access.
   if (const auto *FD = dyn_cast<FieldDecl>(ND)) {
-    if (FD->getParent()->isUnion())
-      return true;
+    // GCC treats an array memeber of a union as an FAM if the size is one or
+    // zero.
+    if (CAT) {
+      llvm::APInt Size = CAT->getSize();
+      if (FD->getParent()->isUnion() && (Size.isZero() || Size.isOne()))
+        return true;
+    }
 
     // Don't consider sizes resulting from macro expansions or template argument
     // substitution to form C89 tail-padded arrays.

diff --git a/clang/test/CodeGen/bounds-checking-fam.c b/clang/test/CodeGen/bounds-checking-fam.c
index 1b4f183..2442e72 100644 (file)
--- a/clang/test/CodeGen/bounds-checking-fam.c
+++ b/clang/test/CodeGen/bounds-checking-fam.c
@@ -9,16 +9,44 @@
 // one-element array as the last member of a structure as an alternative.
 // E.g. https://github.com/python/cpython/issues/84301
 // Suppress such errors with -fstrict-flex-arrays=0.
+
+struct Incomplete {
+  int ignored;
+  int a[];
+};
+struct Zero {
+  int ignored;
+  int a[0];
+};
 struct One {
+  int ignored;
   int a[1];
 };
 struct Two {
+  int ignored;
   int a[2];
 };
 struct Three {
+  int ignored;
   int a[3];
 };
 
+// CHECK-LABEL: define {{.*}} @{{.*}}test_incomplete{{.*}}(
+int test_incomplete(struct Incomplete *p, int i) {
+  // CHECK-STRICT-0-NOT: @__ubsan
+  // CHECK-STRICT-1-NOT: @__ubsan
+  // CHECK-STRICT-2-NOT: @__ubsan
+  return p->a[i] + (p->a)[i];
+}
+
+// CHECK-LABEL: define {{.*}} @{{.*}}test_zero{{.*}}(
+int test_zero(struct Zero *p, int i) {
+  // CHECK-STRICT-0-NOT: @__ubsan
+  // CHECK-STRICT-1-NOT: @__ubsan
+  // CHECK-STRICT-2-NOT: @__ubsan
+  return p->a[i] + (p->a)[i];
+}
+
 // CHECK-LABEL: define {{.*}} @{{.*}}test_one{{.*}}(
 int test_one(struct One *p, int i) {
   // CHECK-STRICT-0-NOT: @__ubsan
@@ -29,7 +57,15 @@ int test_one(struct One *p, int i) {
 
 // CHECK-LABEL: define {{.*}} @{{.*}}test_two{{.*}}(
 int test_two(struct Two *p, int i) {
-  // CHECK-STRICT-0:     call void @__ubsan_handle_out_of_bounds_abort(
+  // CHECK-STRICT-0-NOT: @__ubsan
+  // CHECK-STRICT-1:     call void @__ubsan_handle_out_of_bounds_abort(
+  // CHECK-STRICT-2:     call void @__ubsan_handle_out_of_bounds_abort(
+  return p->a[i] + (p->a)[i];
+}
+
+// CHECK-LABEL: define {{.*}} @{{.*}}test_three{{.*}}(
+int test_three(struct Three *p, int i) {
+  // CHECK-STRICT-0-NOT: @__ubsan
   // CHECK-STRICT-1:     call void @__ubsan_handle_out_of_bounds_abort(
   // CHECK-STRICT-2:     call void @__ubsan_handle_out_of_bounds_abort(
   return p->a[i] + (p->a)[i];
@@ -60,29 +96,21 @@ int test_uzero(union uZero *p, int i) {
 int test_uone(union uOne *p, int i) {
   // CHECK-STRICT-0-NOT: @__ubsan
   // CHECK-STRICT-1-NOT: @__ubsan
-  // CHECK-STRICT-2: @__ubsan
+  // CHECK-STRICT-2:     call void @__ubsan_handle_out_of_bounds_abort(
   return p->a[i] + (p->a)[i];
 }
 
 // CHECK-LABEL: define {{.*}} @{{.*}}test_utwo{{.*}}(
 int test_utwo(union uTwo *p, int i) {
-  // CHECK-STRICT-0: @__ubsan
-  // CHECK-STRICT-1: @__ubsan
-  // CHECK-STRICT-2: @__ubsan
+  // CHECK-STRICT-0-NOT: @__ubsan
+  // CHECK-STRICT-1:     call void @__ubsan_handle_out_of_bounds_abort(
+  // CHECK-STRICT-2:     call void @__ubsan_handle_out_of_bounds_abort(
   return p->a[i] + (p->a)[i];
 }
 
 // CHECK-LABEL: define {{.*}} @{{.*}}test_uthree{{.*}}(
 int test_uthree(union uThree *p, int i) {
-  // CHECK-STRICT-0: @__ubsan
-  // CHECK-STRICT-1: @__ubsan
-  // CHECK-STRICT-2: @__ubsan
-  return p->a[i] + (p->a)[i];
-}
-
-// CHECK-LABEL: define {{.*}} @{{.*}}test_three{{.*}}(
-int test_three(struct Three *p, int i) {
-  // CHECK-STRICT-0:     call void @__ubsan_handle_out_of_bounds_abort(
+  // CHECK-STRICT-0-NOT: @__ubsan
   // CHECK-STRICT-1:     call void @__ubsan_handle_out_of_bounds_abort(
   // CHECK-STRICT-2:     call void @__ubsan_handle_out_of_bounds_abort(
   return p->a[i] + (p->a)[i];
@@ -96,6 +124,8 @@ struct Macro {
 // CHECK-LABEL: define {{.*}} @{{.*}}test_macro{{.*}}(
 int test_macro(struct Macro *p, int i) {
   // CHECK-STRICT-0-NOT: @__ubsan
+  // CHECK-STRICT-1-NOT: @__ubsan
+  // CHECK-STRICT-2:     call void @__ubsan_handle_out_of_bounds_abort(
   return p->a[i] + (p->a)[i];
 }
 

diff --git a/clang/test/CodeGen/bounds-checking.c b/clang/test/CodeGen/bounds-checking.c
index ca44adf..1b9e289 100644 (file)
--- a/clang/test/CodeGen/bounds-checking.c
+++ b/clang/test/CodeGen/bounds-checking.c
@@ -1,10 +1,10 @@
 // RUN: %clang_cc1 -fsanitize=local-bounds -emit-llvm -triple x86_64-apple-darwin10 %s -o - | FileCheck %s
-// RUN: %clang_cc1 -fsanitize=array-bounds -O -fsanitize-trap=array-bounds -emit-llvm -triple x86_64-apple-darwin10 -DNO_DYNAMIC %s -o - | FileCheck %s --check-prefixes=CHECK,NONLOCAL
+// RUN: %clang_cc1 -fsanitize=array-bounds -O -fsanitize-trap=array-bounds -emit-llvm -triple x86_64-apple-darwin10 -DNO_DYNAMIC %s -o - | FileCheck %s
 //
 // REQUIRES: x86-registered-target
 
-// CHECK-LABEL: @f
-double f(int b, int i) {
+// CHECK-LABEL: @f1
+double f1(int b, int i) {
   double a[b];
   // CHECK: call {{.*}} @llvm.{{(ubsan)?trap}}
   return a[i];
@@ -31,29 +31,38 @@ void f3(void) {
   a[2] = 1;
 }
 
+// CHECK-LABEL: @f4
+__attribute__((no_sanitize("bounds")))
+int f4(int i) {
+  int b[64];
+  // CHECK-NOT: call void @llvm.trap()
+  // CHECK-NOT: trap:
+  // CHECK-NOT: cont:
+  return b[i];
+}
+
+// Union flexible-array memebers are a C99 extension. All array members with a
+// constant size should be considered FAMs.
+
 union U { int a[0]; int b[1]; int c[2]; };
 
-// CHECK-LABEL: define {{.*}} @f4
-int f4(union U *u, int i) {
-  // a and b are treated as flexible array members.
+// CHECK-LABEL: @f5
+int f5(union U *u, int i) {
+  // a is treated as a flexible array member.
   // CHECK-NOT: @llvm.ubsantrap
-  return u->a[i] + u->b[i];
-  // CHECK: }
+  return u->a[i];
 }
 
-// CHECK-LABEL: define {{.*}} @f5
-int f5(union U *u, int i) {
-  // c is not a flexible array member.
-  // NONLOCAL: call {{.*}} @llvm.ubsantrap
-  return u->c[i];
-  // CHECK: }
+// CHECK-LABEL: @f6
+int f6(union U *u, int i) {
+  // b is treated as a flexible array member.
+  // CHECK-NOT: call {{.*}} @llvm.{{(ubsan)?trap}}
+  return u->b[i];
 }
 
-__attribute__((no_sanitize("bounds")))
-int f6(int i) {
-       int b[64];
-       // CHECK-NOT: call void @llvm.trap()
-       // CHECK-NOT: trap:
-       // CHECK-NOT: cont:
-       return b[i];
+// CHECK-LABEL: @f7
+int f7(union U *u, int i) {
+  // c is treated as a flexible array member.
+  // CHECK-NOT: @llvm.ubsantrap
+  return u->c[i];
 }

diff --git a/clang/test/Sema/array-bounds-ptr-arith.c b/clang/test/Sema/array-bounds-ptr-arith.c
index a76a888..974e420 100644 (file)
--- a/clang/test/Sema/array-bounds-ptr-arith.c
+++ b/clang/test/Sema/array-bounds-ptr-arith.c
@@ -1,10 +1,12 @@
-// RUN: %clang_cc1 -verify=expected -Warray-bounds-pointer-arithmetic %s
-// RUN: %clang_cc1 -verify=expected -Warray-bounds-pointer-arithmetic %s -fstrict-flex-arrays=0
+// RUN: %clang_cc1 -verify=expected        -Warray-bounds-pointer-arithmetic %s
+// RUN: %clang_cc1 -verify=expected        -Warray-bounds-pointer-arithmetic %s -fstrict-flex-arrays=0
 // RUN: %clang_cc1 -verify=expected,strict -Warray-bounds-pointer-arithmetic %s -fstrict-flex-arrays=2
 
 // Test case from PR10615
 struct ext2_super_block{
   unsigned char s_uuid[8]; // expected-note {{declared here}}
+  int ignored; // Prevents "s_uuid" from being treated as a flexible array
+               // member.
 };
 
 void* ext2_statfs (struct ext2_super_block *es,int a) {