[analyzer] Fix crash for array-delete of UnknownVal values.

[tomasz-kaminski-sonarsource]

Summary

We now skip the destruction of array elements for delete[] p,
if the value of p is UnknownVal and does not have corresponding region.
This eliminate the crash in getDynamicElementCount on that
region and matches the behavior for deleting the array of
non-constant range.

[isuckatcs]

This eliminate the crash in getDynamicElementCount on that region

I think that if the crash comes from getDynamicElementCount, we should address it there too, so
when the function is called elsewhere under the same circumstances it won't crash again.

What do you think?

[tomasz-kaminski-sonarsource]

In D136671#3882166, @isuckatcs wrote:

This eliminate the crash in getDynamicElementCount on that region

I think that if the crash comes from getDynamicElementCount, we should address it there too, so
when the function is called elsewhere under the same circumstances it won't crash again.

What do you think?

Good point .
I was also wondering about it and went for the current approach or preserving the precondition that the passed region cannot be null, because:

• For other uses of prepareStateForArrayDestruction in case of automatic variables or members, the destroyed should be always known, so this precondition makes sense. In this context delete[] p of unknown pointer is special case, so localized handling made sense.
• This case of ArgR being null was already handled in the code, so extending it seemed to be more consistent.

Does it make sense to you?

[steakhal]

In D136671#3882166, @isuckatcs wrote:

This eliminate the crash in getDynamicElementCount on that region

I think that if the crash comes from getDynamicElementCount, we should address it there too, so
when the function is called elsewhere under the same circumstances it won't crash again.

What do you think?

For me, it makes sense that getDynamicElementCount() is not intended to be called on a null MemRegion. So, from my point of view, the bug is at some callsite.

[isuckatcs]

For me, it makes sense that getDynamicElementCount() is not intended to be called on a null MemRegion.

It would still be helpful to have an assertion on that inside getDynamicElementCount(), so the caller will know it right away.

What I want to say is that it seems there are multiple error prone snippets involved in this crash and I think we should address all
of them, because some might cause another crash somewhere else.

[tomasz-kaminski-sonarsource]

Added assertion.

[isuckatcs]

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

1244

I meant to assert this inside getDynamicElementCount() as that's the function that dereferences the pointer.

DefinedOrUnknownSVal getDynamicElementCount(..., const MemRegion *MR, ...) {
  MR = MR->StripCasts();                                                            <-- crash here if MR is a nullptr

  DefinedOrUnknownSVal Size = getDynamicExtent(State, MR, SVB);
  SVal ElementSize = getElementExtent(ElementTy, SVB);

  SVal ElementCount =
      SVB.evalBinOp(State, BO_Div, Size, ElementSize, SVB.getArrayIndexType());     <-- this could be the origin of 
                                                                                        a crash too

  return ElementCount.castAs<DefinedOrUnknownSVal>();
}

Also SVB.evalBinOp() can technically return an UndefinedVal and if it does that, then ElementCount.castAs<DefinedOrUnknownSVal>() will also cause a crash as it happened in D130974.

Also since this function can return an UnknownSVal, what do you think about returning that if MR is a nullptr instead of stopping execution? Would that behaviour even make sense?

[tomasz-kaminski-sonarsource]

Added assert

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

1244

I meant to assert this inside getDynamicElementCount() as that's the function that dereferences the pointer.

DefinedOrUnknownSVal getDynamicElementCount(..., const MemRegion *MR, ...) {
  MR = MR->StripCasts();                                                            <-- crash here if MR is a nullptr

  DefinedOrUnknownSVal Size = getDynamicExtent(State, MR, SVB);
  SVal ElementSize = getElementExtent(ElementTy, SVB);

  SVal ElementCount =
      SVB.evalBinOp(State, BO_Div, Size, ElementSize, SVB.getArrayIndexType());     <-- this could be the origin of 
                                                                                        a crash too

  return ElementCount.castAs<DefinedOrUnknownSVal>();
}

Also SVB.evalBinOp() can technically return an UndefinedVal and if it does that, then ElementCount.castAs<DefinedOrUnknownSVal>() will also cause a crash as it happened in D130974.

Also since this function can return an UnknownSVal, what do you think about returning that if MR is a nullptr instead of stopping execution? Would that behaviour even make sense?

1244

I meant to assert this inside getDynamicElementCount() as that's the function that dereferences the pointer.

I see. I will add the assert.

Also since this function can return an UnknownSVal, what do you think about returning that if MR is a nullptr instead of stopping execution? Would that behaviour even make sense?

I still believe that passing nullptr as MR to this function is bug on the caller side, and not expected behavior. This is why I would prefer to have to assert here, instead of returning some symbol, as this will hide such bugs.

[isuckatcs]

