[clang] Correct sanitizer behavior in union FAMs

[void]

Summary

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.

[kees]

Change log typo? "but for all" should be "but not for all" ?

clang/lib/AST/Expr.cpp

220–221

Isn't this commit addressing this commit, and it can be removed?

clang/test/CodeGen/bounds-checking-fam.c

13

I think it might be better to make this a struct, and adjust it and the other to include a second member to avoid the C99 issues.

struct Zero {
    int ignored;
    int a[0];
};

Also, I think a "real" FAM should be included as well, since its behavior should always be the same, regardless of -fstrict-flex-arrays:

struct FAM {
    int ignored;
    int a[];
}

[serge-sans-paille]

clang/lib/AST/Expr.cpp

226

I <3 this simplification! This is definitively going to introduce regression, but if this aligns with GCC behavior I think it's okay.
I don't see how it's related to union though. Could you explain?

[void]

@kees @serge-sans-paille: It appears to me that a terminating array of size > 2 *isn't* treated as a FAM in Clang, at least Clang warns about it. The first failure above (clang/test/Sema/array-bounds-ptr-arith.c) shows that. It turns out that the same failure occurs in that testcase when the array isn't the last in a structure, so I'll change it.

There's another failure that I'm not too sure about. The Clang.SemaCXX::array-bounds.cpp failure is due to a union that's acting like an FAM. I have a question for you. Should a and c in the union in this code be considered an FAM?

struct {
  union {
    short a[2]; // expected-note 4 {{declared here}}
    char c[4];
  };
  int ignored;
};

clang/lib/AST/Expr.cpp

226

The original code allows any terminating array over size one to not be considered a FAM. And so the conditional below (if (FD->getParent()->isUnion())) is never executed. The rest of this commit is mostly cleaning up the test cases. :-)

[kees]

In D135727#3853896, @void wrote:

@kees @serge-sans-paille: It appears to me that a terminating array of size > 2 *isn't* treated as a FAM in Clang, at least Clang warns about it. The first failure above (clang/test/Sema/array-bounds-ptr-arith.c) shows that. It turns out that the same failure occurs in that testcase when the array isn't the last in a structure, so I'll change it.

Okay, what the heck is even going on in this test? The diagnostic appears to think the array changes in size based on the cast??

<source>:16:24: warning: the pointer incremented by 80 refers past the end of the array (that contains 64 elements) [-Warray-bounds-pointer-arithmetic]
        return (void *)es->s_uuid + 80;
                       ^            ~~

s_uuid is 8, not 64. 64 would be 8 "void *"s. This seems like a very very broken diagnostic?

These should all trip the diagnostic, but don't:

es->s_uuid + 8; /* this is past the end */
(void *)es->s_uuid + 9; /* this is past the end by 1, but doesn't trip because it thinks it's suddenly 8 times larger */

For -Warray-bounds, GCC isn't fooled by the "void *" casts (but doesn't have -Warray-bounds-arithmetic):
https://godbolt.org/z/5eqEEv4of

Note that while the diagnostics of both GCC and Clang complain only about >1 terminal arrays, they both return -1 for __builtin_object_size regardless of length.

So, we're facing, again, a disconnect between diagnostics, bos, and sanitizer. GCC's sanitizer follows bos rules, where-as Clang's sanitizer followed diagnostics rules. Given that bos is used for run-time analysis, it follows that sanitizer and bos should match.

There's another failure that I'm not too sure about. The Clang.SemaCXX::array-bounds.cpp failure is due to a union that's acting like an FAM. I have a question for you. Should a and c in the union in this code be considered an FAM?

This test looks correct to me:

struct {
  union {
    short a[2]; // expected-note 4 {{declared here}}
    char c[4];
  };
  int ignored;
};

I would not expect this to warn or trap because it's not the trailing member of the structure.

[void]

In D135727#3854252, @kees wrote:

In D135727#3853896, @void wrote:

@kees @serge-sans-paille: It appears to me that a terminating array of size > 2 *isn't* treated as a FAM in Clang, at least Clang warns about it. The first failure above (clang/test/Sema/array-bounds-ptr-arith.c) shows that. It turns out that the same failure occurs in that testcase when the array isn't the last in a structure, so I'll change it.

Okay, what the heck is even going on in this test? The diagnostic appears to think the array changes in size based on the cast??

I think we opened the can or worms. :-)

<source>:16:24: warning: the pointer incremented by 80 refers past the end of the array (that contains 64 elements) [-Warray-bounds-pointer-arithmetic]
        return (void *)es->s_uuid + 80;
                       ^            ~~

s_uuid is 8, not 64. 64 would be 8 "void *"s. This seems like a very very broken diagnostic?

These should all trip the diagnostic, but don't:

es->s_uuid + 8; /* this is past the end */
(void *)es->s_uuid + 9; /* this is past the end by 1, but doesn't trip because it thinks it's suddenly 8 times larger */

For -Warray-bounds, GCC isn't fooled by the "void *" casts (but doesn't have -Warray-bounds-arithmetic):
https://godbolt.org/z/5eqEEv4of

