[analyzer] Fix for incorrect handling of 0 length non-POD array construction

[isuckatcs]

Summary

We current handle 0 size non-POD arrays as if they had 1 element, which leads to false positives.

See https://godbolt.org/z/6E98xY66W for one example.

This patch fixes this bug.

[steakhal]

Thanks for the patch.
Please check if all the touched lines are covered by tests.
Other than a few nits it looks great!

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

610

Consider moving this comment to the condition where you check Size == 0.

613

1183

Why was the UndefinedVal inappropriate? In fact, that seems to be the right value.

1206–1207

You could be specific about that case and set the UndefinedVal initial value instead of relying on that's the previous value of InitVal.

clang/test/Analysis/array-init-loop.cpp

237

I didn't know this is a thing. I thought it was ill-formed c++.
Aha, they are diagnosed by -Wzero-length-array as warning: zero size arrays are an extension. Interesting.
And it seems like sizeof(arr) is zero in the backend; and we should in deed support this construct.

[isuckatcs]

• Addressed comments
• Added more testcases

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

1183

If InitVal is undefined, State = State->bindLoc(FieldLoc, InitVal, LocCtxt); will crash.

That method eventually calls RegionStoreManager::bindArray() where we are not handling if the value is Undefined, so when we reach const nonloc::CompoundVal& CV = Init.castAs<nonloc::CompoundVal>(); the cast fails and we have a crash.

I changed the default value and if the value is Undefined I don't bind it to the store at all.

It seems that in this situation it doesn't matter what the value is, as the captured 0 size array will only function as a pointer to the beginning of the lambda. See it on godbolt. That means if we read some value from the array it is not necessarily Undefined, so I assume there's no need for any special handling here.

[isuckatcs]

Rebased

[isuckatcs]

Rebased and added more test cases

[NoQ]

Brilliant catch as always!

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

1199–1205

This code is communicating a fairly specific, unusual, peculiar situation of a zero-size array through a very generic, uninformative, unexpressive means of object-under-construction not being set. I.e., if I see object-under-construction not being set, zero-size array is the last thing I'd suspect. It's very likely that someone else will try to communicate some other information by not setting object-under-construction, and then we'll hit a hard-to-debug cornercase interaction when you think it means one thing but it actually means the other thing.

Is there a more direct way to communicate that the array is zero size? Like, make a special marker specifically for that purpose, that definitely can't be confused with anything else?

[xazax.hun]

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

616

Should this be an else if?

623–628

Note that some compilers treat zero size arrays as flexible array members, see https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

I am not sure whether we actually would ever need special handling for those as the Ctors probably need to be invoked manually for those cases and we would see that invocation in the analyzer. But a simple TODO to test that out at some point might be handy.

[isuckatcs]

Rebase and addressed some comments.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

1199–1205

In this situation I think it's the other way around. It's not like the array being zero length is communicated by not marking it as under construction, it's rather like when the array is zero length, the constructors are not invoked, so object under construction isn't set either.

But this snippet has changed over time, and the size of the array is explicitly checked instead.

[isuckatcs]

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

623–628

I think that case requires no special handling. The declaration of a zero length array is like a no-op regardless of where it is, no assembly code is generated for it.

If you have a struct like struct S { int size; Aggregate arr[0]; }, and instantiate it on the stack, the constructor for Aggregate is not invoked, so I also ignore it in this patch.

If memory is allocated using malloc() or ::operator new()
(e.g.: S *s = (S*)::operator new(offsetof(S, arr) + sizeof(Aggregate) * 10);),
the constructors need to be called manually.

If the developer calls new that will either not compile (e.g.: S s; s.arr = new Aggregate[10];), or will lead to memory corruption (e.g.: S *s = (S*)new Aggregate[10];). In the later case the analyzer should simply take the symbolic region of s and construct everything there, though I haven't tested this yet.

I'm going to add a test case for this feature soon, I just wanted to explain how I think about it, in case I miss something.

[isuckatcs]

Fixed an error prone line

[xazax.hun]

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

623–628

Sounds good, thanks!

[isuckatcs]

@xazax.hun I added a test case for how I imagine flexible non-POD array members in C++

[xazax.hun]

In D131501#3745249, @isuckatcs wrote:

@xazax.hun I added a test case for how I imagine flexible non-POD array members in C++

Thanks! I think the test case could be simplified a bit, I am not sure whether we need offsetof, but otherwise looks good to me.

clang/test/Analysis/flexible-array-member.cpp

35

I am not sure how idiomatic calling operator new is, I mostly see code examples with malloc around the internet. But feel free to leave this as is.

Most of the examples I see look like: (Flex*)malloc(sizeof(Flex) + sizeof(S) * size) which is admittedly a bit C-like, but I think you might not need the offsetof.