I have nothing else to add, this patch looks good to me. If the others don't have anything to add either, feel free to commit.

clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp

DefinedOrUnknownSVal getElementExtent(QualType Ty, SValBuilder &SVB) {
   return SVB.makeIntVal(SVB.getContext().getTypeSizeInChars(Ty).getQuantity(),
                         SVB.getArrayIndexType());
 }
 
 DefinedOrUnknownSVal getDynamicElementCount(ProgramStateRef State,
                                             const MemRegion *MR,
                                             SValBuilder &SVB,
                                             QualType ElementTy) {
+  assert(MR != nullptr && "Not-null region expected");
   MR = MR->StripCasts();
 
   DefinedOrUnknownSVal Size = getDynamicExtent(State, MR, SVB);
   SVal ElementSize = getElementExtent(ElementTy, SVB);
 
   SVal ElementCount =
       SVB.evalBinOp(State, BO_Div, Size, ElementSize, SVB.getArrayIndexType());
 

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

 }
 
 std::pair<ProgramStateRef, uint64_t>
 ExprEngine::prepareStateForArrayDestruction(const ProgramStateRef State,
                                             const MemRegion *Region,
                                             const QualType &ElementTy,
                                             const LocationContext *LCtx,
                                             SVal *ElementCountVal) {
+  assert(Region != nullptr && "Not-null region expected");  
 
   QualType Ty = ElementTy.getDesugaredType(getContext());
   while (const auto *NTy = dyn_cast<ArrayType>(Ty))
     Ty = NTy->getElementType().getDesugaredType(getContext());
 
   auto ElementCount = getDynamicElementCount(State, Region, svalBuilder, Ty);
 
   if (ElementCountVal)
auto getDtorDecl = [](const QualType &DTy) {
     return RD->getDestructor();
   };
 
   unsigned Idx = 0;
   EvalCallOptions CallOpts;
   const MemRegion *ArgR = ArgVal.getAsRegion();
 
   if (DE->isArrayForm()) {
-    SVal ElementCount;
-    std::tie(State, Idx) =
-        prepareStateForArrayDestruction(State, ArgR, DTy, LCtx, &ElementCount);
-
     CallOpts.IsArrayCtorOrDtor = true;
     // Yes, it may even be a multi-dimensional array.
     while (const auto *AT = getContext().getAsArrayType(DTy))
       DTy = AT->getElementType();
 
+    if (ArgR) {
+      SVal ElementCount;
+      std::tie(State, Idx) = prepareStateForArrayDestruction(
+          State, ArgR, DTy, LCtx, &ElementCount);
+
       // If we're about to destruct a 0 length array, don't run any of the
       // destructors.
       if (ElementCount.isConstant() &&
           ElementCount.getAsInteger()->getLimitedValue() == 0) {
 
         static SimpleProgramPointTag PT(
             "ExprEngine", "Skipping 0 length array delete destruction");
         PostImplicitCall PP(getDtorDecl(DTy), DE->getBeginLoc(), LCtx, &PT);
         NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
         Bldr.generateNode(PP, Pred->getState(), Pred);
         return;
       }
 
-    if (ArgR)
       ArgR = State->getLValue(DTy, svalBuilder.makeArrayIndex(Idx), ArgVal)
                  .getAsRegion();
     }
+  }
 
   NodeBuilder Bldr(Pred, Dst, getBuilderContext());
   static SimpleProgramPointTag PT("ExprEngine",
                                   "Prepare for object destruction");
   PreImplicitCall PP(getDtorDecl(DTy), DE->getBeginLoc(), LCtx, &PT);
   Pred = Bldr.generateNode(PP, State, Pred);
 
   if (!Pred)

clang/test/Analysis/dtor-array.cpp

void nonConstantRegionExtent(){
   InlineDtor *arr = new InlineDtor[x];
   clang_analyzer_dumpElementCount(arr); // expected-warning {{conj_$0}}
   delete [] arr;
 
   //FIXME: This should be TRUE but memset also sets this
   // region to a conjured symbol.
   clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}}
 }
+
+namespace crash6 {
+
+struct NonTrivialItem {
+  ~NonTrivialItem();
+};
+
+struct WeirdVec {
+  void clear() {
+    delete[] data;
+    size = 0;
+  }
+  NonTrivialItem *data;
+  unsigned size;
+};
+
+void top(int j) {
+  WeirdVec *p = new WeirdVec;
+
+  p[j].size = 0;
+  delete[] p->data; // no-crash
+}
+
+template <typename T>
+T make_unknown() {
+  return reinterpret_cast<T>(static_cast<int>(0.404));
+}
+
+void directUnknownSymbol() {
+  delete[] make_unknown<NonTrivialItem*>(); // no-crash
+}
+
+}