Note that while the diagnostics of both GCC and Clang complain only about >1 terminal arrays, they both return -1 for __builtin_object_size regardless of length.

So, we're facing, again, a disconnect between diagnostics, bos, and sanitizer. GCC's sanitizer follows bos rules, where-as Clang's sanitizer followed diagnostics rules. Given that bos is used for run-time analysis, it follows that sanitizer and bos should match.

I created a separate patch to fix the diagnostic (https://reviews.llvm.org/D135920).

There's another failure that I'm not too sure about. The Clang.SemaCXX::array-bounds.cpp failure is due to a union that's acting like an FAM. I have a question for you. Should a and c in the union in this code be considered an FAM?

This test looks correct to me:

struct {
  union {
    short a[2]; // expected-note 4 {{declared here}}
    char c[4];
  };
  int ignored;
};

I would not expect this to warn or trap because it's not the trailing member of the structure.

Yeah, that's what I thought. I'll send out a separate fix.

[kees]

In D135727#3856978, @void wrote:

I think we opened the can or worms. :-)

At this point I think the can we opened has worms with cans of worms with cans of worms. I'd say it's turtles all the way down, but it seems to just be worms instead. ;)

[void]

Update testcases to pass tests.

[void]

@kees @serge-sans-paille I think this is ready for another go. PTAL.

[void]

Update to support SemaCXX tests.

[void]

Add "real" FAM to testcase.

[kees]

This looks great to me. Thanks!

clang/lib/AST/Expr.cpp

 
 bool Expr::isFlexibleArrayMemberLike(
     ASTContext &Context,
     LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel,
     bool IgnoreTemplateOrMacroSubstitution) const {
 
   // 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;
 
   const Expr *E = IgnoreParens();
 
   const NamedDecl *ND = nullptr;
   if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
     ND = DRE->getDecl();
   else if (const auto *ME = dyn_cast<MemberExpr>(E))
     ND = ME->getMemberDecl();
   else if (const auto *IRE = dyn_cast<ObjCIvarRefExpr>(E))
     return IRE->getDecl()->getNextIvar() == nullptr;
 
   if (!ND)
     return false;
 
   // A flexible array member must be the last member in the class.
   // 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())
+    // 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.
     if (IgnoreTemplateOrMacroSubstitution) {
       TypeSourceInfo *TInfo = FD->getTypeSourceInfo();
       while (TInfo) {
         TypeLoc TL = TInfo->getTypeLoc();
         // Look through typedefs.

clang/test/CodeGen/bounds-checking-fam.c

 // REQUIRES: x86-registered-target
 // RUN: %clang_cc1 -emit-llvm -triple x86_64 -fstrict-flex-arrays=0 -fsanitize=array-bounds        %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-STRICT-0
 // RUN: %clang_cc1 -emit-llvm -triple x86_64 -fstrict-flex-arrays=0 -fsanitize=array-bounds -x c++ %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-STRICT-0,CXX,CXX-STRICT-0
 // RUN: %clang_cc1 -emit-llvm -triple x86_64 -fstrict-flex-arrays=1 -fsanitize=array-bounds        %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-STRICT-1
 // RUN: %clang_cc1 -emit-llvm -triple x86_64 -fstrict-flex-arrays=1 -fsanitize=array-bounds -x c++ %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-STRICT-1,CXX
 // RUN: %clang_cc1 -emit-llvm -triple x86_64 -fstrict-flex-arrays=2 -fsanitize=array-bounds        %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-STRICT-2
 // RUN: %clang_cc1 -emit-llvm -triple x86_64 -fstrict-flex-arrays=2 -fsanitize=array-bounds -x c++ %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-STRICT-2,CXX
 // Before flexible array member was added to C99, many projects use a
 // 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
   // CHECK-STRICT-1-NOT: @__ubsan
   // CHECK-STRICT-2:     call void @__ubsan_handle_out_of_bounds_abort(
   return p->a[i] + (p->a)[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];
 }
 
 union uZero {
   int a[0];
 };
int test_uzero(union uZero *p, int i) {
   // CHECK-STRICT-2-NOT: @__ubsan
   return p->a[i] + (p->a)[i];
 }
 
 // CHECK-LABEL: define {{.*}} @{{.*}}test_uone{{.*}}(
 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];
 }
 
 #define FLEXIBLE 1
 struct Macro {
   int a[FLEXIBLE];
 };
 
 // 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];
 }
 
 #if defined __cplusplus
 
 struct Base {
   int b;
 };

clang/test/CodeGen/bounds-checking.c

 // 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];
 }
 
 // CHECK-LABEL: @f2
 void f2(void) {
   // everything is constant; no trap possible

 
 // CHECK-LABEL: @f3
 void f3(void) {
   int a[1];
   // CHECK: call {{.*}} @llvm.{{(ubsan)?trap}}
   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];
 }

clang/test/Sema/array-bounds-ptr-arith.c

-// 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) {
   return (void *)es->s_uuid + sizeof(int); // no-warning
 }
 void* broken (struct ext2_super_block *es,int a) {
   return (void *)es->s_uuid + 9; // expected-warning {{the pointer incremented by 9 refers past the end of the array (that has type 'unsigned char[8]')}}
 }