[clang] aa12a48 - [analyzer] Fix assertion failure with conflicting prototype calls
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 26 02:27:31 PDT 2022
Author: Balazs Benics
Date: 2022-10-26T11:27:01+02:00
New Revision: aa12a48c8223aafafa45fb1e6e9ea49dc18a62d2
URL: https://github.com/llvm/llvm-project/commit/aa12a48c8223aafafa45fb1e6e9ea49dc18a62d2
DIFF: https://github.com/llvm/llvm-project/commit/aa12a48c8223aafafa45fb1e6e9ea49dc18a62d2.diff
LOG: [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
Added:
Modified:
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
clang/test/Analysis/region-store.c
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index a8b49adfb4c9a..94ab3245e47b5 100644
--- 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 86b8c27991f80..568bcd719a707 100644
--- 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 at +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 at +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
+}
More information about the cfe-commits
mailing list