[isuckatcs]

clang/test/Analysis/flexible-array-member.cpp

35

I am not sure how idiomatic calling operator new is, I mostly see code examples with malloc around the internet. But feel free to leave this as is.

I wanted the test to be more "C++-like", and both of those functions do the same, so I chose ::operator new(). I'll leave it like this for now. We can change it anytime later if required.

I think you might not need the offsetof.

True, the size of a zero length array is 0, so the size of the struct is equivalent to the size of length. I was assuming some padding between length and contents, which might happen with a 1 element array (I've seen this construct used with a 1 element array, since that's accepted by every compiler). If a 1 element array is used, I think it's more expressive to do offsetof(Flex, contents) + sizeof(S) * size instead of sizeof(Flex) + sizeof(S) * (size - 1).

[xazax.hun]

clang/test/Analysis/flexible-array-member.cpp

35

I was assuming some padding between length and contents

I am not very well versed in this area, but I'd expect the compiler to include that padding in sizeof(Flex). At least, that would be the least error prone solution.

[isuckatcs]

clang/test/Analysis/flexible-array-member.cpp

35

I'd expect the compiler to include that padding in sizeof(Flex).

It includes the padding. What I meant is that imagine you have a similar struct, with a one element array
(e.g.: struct S {int length; void *elements[1];};). Then you will probably have 4 padding bytes between length and elements.

If you want to allocate memory for an array with size elements, you would either do sizeof(S) + sizeof(void*) * (size - 1) or
offsetof(S, elements) + sizeof(void*) * size. The second one is more readable I think.

This was on my mind when I was writing the test case, hence the offsetof solution. It's true that you probably don't want to create a field like S arr[1], because that would also invoke a default constructor (if exists).

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

