[analyzer] Fix assertion failure with conflicting prototype calls

It turns out we can reach the `Init.castAs<nonlock::CompoundVal>()`
expression with other kinds of SVals. Such as by `nonloc::ConcreteInt`
in this example: https://godbolt.org/z/s4fdxrcs9

```lang=C++
int buffer[10];
void b();
void top() {
  b(&buffer);
}
void b(int *c) {
  *c = 42; // would crash
}
```
In this example, we try to store `42` to the `Elem{buffer, 0}`.

This situation can appear if the CallExpr refers to a function
declaration without prototype. In such cases, the engine will pick the
redecl of the referred function decl which has function body, hence has
a function prototype.

This weird situation will have an interesting effect to the AST, such as
the argument at the callsite will miss a cast, which would cast the
`int (*)[10]` expression into `int *`, which means that when we evaluate
the `*c = 42` expression, we want to bind `42` to an array, causing the
crash.

Look at the AST of the callsite with and without the function prototype:
https://godbolt.org/z/Gncebcbdb
The only difference is that without the proper function prototype, we
will not have the `ImplicitCastExpr` `BitCasting` from `int (*)[10]`
to `int *` to match the expected type of the parameter declaration.

In this patch, I'm proposing to emit a cast in the mentioned edge-case,
to bind the argument value of the expected type to the parameter.

I'm only proposing this if the runtime definition has exactly the same
number of parameters as the callsite feeds it by arguments.
If that's not the case, I believe, we are better off by binding `Unknown`
to those parameters.

Reviewed By: martong

Differential Revision: https://reviews.llvm.org/D136162

diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index a8b49adfb4c9a61c196e8f7a86310c529fc830e0..94ab3245e47b5260301ad064ecf45def6d5faab1 100644 (file)
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -424,6 +424,38 @@ static SVal processArgument(SVal Value, const Expr *ArgumentExpr,
   return Value;
 }
 
+/// Cast the argument value to the type of the parameter at the function
+/// declaration.
+/// Returns the argument value if it didn't need a cast.
+/// Or returns the cast argument if it needed a cast.
+/// Or returns 'Unknown' if it would need a cast but the callsite and the
+/// runtime definition don't match in terms of argument and parameter count.
+static SVal castArgToParamTypeIfNeeded(const CallEvent &Call, unsigned ArgIdx,
+                                       SVal ArgVal, SValBuilder &SVB) {
+  const FunctionDecl *RTDecl =
+      Call.getRuntimeDefinition().getDecl()->getAsFunction();
+  const auto *CallExprDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+
+  if (!RTDecl || !CallExprDecl)
+    return ArgVal;
+
+  // The function decl of the Call (in the AST) will not have any parameter
+  // declarations, if it was 'only' declared without a prototype. However, the
+  // engine will find the appropriate runtime definition - basically a
+  // redeclaration, which has a function body (and a function prototype).
+  if (CallExprDecl->hasPrototype() || !RTDecl->hasPrototype())
+    return ArgVal;
+
+  // Only do this cast if the number arguments at the callsite matches with
+  // the parameters at the runtime definition.
+  if (Call.getNumArgs() != RTDecl->getNumParams())
+    return UnknownVal();
+
+  const Expr *ArgExpr = Call.getArgExpr(ArgIdx);
+  const ParmVarDecl *Param = RTDecl->getParamDecl(ArgIdx);
+  return SVB.evalCast(ArgVal, Param->getType(), ArgExpr->getType());
+}
+
 static void addParameterValuesToBindings(const StackFrameContext *CalleeCtx,
                                          CallEvent::BindingsTy &Bindings,
                                          SValBuilder &SVB,
@@ -449,12 +481,18 @@ static void addParameterValuesToBindings(const StackFrameContext *CalleeCtx,
     // which makes getArgSVal() fail and return UnknownVal.
     SVal ArgVal = Call.getArgSVal(Idx);
     const Expr *ArgExpr = Call.getArgExpr(Idx);
-    if (!ArgVal.isUnknown()) {
-      Loc ParamLoc = SVB.makeLoc(
-          MRMgr.getParamVarRegion(Call.getOriginExpr(), Idx, CalleeCtx));
-      Bindings.push_back(
-          std::make_pair(ParamLoc, processArgument(ArgVal, ArgExpr, *I, SVB)));
-    }
+
+    if (ArgVal.isUnknown())
+      continue;
+
+    // Cast the argument value to match the type of the parameter in some
+    // edge-cases.
+    ArgVal = castArgToParamTypeIfNeeded(Call, Idx, ArgVal, SVB);
+
+    Loc ParamLoc = SVB.makeLoc(
+        MRMgr.getParamVarRegion(Call.getOriginExpr(), Idx, CalleeCtx));
+    Bindings.push_back(
+        std::make_pair(ParamLoc, processArgument(ArgVal, ArgExpr, *I, SVB)));
   }
 
   // FIXME: Variadic arguments are not handled at all right now.

diff --git a/clang/test/Analysis/region-store.c b/clang/test/Analysis/region-store.c
index 86b8c27991f80a9ec9d62c1bec545bd39fc963c2..568bcd719a7075802d6a5b92e59fb92efd441941 100644 (file)
--- a/clang/test/Analysis/region-store.c
+++ b/clang/test/Analysis/region-store.c
@@ -1,7 +1,12 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection \
+// RUN:    -verify -analyzer-config eagerly-assume=false -std=c99 %s \
+// RUN:    -Wno-implicit-function-declaration
 
 int printf(const char *restrict,...);
 
+void clang_analyzer_eval(int);
+void clang_analyzer_dump(int*);
+
 // Testing core functionality of the region store.
 // radar://10127782
 int compoundLiteralTest(void) {
@@ -39,7 +44,6 @@ int structOffsetBindingIsInvalidated(int length, int i){
   return l.mem; // no-warning
 }
 
-void clang_analyzer_eval(int);
 void testConstraintOnRegionOffset(int *values, int length, int i){
   if (values[1] == 4) {
     values[i] = 5;
@@ -54,3 +58,24 @@ void testConstraintOnRegionOffsetStack(int *values, int length, int i) {
     clang_analyzer_eval(values[0] == 4);// expected-warning {{UNKNOWN}}
   }
 }
+
+int buffer[10];
+void b(); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C and is treated as a zero-parameter prototype in C2x, conflicting with a subsequent definition}}
+void missingPrototypeCallsiteMatchingArgsAndParams() {
+  // expected-warning@+1 {{passing arguments to 'b' without a prototype is deprecated in all versions of C and is not supported in C2x}}
+  b(&buffer);
+}
+void b(int *c) { // expected-note {{conflicting prototype is here}}
+  clang_analyzer_dump(c); // expected-warning {{&Element{buffer,0 S64b,int}}}
+  *c = 42; // no-crash
+}
+
+void c(); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C and is treated as a zero-parameter prototype in C2x, conflicting with a subsequent definition}}
+void missingPrototypeCallsiteMismatchingArgsAndParams() {
+  // expected-warning@+1 {{passing arguments to 'c' without a prototype is deprecated in all versions of C and is not supported in C2x}}
+  c(&buffer, &buffer);
+}
+void c(int *c) { // expected-note {{conflicting prototype is here}}
+  clang_analyzer_dump(c); // expected-warning {{Unknown}}
+  *c = 42; // no-crash
+}