[analyzer] Fix for the crash in #56873

[isuckatcs]

Summary

In ExprEngine::bindReturnValue() we cast an SVal to DefinedOrUnknownSVal,
however this SVal can also be Undefined, which leads to an assertion failure.

Fixes: #56873

[martong]

Thanks, LGTM!

clang/test/Analysis/Issue56873.cpp

13

I think, MallocChecker (or another checker?) should issue a warning if the extent is undefined. Do you plan to address that?

[isuckatcs]

clang/test/Analysis/Issue56873.cpp

13

I agree that we should warn in this case, though I don't plan to address this in the near future. I keep this idea in mind, however if you or someone else wants to look into it, feel free to do so.

[steakhal]

Some checker should have caught the uninitialized value earlier than the defaultEvalCall().
I guess, the MallocCkecher could have checked for it in PreStmt<CXXNewExpr>.
Or alternatively, the CallAndMessageChecker::preCall() already does something like this in the PreVisitProcessArg(). I know that CXXNewExpr is not a call, but you get the idea.
WDYT, worth catching it?

Other than that, I think it's a good practice to not rely on some checkers to catch things to prevent crashes; so this 'fix' seems reasonable to me.

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

768

I'm not a fan of mutating values like this.
Alternatively we could have used something like this at the point of use:
Size.getAs<DefinedOrUnknownSVal>().getValueOr(UnknownVal{})
I'm not sure if it's more readable :D

[isuckatcs]

Some checker should have caught the uninitialized value earlier than the defaultEvalCall().
I guess, the MallocCkecher could have checked for it in PreStmt<CXXNewExpr>.
Or alternatively, the CallAndMessageChecker::preCall() already does something like this in the PreVisitProcessArg(). I know that CXXNewExpr is not a call, but you get the idea.
WDYT, worth catching it?

I definitely think it's worth catching it. I'm working on a checker which addresses this in D131299. It was originally intended to be a part of MallocChecker but has been moved to a separate one.

[steakhal]

In D130974#3709502, @isuckatcs wrote:

Some checker should have caught the uninitialized value earlier than the defaultEvalCall().
I guess, the MallocCkecher could have checked for it in PreStmt<CXXNewExpr>.
Or alternatively, the CallAndMessageChecker::preCall() already does something like this in the PreVisitProcessArg(). I know that CXXNewExpr is not a call, but you get the idea.
WDYT, worth catching it?

I definitely think it's worth catching it. I'm working on a checker which addresses this in D131299. It was originally intended to be a part of MallocChecker but has been moved to a separate one.

If so, shouldn't be some dependencies across these revisions? You could also specify an additional RUN line to demonstrate that this can be caught by an experimental configuration.

[isuckatcs]

If so, shouldn't be some dependencies across these revisions?

I don't think they are that closely related.

This patch is about fixing an assertion failure. This assertion failure happens because we don't handle a case not because the checker doesn't exist.
Also the checker alone wouldn't solve the problem, because if it's turned off, the same crash will happen.

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

if (CNE && CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
       }
 
       SVal ElementSize = getElementExtent(CNE->getAllocatedType(), svalBuilder);
 
       SVal Size =
           svalBuilder.evalBinOp(State, BO_Mul, ElementCount, ElementSize,
                                 svalBuilder.getArrayIndexType());
 
+      // FIXME: This line is to prevent a crash. For more details please check
+      // issue #56264.
+      if (Size.isUndef())
+        Size = UnknownVal();
+
       State = setDynamicExtent(State, MR, Size.castAs<DefinedOrUnknownSVal>(),
                                svalBuilder);
     } else {
       R = svalBuilder.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
     }
   }
   return State->BindExpr(E, LCtx, R);
 }

clang/test/Analysis/Issue56873.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_warnIfReached();
+
+struct S {
+};
+
+void Issue56873_1() {
+    int n;
+
+    // This line used to crash
+    S *arr = new S[n];
+    
+    clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
+}
+
+void Issue56873_2() {
+    int n;
+
+    // This line used to crash
+    int *arr = new int[n];
+    
+    clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
+}