[analyzer] Ignore IncompleteArrayTypes in getStaticSize() for FAMs

[steakhal]

Summary

Currently only ConstantArrayType is considered for flexible array
members (FAMs) in getStaticSize().
However, IncompleteArrayType also shows up in practice as FAMs.

This patch will ignore the IncompleteArrayType and return Unknown
for that case as well. This way it will be at least consistent with
the current behavior until we start modeling them accurately.

I'm expecting that this will resolve a bunch of false-positives internally,
caused by the ArrayBoundV2.

[martong]

Thanks!

clang/test/Analysis/out-of-bounds-new.cpp

171 ↗

(On Diff #355504)

Should we have test cases with data[1] (and data[0] if that makes sense at all) ?
Would data[1] behave the same way? And if not, even then perhaps we should have test cases just to document that.

[steakhal]

clang/test/Analysis/out-of-bounds-new.cpp

171 ↗

(On Diff #355504)

We could add tests for them in a subsequent patch.
But strictly speaking, those arrays would be of ConstantArrayType types.
That being said, those were indistinguishable from FAMs or just a regular array of size N.

Maybe we can figure out some heuristics driving this, but that's definitely out of the scope of this patch.

[martong]

clang/test/Analysis/out-of-bounds-new.cpp

169 ↗

(On Diff #355504)

We could add tests for them in a subsequent patch.

Strictly speaking, flexible array members are defined in C99 (6.7.2.1), thus I think we should have these tests at least with a different RUN line (i.e. with -std=c99) or in a different file. But if we create a new test file then that could easily handle data[1] and data[0] cases as well.
So I am okay with this change if I can see the subsequent patch in the stack.

[ASDenysPetrov]

The fix seems pretty resonable to me. Indeed IncompleteArrayType can happen according to this paper https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
I'd like to see more test cases mentioned by @martong.

[steakhal]

• wrapped the FAM detection into a lambda, which could be extended later if needed
• added test cases for: [0], [] and [1] array members, on the stack (automatic storage), alloca malloc using a bunch of C and C++ versions.

[ASDenysPetrov]

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

776–779

I think we can omit getAsArrayType, since we check for certain array types anyway.

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

6–10

AFAIK, FAM is not a part of C++ Standard. Does clang support it as extension?

73

I'd like to use another naming here, because int data[1] is not actually a flexible array.

[steakhal]

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

776–779

No, we can't. ASTContext::getAsArrayType() returns a canonical type, which could be used later by the regular isa/dyn_cast API, but you cannot use them on Ty.
In fact, first I tried what you proposed, but I hit a static assertion checking for exactly this.

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

6–10

You are completely correct.

73

What do you think would be a better fit?
I already highlighted this at the variable declaration with the name likely_fam - at least that was my concept.

[ASDenysPetrov]

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

776–779

I see. OK, then.

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

73

You can name it as SAM kinda Static Array Member or AM - Array Member. Whatever. My point is just to be closer to the meaning. But you can ignore this if you want, because we can understand what you mean from the context.

[steakhal]

Thank you for the review @ASDenysPetrov! I'd like to commit this in its current form.

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

case MemRegion::ObjCStringRegionKind: {
     return getElementExtent(Ty, SVB);
   }
   case MemRegion::FieldRegionKind: {
     // Force callers to deal with bitfields explicitly.
     if (cast<FieldRegion>(SR)->getDecl()->isBitField())
       return UnknownVal();
 
     QualType Ty = cast<TypedValueRegion>(SR)->getDesugaredValueType(Ctx);
-    DefinedOrUnknownSVal Size = getElementExtent(Ty, SVB);
+    const DefinedOrUnknownSVal Size = getElementExtent(Ty, SVB);
 
     // A zero-length array at the end of a struct often stands for dynamically
     // allocated extra memory.
-    if (Size.isZeroConstant()) {
-      if (isa<ConstantArrayType>(Ty))
-        return UnknownVal();
+    const auto isFlexibleArrayMemberCandidate = [this](QualType Ty) -> bool {
+      const ArrayType *AT = Ctx.getAsArrayType(Ty);
+      if (!AT)
+        return false;
+      if (isa<IncompleteArrayType>(AT))
+        return true;
+
+      if (const auto *CAT = dyn_cast<ConstantArrayType>(AT)) {
+        const llvm::APInt &Size = CAT->getSize();
+        if (Size.isNullValue())
+          return true;
       }
+      return false;
+    };
+
+    if (isFlexibleArrayMemberCandidate(Ty))
+      return UnknownVal();
 
     return Size;
   }
     // FIXME: The following are being used in 'SimpleSValBuilder' and in
     // 'ArrayBoundChecker::checkLocation' because there is no symbol to
     // represent the regions more appropriately.
   case MemRegion::BlockDataRegionKind:
   case MemRegion::BlockCodeRegionKind:

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

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection %s -verify -std=c90
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection %s -verify -std=c99
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection %s -verify -std=c11
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection %s -verify -std=c17
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection %s -verify -std=c++98 -x c++
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection %s -verify -std=c++03 -x c++
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection %s -verify -std=c++11 -x c++
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection %s -verify -std=c++14 -x c++
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection %s -verify -std=c++17 -x c++
+
+typedef __typeof(sizeof(int)) size_t;
+size_t clang_analyzer_getExtent(void *);
+void clang_analyzer_dump(size_t);
+
+void *alloca(size_t size);
+void *malloc(size_t size);
+void free(void *ptr);
+
+void test_incomplete_array_fam() {
+  typedef struct FAM {
+    char c;
+    int data[];
+  } FAM;
+
+  FAM fam;
+  clang_analyzer_dump(clang_analyzer_getExtent(&fam));
+  clang_analyzer_dump(clang_analyzer_getExtent(fam.data));
+  // expected-warning@-2 {{4 S64b}}
+  // expected-warning@-2 {{Unknown}}
+
+  FAM *p = (FAM *)alloca(sizeof(FAM));
+  clang_analyzer_dump(clang_analyzer_getExtent(p));
+  clang_analyzer_dump(clang_analyzer_getExtent(p->data));
+  // expected-warning@-2 {{4 U64b}}
+  // expected-warning@-2 {{Unknown}}
+
+  FAM *q = (FAM *)malloc(sizeof(FAM));
+  clang_analyzer_dump(clang_analyzer_getExtent(q));
+  clang_analyzer_dump(clang_analyzer_getExtent(q->data));
+  // expected-warning@-2 {{4 U64b}}
+  // expected-warning@-2 {{Unknown}}
+  free(q);
+}
+
+void test_zero_length_array_fam() {
+  typedef struct FAM {
+    char c;
+    int data[0];
+  } FAM;
+
+  FAM fam;
+  clang_analyzer_dump(clang_analyzer_getExtent(&fam));
+  clang_analyzer_dump(clang_analyzer_getExtent(fam.data));
+  // expected-warning@-2 {{4 S64b}}
+  // expected-warning@-2 {{Unknown}}
+
+  FAM *p = (FAM *)alloca(sizeof(FAM));
+  clang_analyzer_dump(clang_analyzer_getExtent(p));
+  clang_analyzer_dump(clang_analyzer_getExtent(p->data));
+  // expected-warning@-2 {{4 U64b}}
+  // expected-warning@-2 {{Unknown}}
+
+  FAM *q = (FAM *)malloc(sizeof(FAM));
+  clang_analyzer_dump(clang_analyzer_getExtent(q));
+  clang_analyzer_dump(clang_analyzer_getExtent(q->data));
+  // expected-warning@-2 {{4 U64b}}
+  // expected-warning@-2 {{Unknown}}
+  free(q);
+}
+
+void test_single_element_array_possible_fam() {
+  typedef struct FAM {
+    char c;
+    int data[1];
+  } FAM;
+
+  FAM likely_fam;
+  clang_analyzer_dump(clang_analyzer_getExtent(&likely_fam));
+  clang_analyzer_dump(clang_analyzer_getExtent(likely_fam.data));
+  // expected-warning@-2 {{8 S64b}}
+  // expected-warning@-2 {{4 S64b}}
+
+  FAM *p = (FAM *)alloca(sizeof(FAM));
+  clang_analyzer_dump(clang_analyzer_getExtent(p));
+  clang_analyzer_dump(clang_analyzer_getExtent(p->data));
+  // expected-warning@-2 {{8 U64b}}
+  // expected-warning@-2 {{4 S64b}}
+
+  FAM *q = (FAM *)malloc(sizeof(FAM));
+  clang_analyzer_dump(clang_analyzer_getExtent(q));
+  clang_analyzer_dump(clang_analyzer_getExtent(q->data));
+  // expected-warning@-2 {{8 U64b}}
+  // expected-warning@-2 {{4 S64b}}
+  free(q);
+}