case CXXConstructExpr::CK_Complete: {
     assert(CE && !CIE && "A complete constructor is inherited?!");
 
     // If the ctor is part of an ArrayInitLoopExpr, we want to handle it
     // differently.
     auto *AILE = CC ? CC->getArrayInitLoop() : nullptr;
 
     unsigned Idx = 0;
     if (CE->getType()->isArrayType() || AILE) {
+
+      auto isZeroSizeArray = [&] {
+        uint64_t Size = 1;
+
+        if (const auto *CAT = dyn_cast<ConstantArrayType>(CE->getType()))
+          Size = getContext().getConstantArrayElementCount(CAT);
+        else if (AILE)
+          Size = getContext().getArrayInitLoopExprElementCount(AILE);
+
+        return Size == 0;
+      };
+
+      // No element construction will happen in a 0 size array.
+      if (isZeroSizeArray()) {
+        StmtNodeBuilder Bldr(Pred, destNodes, *currBldrCtx);
+        static SimpleProgramPointTag T{"ExprEngine",
+                                       "Skipping 0 size array construction"};
+        Bldr.generateNode(CE, Pred, State, &T);
+        return;
+      }
+
       Idx = getIndexOfElementToConstruct(State, CE, LCtx).value_or(0u);
       State = setIndexOfElementToConstruct(State, CE, LCtx, Idx + 1);
     }
 
     if (AILE) {
       // Only set this once even though we loop through it multiple times.
       if (!getPendingInitLoop(State, CE, LCtx))
         State = setPendingInitLoop(
void ExprEngine::VisitLambdaExpr(const LambdaExpr *LE, ExplodedNode *Pred,
   unsigned Idx = 0;
   CXXRecordDecl::field_iterator CurField = LE->getLambdaClass()->field_begin();
   for (LambdaExpr::const_capture_init_iterator i = LE->capture_init_begin(),
                                                e = LE->capture_init_end();
        i != e; ++i, ++CurField, ++Idx) {
     FieldDecl *FieldForCapture = *CurField;
     SVal FieldLoc = State->getLValue(FieldForCapture, V);
 
     SVal InitVal;
     if (!FieldForCapture->hasCapturedVLAType()) {
       const Expr *InitExpr = *i;
 
       assert(InitExpr && "Capture missing initialization expression");
 
+      // Capturing a 0 length array is a no-op, so we ignore it to get a more
+      // accurate analysis. If it's not ignored, it would set the default
+      // binding of the lambda to 'Unknown', which can lead to falsely detecting
+      // 'Uninitialized' values as 'Unknown' and not reporting a warning.
+      const auto FTy = FieldForCapture->getType();
+      if (FTy->isConstantArrayType() &&
+          getContext().getConstantArrayElementCount(
+              getContext().getAsConstantArrayType(FTy)) == 0)
+        continue;
+
       // With C++17 copy elision the InitExpr can be anything, so instead of
       // pattern matching all cases, we simple check if the current field is
       // under construction or not, regardless what it's InitExpr is.
       if (const auto OUC =
               getObjectUnderConstruction(State, {LE, Idx}, LocCtxt)) {
         InitVal = State->getSVal(OUC->getAsRegion());
 
         State = finishObjectConstruction(State, {LE, Idx}, LocCtxt);
       } else
         InitVal = State->getSVal(InitExpr, LocCtxt);
 
     } else {
 
       assert(!getObjectUnderConstruction(State, {LE, Idx}, LocCtxt) &&
              "VLA capture by value is a compile time error!");
 
       // The field stores the length of a captured variable-length array.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

if (FD->isUnnamedBitfield())
       continue;
 
     // If there are too many fields, or if any of the fields are aggregates,
     // just use the LCV as a default binding.
     if (Fields.size() == SmallStructLimit)
       return None;
 
     QualType Ty = FD->getType();
+
+    // Zero length arrays are basically no-ops, so we also ignore them here.
+    if (Ty->isConstantArrayType() &&
+        Ctx.getConstantArrayElementCount(Ctx.getAsConstantArrayType(Ty)) == 0)
+      continue;
+
     if (!(Ty->isScalarType() || Ty->isReferenceType()))
       return None;
 
     Fields.push_back(FD);
   }
 
   RegionBindingsRef NewB = B;
 

clang/test/Analysis/array-init-loop.cpp

 void lambda_capture_multi_array() {
   S3 arr[2][2] = {1,2,3,4};
 
   {
     int x = [arr] { return arr[0][0].i; }();
     clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}
   }
 
   {
     int x = [arr] { return arr[0][1].i; }();
     clang_analyzer_eval(x == 2); // expected-warning{{TRUE}}
   }
 
   {
     int x = [arr] { return arr[1][0].i; }();
     clang_analyzer_eval(x == 3); // expected-warning{{TRUE}}
   }
void structured_binding_multi() {
 
   auto [a,b] = arr;
 
   clang_analyzer_eval(a[0].i == 1); // expected-warning{{TRUE}}
   clang_analyzer_eval(a[1].i == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(b[0].i == 3); // expected-warning{{TRUE}}
   clang_analyzer_eval(b[1].i == 4); // expected-warning{{TRUE}}
 }
+
+// This snippet used to crash
+namespace crash {
+
+struct S
+{
+  int x;
+  S() { x = 1; }
+};
+
+void no_crash() {
+  S arr[0];
+  int n = 1;
+
+  auto l = [arr, n] { return n; };
+
+  int x = l();
+  clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}
+
+  // FIXME: This should be 'Undefined'.
+  clang_analyzer_eval(arr[0].x); // expected-warning{{UNKNOWN}}
+}
+
+} // namespace crash

clang/test/Analysis/dtor-array.cpp

void zeroLength(){
   delete[] arr2;
 
   auto *arr3 = new InlineDtor[0][2][2];
   delete[] arr3;
 
   auto *arr4 = new InlineDtor[2][2][0];
   delete[] arr4;
 
-  // FIXME: Should be TRUE once the constructors are handled properly.
-  clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+  clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}}
 }
 
 
 void evalOrderPrep() {
   EvalOrderArr[0] = 0;
   EvalOrderArr[1] = 0;
   EvalOrderArr[2] = 0;
   EvalOrderArr[3] = 0;

clang/test/Analysis/flexible-array-member.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_eval(bool);
+
+struct S
+{
+    static int c;
+    static int d;
+    int x;
+    S() { x = c++; }
+    ~S() { d++; }
+};
+
+int S::c = 0;
+int S::d = 0;
+
+struct Flex
+{
+    int length;
+    S contents[0];
+};
+
+void flexibleArrayMember()
+{
+    S::c = 0;
+    S::d = 0;
+
+    const int size = 4;
+
+    Flex *arr =
+        (Flex *)::operator new(__builtin_offsetof(Flex, contents) + sizeof(S) * size);
+
+    clang_analyzer_eval(S::c == 0); // expected-warning{{TRUE}}
+
+    new (&arr->contents[0]) S;
+    new (&arr->contents[1]) S;
+    new (&arr->contents[2]) S;
+    new (&arr->contents[3]) S;
+
+    clang_analyzer_eval(S::c == size); // expected-warning{{TRUE}}
+
+    clang_analyzer_eval(arr->contents[0].x == 0); // expected-warning{{TRUE}}
+    clang_analyzer_eval(arr->contents[1].x == 1); // expected-warning{{TRUE}}
+    clang_analyzer_eval(arr->contents[2].x == 2); // expected-warning{{TRUE}}
+    clang_analyzer_eval(arr->contents[3].x == 3); // expected-warning{{TRUE}}
+
+    arr->contents[0].~S();
+    arr->contents[1].~S();
+    arr->contents[2].~S();
+    arr->contents[3].~S();
+
+    ::operator delete(arr);
+
+    clang_analyzer_eval(S::d == size); // expected-warning{{TRUE}}
+}

clang/test/Analysis/zero-size-non-pod-array.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+
+void clang_analyzer_eval(bool);
+
+struct S{
+    static int CtorInvocationCount;
+    static int DtorInvocationCount;
+
+    S(){CtorInvocationCount++;}
+    ~S(){DtorInvocationCount++;}
+};
+
+int S::CtorInvocationCount = 0;
+int S::DtorInvocationCount = 0;
+
+void zeroSizeArrayStack() {
+    S::CtorInvocationCount = 0;
+
+    S arr[0];
+
+    clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+void zeroSizeMultidimensionalArrayStack() {
+    S::CtorInvocationCount = 0;
+    S::DtorInvocationCount = 0;
+
+    {
+        S arr[2][0];
+        S arr2[0][2];
+
+        S arr3[0][2][2];
+        S arr4[2][2][0];
+        S arr5[2][0][2];
+    }
+
+    clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+    clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+void zeroSizeArrayStackInLambda() {
+    S::CtorInvocationCount = 0;
+    S::DtorInvocationCount = 0;
+
+    []{
+        S arr[0];
+    }();    
+    
+    clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+    clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+void zeroSizeArrayHeap() {
+    S::CtorInvocationCount = 0;
+    S::DtorInvocationCount = 0;
+
+    auto *arr = new S[0];
+    delete[] arr;
+    
+    clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+    clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+void zeroSizeMultidimensionalArrayHeap() {
+    S::CtorInvocationCount = 0;
+    S::DtorInvocationCount = 0;
+
+    auto *arr = new S[2][0];
+    delete[] arr;
+        
+    auto *arr2 = new S[0][2];
+    delete[] arr2;
+
+    auto *arr3 = new S[0][2][2];
+    delete[] arr3;
+
+    auto *arr4 = new S[2][2][0];
+    delete[] arr4;
+
+    auto *arr5 = new S[2][0][2];
+    delete[] arr5;
+
+    clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+    clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+#if __cplusplus >= 201703L
+
+void zeroSizeArrayBinding() {
+    S::CtorInvocationCount = 0;
+
+    S arr[0];
+
+    // Note: This is an error in gcc but a warning in clang.
+    // In MSVC the declaration of 'S arr[0]' is already an error
+    // and it doesn't recognize this syntax as a structured binding.
+    auto [] = arr; //expected-warning{{ISO C++17 does not allow a decomposition group to be empty}}
+
+    clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+#endif
+
+void zeroSizeArrayLambdaCapture() {
+    S::CtorInvocationCount = 0;
+    S::DtorInvocationCount = 0;
+        
+    S arr[0];
+
+    auto l = [arr]{};
+    [arr]{}();    
+    
+    //FIXME: These should be TRUE. We should avoid calling the destructor 
+    // of the temporary that is materialized as the lambda.
+    clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}} expected-warning{{FALSE}}
+    clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}} expected-warning{{FALSE}}
+}
+
+// FIXME: Report a warning if the standard is at least C++17.
+#if __cplusplus < 201703L
+void zeroSizeArrayLambdaCaptureUndefined1() {
+    S arr[0];
+    int n;
+
+    auto l = [arr, n]{
+        int x = n; //expected-warning{{Assigned value is garbage or undefined}}
+        (void) x;
+    };
+
+    l();
+}
+#endif
+
+void zeroSizeArrayLambdaCaptureUndefined2() {
+    S arr[0];
+    int n;
+
+    [arr, n]{
+        int x = n; //expected-warning{{Assigned value is garbage or undefined}}
+        (void) x;
+    }();
+}
+
+struct Wrapper{
+    S arr[0];
+};
+
+void zeroSizeArrayMember() {
+    S::CtorInvocationCount = 0;
+    S::DtorInvocationCount = 0;
+
+    {
+        Wrapper W;
+    }
+
+    clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+    clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+void zeroSizeArrayMemberCopyMove() {
+    S::CtorInvocationCount = 0;
+    S::DtorInvocationCount = 0;
+    
+    {
+        Wrapper W;
+        Wrapper W2 = W;
+        Wrapper W3 = (Wrapper&&) W2;
+    }
+
+    clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+    clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+struct MultiWrapper{
+    S arr[2][0];
+};
+
+void zeroSizeMultidimensionalArrayMember() {
+    S::CtorInvocationCount = 0;
+    S::DtorInvocationCount = 0;
+
+    {
+        MultiWrapper MW;
+    }
+
+    clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+    clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}