[analyzer] Enhance array bound checking for ConstantArrayType

clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp

@@ -593,19 +593,19 @@ std::string StateUpdateReporter::getMessage(PathSensitiveBugReport &BR) const {
 // Note that "real" Flexible Array Members are `IncompleteArrayType`.
 // For them, this function returns `std::nullopt`.
 static std::optional<std::pair<QualType, nonloc::ConcreteInt>>
-getArrayTypeInfo(SValBuilder &svb, ArraySubscriptExpr const *expr) {
-  auto const *arrayBaseExpr = expr->getBase()->IgnoreParenImpCasts();
-  auto const arrayQualType = arrayBaseExpr->getType().getCanonicalType();
-  if (arrayQualType.isNull() || !arrayQualType->isConstantArrayType()) {
+getArrayTypeInfo(SValBuilder &SVB, ArraySubscriptExpr const *E) {
+  Expr const *ArrayBaseExpr = E->getBase()->IgnoreParenImpCasts();
+  QualType const ArrayQualType = ArrayBaseExpr->getType().getCanonicalType();
+  if (ArrayQualType.isNull() || !ArrayQualType->isConstantArrayType())
     return std::nullopt;
-  }
-  auto *constArrayType =
-      dyn_cast<ConstantArrayType>(arrayQualType->getAsArrayTypeUnsafe());
-  if (!constArrayType) {
+
+  auto *ConstArrayType =
+      dyn_cast<ConstantArrayType>(ArrayQualType->getAsArrayTypeUnsafe());
+  if (!ConstArrayType)
     return std::nullopt;
-  }
+
   return std::make_pair(
-      constArrayType->getElementType(),
+      ConstArrayType->getElementType(),
       // Note that an array size is technically unsigned, but
       // `compareValueToThreshold` (via `getSimplifiedOffsets`)
       // will do some arithmetics that could overflow and cause FN. For instance
@@ -617,17 +617,16 @@ getArrayTypeInfo(SValBuilder &svb, ArraySubscriptExpr const *expr) {
       // and if 20 is unsigned, that will overflow to the biggest possible array
       // which is always trivially true. We obviously do not want that, so we
       // need to treat the size as signed.
-      svb.makeIntVal(llvm::APSInt{constArrayType->getSize(), false}));
+      SVB.makeIntVal(llvm::APSInt{ConstArrayType->getSize(), false}));
 }
 
 static Messages getNegativeIndexMessage(StringRef Name,
                                         nonloc::ConcreteInt ArraySize,
                                         NonLoc Index) {
-  auto const ArraySizeVal = ArraySize.getValue()->getZExtValue();
+  uint64_t const ArraySizeVal = ArraySize.getValue()->getZExtValue();
   std::string const IndexStr = [&]() -> std::string {
-    if (auto ConcreteIndex = getConcreteValue(Index);
-        ConcreteIndex.has_value()) {
-      return formatv(" {0}", ConcreteIndex);
+    if (std::optional<int64_t> ConcreteIndex = getConcreteValue(Index)) {
+      return formatv(" {0}", *ConcreteIndex);
     }
     return "";
   }();
@@ -637,11 +636,11 @@ static Messages getNegativeIndexMessage(StringRef Name,
                   Name, ArraySizeVal, IndexStr)};
 }
 
-static std::string truncateWithEllipsis(StringRef str, size_t maxLength) {
-  if (str.size() <= maxLength)
-    return str.str();
+static std::string truncateWithEllipsis(StringRef Str, size_t MaxLength) {
+  if (Str.size() <= MaxLength)
+    return Str.str();
 
-  return (str.substr(0, maxLength - 3) + "...").str();
+  return (Str.substr(0, MaxLength - 3) + "...").str();
 }
 
 static Messages getOOBIndexMessage(StringRef Name, NonLoc Index,
@@ -662,7 +661,8 @@ static Messages getOOBIndexMessage(StringRef Name, NonLoc Index,
     Out << "an overflowing index";
   }
 
-  const auto ElemTypeStr = truncateWithEllipsis(ElemType.getAsString(), 20);
+  std::string const ElemTypeStr =
+      truncateWithEllipsis(ElemType.getAsString(), 20);
 
   Out << ", while it holds only ";
   if (ExtentN != 1)
@@ -675,54 +675,11 @@ static Messages getOOBIndexMessage(StringRef Name, NonLoc Index,
           std::string(Buf)};
 }
 
-// "True" flexible array members do not specify any size (`int elems[];`)
-// However, some projects use "fake flexible arrays" (aka "struct hack"), where
-// they specify a size of 0 or 1 to work around a compiler limitation.
-// "True" flexible array members are `IncompleteArrayType` and will be skipped
-// by `performCheckArrayTypeIndex`. We need an heuristic to identify "fake"
-// ones.
-static bool isFakeFlexibleArrays(const ArraySubscriptExpr *E) {
-  auto getFieldDecl = [](ArraySubscriptExpr const *array) -> FieldDecl * {
-    const Expr *BaseExpr = array->getBase()->IgnoreParenImpCasts();
-    if (const MemberExpr *ME = dyn_cast<MemberExpr>(BaseExpr)) {
-      return dyn_cast<FieldDecl>(ME->getMemberDecl());
-    }
-    return nullptr;
-  };
-  auto const isLastField = [](RecordDecl const *Parent,
-                              FieldDecl const *Field) {
-    const FieldDecl *LastField = nullptr;
-    for (const FieldDecl *F : Parent->fields()) {
-      LastField = F;
-    }
-
-    return (LastField == Field);
-  };
-  // We expect placeholder constant arrays to have size 0 or 1.
-  auto maybeConstArrayPlaceholder = [](QualType Type) {
-    const ConstantArrayType *CAT =
-        dyn_cast<ConstantArrayType>(Type->getAsArrayTypeUnsafe());
-    return CAT && CAT->getSize().getZExtValue() <= 1;
-  };
-
-  if (auto const *Field = getFieldDecl(E)) {
-    if (!maybeConstArrayPlaceholder(Field->getType()))
-      return false;
-
-    const RecordDecl *Parent = Field->getParent();
-    return Parent && (Parent->isUnion() || isLastField(Parent, Field));
-  }
-
-  return false;
-}
-
 // Generate a representation of `Expr` suitable for diagnosis.
-SmallString<128> ExprReprForDiagnosis(ArraySubscriptExpr const *E,
-                                      CheckerContext &C) {
+static std::string ExprReprForDiagnosis(Expr const *Base, CheckerContext &C) {
   SmallString<128> Buf;
   llvm::raw_svector_ostream Out(Buf);
 
-  auto const *Base = E->getBase()->IgnoreParenImpCasts();
   switch (Base->getStmtClass()) {
   case Stmt::MemberExprClass:
     Out << "the field '";
@@ -740,7 +697,7 @@ SmallString<128> ExprReprForDiagnosis(ArraySubscriptExpr const *E,
     Out << '\'';
   }
 
-  return Buf;
+  return std::string(Buf);
 }
 
 class StateIndexUpdateReporter {
@@ -800,16 +757,20 @@ auto ArrayBoundChecker::performCheckArrayTypeIndex(const ArraySubscriptExpr *E,
   auto State = C.getState();
   SValBuilder &SVB = C.getSValBuilder();
 
-  auto const ArrayInfo = getArrayTypeInfo(SVB, E);
-  auto const Index =
+  std::optional<std::pair<QualType, nonloc::ConcreteInt>> const ArrayInfo =
+      getArrayTypeInfo(SVB, E);
+  std::optional<NonLoc> const Index =
       SVB.simplifySVal(State, C.getSVal(E->getIdx())).getAs<NonLoc>();
   if (!ArrayInfo || !Index)
     return ConstantArrayIndexResult::Unknown;
 
   auto const &[ArrayType, ArraySize] = *ArrayInfo;
 
-  auto const ExprAsStr = ExprReprForDiagnosis(E, C);
-  bool const IsFakeFAM = isFakeFlexibleArrays(E);
+  Expr const *BaseExpr = E->getBase()->IgnoreImpCasts();
+  std::string const ExprAsStr = ExprReprForDiagnosis(BaseExpr, C);
+  bool const IsFakeFAM = BaseExpr->isFlexibleArrayMemberLike(
+      C.getASTContext(), C.getLangOpts().getStrictFlexArraysLevel(),
+      /*IgnoreTemplateOrMacroSubstitution=*/true);
 
   StateIndexUpdateReporter SUR(ExprAsStr, ArrayType, *Index, ArraySize);
 

clang/test/Analysis/ArrayBound/brief-tests.c

@@ -1,8 +1,4 @@
 // RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,security.ArrayBound,debug.ExprInspection -verify %s
-// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,security.ArrayBound,debug.ExprInspection \
-// RUN:                    -DWARN_FLEXIBLE_ARRAY -analyzer-config security.ArrayBound:EnableFakeFlexibleArrayWarn=true -verify %s
-
-#include "../Inputs/system-header-simulator-for-malloc.h"
 
 // Miscellaneous tests for `security.ArrayBound` where we only test the
 // presence or absence of a bug report. If a test doesn't fit in a more
@@ -11,8 +7,6 @@
 
 void clang_analyzer_value(int);
 void clang_analyzer_eval(int);
-void clang_analyzer_warnIfReached();
-
 
 // Tests doing an out-of-bounds access after the end of an array using:
 // - constant integer index
@@ -331,106 +325,6 @@ int unsigned_index_quirk_inner(unsigned char index) {
   return array[index]; // no warning
 }
 
-struct WithTrailingArray {
-  int nargs;
-  int args[1];
-};
-
-int use_trailing(struct WithTrailingArray *p) {
-#ifdef WARN_FLEXIBLE_ARRAY
-  // expected-warning@+2 {{Potential out of bound access to the field 'args', which may be a 'flexible array member'}}
-#endif
-  int x = p->args[3];
-  // No sink
-  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
-  return x;
-}
-
-struct NotReallyTrailing {
-  int array[1];
-  int more_data;
-};
-
-int use_non_trailing(struct NotReallyTrailing *p) {
-  int x = p->array[3]; // expected-warning {{Out of bound access to memory after the end of the field 'array'}}
-  clang_analyzer_warnIfReached(); // sink, no warning
-  return x;
-}
-
-struct TrailingNoException {
-  int n;
-  int some[2];
-};
-
-int use_trailing_no_exception(struct TrailingNoException *p) {
-  int x = p->some[10]; // expected-warning {{Out of bound access to memory after the end of the field 'some'}}
-  clang_analyzer_warnIfReached(); // sink, no warning
-  return x;
-}
-
-int regular_one_array() {
-  int array[1];
-  int x = array[2]; // expected-warning {{Out of bound access to memory after the end of 'array'}}
-  clang_analyzer_warnIfReached(); // sink, no warning
-  return x;
-}
-
-struct WithTrailing0 {
-  int nargs;
-  int args[0];
-};
-
-int use_trailing0(struct WithTrailing0 *p) {
-#ifdef WARN_FLEXIBLE_ARRAY
-  // expected-warning@+2 {{Potential out of bound access to the field 'args', which may be a 'flexible array member'}}
-#endif
-  int x = p->args[10]; // no warning
-  // No sink
-  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
-  return x;
-}
-
-struct RealFlexibleArray {
-  int nargs;
-  int args[];
-};
-
-// Trailing array, allocation unknown
-int use_real_trailing(struct RealFlexibleArray *p) {
-  int x = p->args[3]; // no warning
-  // No sink
-  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
-  return x;
-}
-
-int use_allocated_fam() {
-  struct RealFlexibleArray *p = malloc(sizeof(struct RealFlexibleArray) + 10 * sizeof(int));
-  // This is a false negative.
-  // `ArrayBoundCheckers` queries for the extent of the memory region
-  // `Element{SymRegion{conj_$2{void *, LC1, S1173, #1}},0 S64b,struct RealFlexibleArray}.args`,
-  // which is unknown. Either `ArrayBoundCheckers`, or `DynamicExtent.cpp` should
-  // learn how to handle this pattern.
-  int x = p->args[30];
-  free(p);
-  return x;
-}
-
-// Pattern used in GCC
-union FlexibleArrayUnion {
-  int args[1];
-  struct {int x, y, z;};
-};
-
-int use_union(union FlexibleArrayUnion *p) {
-#ifdef WARN_FLEXIBLE_ARRAY
-  // expected-warning@+2 {{Potential out of bound access to the field 'args', which may be a 'flexible array member'}}
-#endif
-  int x = p->args[2];
-  // No sink
-  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
-  return x;
-}
-
 void custom_unreachable();
 
 int get_index(char b) {

clang/test/Analysis/ArrayBound/flexible-array-members.c

@@ -0,0 +1,101 @@
+// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,security.ArrayBound,debug.ExprInspection \
+// RUN:                    -fstrict-flex-arrays=0 -DSTRICT_FLEX=0 \
+// RUN:                    -verify %s
+//
+// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,security.ArrayBound,debug.ExprInspection \
+// RUN:                    -fstrict-flex-arrays=1 -DSTRICT_FLEX=1 \
+// RUN:                    -verify %s
+//
+// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,security.ArrayBound,debug.ExprInspection \
+// RUN:                    -fstrict-flex-arrays=2 -DSTRICT_FLEX=2 \
+// RUN:                    -verify %s
+//
+// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,security.ArrayBound,debug.ExprInspection \
+// RUN:                    -fstrict-flex-arrays=3 -DSTRICT_FLEX=3 \
+// RUN:                    -verify %s
+
+#include "../Inputs/system-header-simulator-for-malloc.h"
+
+void clang_analyzer_warnIfReached();
+
+struct RealFAM {
+  int size;
+  int args[];
+};
+
+int use_fam(struct RealFAM *ptr) {
+  int x = ptr->args[1];
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  return x;
+}
+
+struct FAM0 {
+  int size;
+  int args[0];
+};
+
+int use_fam0(struct FAM0 *ptr) {
+  int x = ptr->args[1];
+#if STRICT_FLEX > 2
+  // expected-warning@-2 {{Out of bound access to memory after the end of the field 'args'}}
+#else
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+#endif
+  return x;
+}
+
+struct FAM1 {
+  int size;
+  int args[1];
+};
+
+int use_fam1(struct FAM1 *ptr) {
+  int x = ptr->args[2];
+#if STRICT_FLEX > 1
+  // expected-warning@-2 {{Out of bound access to memory after the end of the field 'args'}}
+#else
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+#endif
+  return x;
+}
+
+struct FAMN {
+  int size;
+  int args[64];
+};
+
+int use_famn(struct FAMN *ptr) {
+  int x = ptr->args[128];
+#if STRICT_FLEX > 0
+  // expected-warning@-2 {{Out of bound access to memory after the end of the field 'args'}}
+#else
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+#endif
+  return x;
+}
+
+struct NotFAM {
+  int keys[1];
+  int values[1];
+};
+
+int use_not_fam(struct NotFAM *ptr) {
+  return ptr->keys[3]; // expected-warning {{Out of bound access to memory after the end of the field 'keys'}}
+}
+
+// Pattern used in GCC
+union FlexibleArrayUnion {
+  int args[1];
+  struct {int x, y, z;};
+};
+
+
+int use_union(union FlexibleArrayUnion *p) {
+  int x = p->args[2];
+#if STRICT_FLEX > 1
+  // expected-warning@-2 {{Out of bound access to memory after the end of the field 'args'}}
+#else
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+#endif
+  return x;
+}