Skip to content

Commit d30f42e

Browse files
pfultz2pfultz2
authored
Fix FP when calling a function in a condition (cppcheck-opensource#3412)
1 parent e4f0096 commit d30f42e

5 files changed

Lines changed: 141 additions & 4 deletions

File tree

‎lib/analyzer.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,8 @@ struct Analyzer {
169169
virtual void forkScope(const Token* /*endBlock*/) {}
170170
/// If the value is conditional
171171
virtual bool isConditional() const = 0;
172+
/// If analysis should stop on the condition
173+
virtual bool stopOnCondition(const Token* condTok) const = 0;
172174
/// The condition that will be assumed during analysis
173175
virtual void assume(const Token* tok, bool state, unsigned int flags = 0) = 0;
174176
/// Return analyzer for expression at token

‎lib/forwardanalyzer.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,9 +151,8 @@ struct ForwardTraversal {
151151
bool checkThen, checkElse;
152152
std::tie(checkThen, checkElse) = evalCond(condTok);
153153
if (!checkThen && !checkElse) {
154-
// Stop if the value is conditional
155-
if (!traverseUnknown && analyzer->isConditional() && stopUpdates()) {
156-
return Break(Analyzer::Terminate::Conditional);
154+
if (!traverseUnknown && analyzer->stopOnCondition(condTok) && stopUpdates()) {
155+
return Progress::Continue;
157156
}
158157
checkThen = true;
159158
checkElse = true;
@@ -593,6 +592,8 @@ struct ForwardTraversal {
593592
Branch elseBranch{endBlock->tokAt(2) ? endBlock->linkAt(2) : nullptr};
594593
// Check if condition is true or false
595594
std::tie(thenBranch.check, elseBranch.check) = evalCond(condTok);
595+
if (!thenBranch.check && !elseBranch.check && analyzer->stopOnCondition(condTok) && stopUpdates())
596+
return Break(Analyzer::Terminate::Conditional);
596597
bool hasElse = Token::simpleMatch(endBlock, "} else {");
597598
bool bail = false;
598599

‎lib/valueflow.cpp‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@
114114
#include <string>
115115
#include <tuple>
116116
#include <type_traits>
117+
#include <unordered_set>
117118
#include <vector>
118119

119120
static void bailoutInternal(const std::string& type, TokenList *tokenlist, ErrorLogger *errorLogger, const Token *tok, const std::string &what, const std::string &file, int line, std::string function)
@@ -1996,6 +1997,103 @@ struct ValueFlowAnalyzer : Analyzer {
19961997
return tokenlist->getSettings();
19971998
}
19981999

2000+
struct ConditionState {
2001+
bool dependent = true;
2002+
bool unknown = true;
2003+
2004+
bool isUnknownDependent() const {
2005+
return unknown && dependent;
2006+
}
2007+
};
2008+
2009+
std::unordered_map<nonneg int, const Token*> getSymbols(const Token* tok) const
2010+
{
2011+
std::unordered_map<nonneg int, const Token*> result;
2012+
if (!tok)
2013+
return result;
2014+
for (const ValueFlow::Value& v : tok->values()) {
2015+
if (!v.isSymbolicValue())
2016+
continue;
2017+
if (v.isImpossible())
2018+
continue;
2019+
if (!v.tokvalue)
2020+
continue;
2021+
if (v.tokvalue->exprId() == 0)
2022+
continue;
2023+
if (match(v.tokvalue))
2024+
continue;
2025+
result[v.tokvalue->exprId()] = v.tokvalue;
2026+
}
2027+
return result;
2028+
}
2029+
2030+
ConditionState analyzeCondition(const Token* tok, int depth = 20) const
2031+
{
2032+
ConditionState result;
2033+
if (!tok)
2034+
return result;
2035+
if (depth < 0)
2036+
return result;
2037+
depth--;
2038+
if (analyze(tok, Direction::Forward).isRead()) {
2039+
result.dependent = true;
2040+
result.unknown = false;
2041+
return result;
2042+
} else if (tok->hasKnownIntValue() || tok->isLiteral()) {
2043+
result.dependent = false;
2044+
result.unknown = false;
2045+
return result;
2046+
} else if (Token::Match(tok, "%cop%")) {
2047+
if (isLikelyStream(isCPP(), tok->astOperand1())) {
2048+
result.dependent = false;
2049+
return result;
2050+
}
2051+
ConditionState lhs = analyzeCondition(tok->astOperand1(), depth - 1);
2052+
if (lhs.isUnknownDependent())
2053+
return lhs;
2054+
ConditionState rhs = analyzeCondition(tok->astOperand2(), depth - 1);
2055+
if (rhs.isUnknownDependent())
2056+
return rhs;
2057+
if (Token::Match(tok, "%comp%"))
2058+
result.dependent = lhs.dependent && rhs.dependent;
2059+
else
2060+
result.dependent = lhs.dependent || rhs.dependent;
2061+
result.unknown = lhs.unknown || rhs.unknown;
2062+
return result;
2063+
} else if (Token::Match(tok->previous(), "%name% (")) {
2064+
std::vector<const Token*> args = getArguments(tok->previous());
2065+
if (Token::Match(tok->tokAt(-2), ". %name% (")) {
2066+
args.push_back(tok->tokAt(-2)->astOperand1());
2067+
}
2068+
result.dependent = std::any_of(args.begin(), args.end(), [&](const Token* arg) {
2069+
ConditionState cs = analyzeCondition(arg, depth - 1);
2070+
return cs.dependent;
2071+
});
2072+
if (result.dependent) {
2073+
// Check if we can evaluate the function
2074+
if (!evaluate(Evaluate::Integral, tok).empty())
2075+
result.unknown = false;
2076+
}
2077+
return result;
2078+
} else {
2079+
std::unordered_map<nonneg int, const Token*> symbols = getSymbols(tok);
2080+
result.dependent = false;
2081+
for (auto&& p : symbols) {
2082+
const Token* arg = p.second;
2083+
ConditionState cs = analyzeCondition(arg, depth - 1);
2084+
result.dependent = cs.dependent;
2085+
if (result.dependent)
2086+
break;
2087+
}
2088+
if (result.dependent) {
2089+
// Check if we can evaluate the token
2090+
if (!evaluate(Evaluate::Integral, tok).empty())
2091+
result.unknown = false;
2092+
}
2093+
return result;
2094+
}
2095+
}
2096+
19992097
virtual Action isModified(const Token* tok) const {
20002098
Action read = Action::Read;
20012099
bool inconclusive = false;
@@ -2411,6 +2509,18 @@ struct SingleValueFlowAnalyzer : ValueFlowAnalyzer {
24112509
return false;
24122510
}
24132511

2512+
virtual bool stopOnCondition(const Token* condTok) const OVERRIDE
2513+
{
2514+
if (value.isNonValue())
2515+
return false;
2516+
if (value.isImpossible())
2517+
return false;
2518+
if (isConditional() && !value.isKnown() && !value.isImpossible())
2519+
return true;
2520+
ConditionState cs = analyzeCondition(condTok);
2521+
return cs.isUnknownDependent();
2522+
}
2523+
24142524
virtual bool updateScope(const Token* endBlock, bool) const OVERRIDE {
24152525
const Scope* scope = endBlock->scope();
24162526
if (!scope)
@@ -5807,6 +5917,10 @@ struct MultiValueFlowAnalyzer : ValueFlowAnalyzer {
58075917
return false;
58085918
}
58095919

5920+
virtual bool stopOnCondition(const Token*) const OVERRIDE {
5921+
return isConditional();
5922+
}
5923+
58105924
virtual bool updateScope(const Token* endBlock, bool) const OVERRIDE {
58115925
const Scope* scope = endBlock->scope();
58125926
if (!scope)

‎test/cfg/gtk.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -400,8 +400,9 @@ void g_once_init_enter_leave_test()
400400
gsize * init_val3 = NULL;
401401
// cppcheck-suppress nullPointer
402402
if (g_once_init_enter(init_val3)) {
403+
gsize* init_val31 = NULL;
403404
// cppcheck-suppress nullPointer
404-
g_once_init_leave(init_val3, 1);
405+
g_once_init_leave(init_val31, 1);
405406
}
406407

407408
gsize * init_val4;

‎test/testnullpointer.cpp‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ class TestNullPointer : public TestFixture {
116116
TEST_CASE(nullpointer74);
117117
TEST_CASE(nullpointer75);
118118
TEST_CASE(nullpointer76); // #10408
119+
TEST_CASE(nullpointer77);
119120
TEST_CASE(nullpointer_addressOf); // address of
120121
TEST_CASE(nullpointerSwitch); // #2626
121122
TEST_CASE(nullpointer_cast); // #4692
@@ -2377,6 +2378,24 @@ class TestNullPointer : public TestFixture {
23772378
ASSERT_EQUALS("", errout.str());
23782379
}
23792380

2381+
void nullpointer77()
2382+
{
2383+
check("bool h(int*);\n"
2384+
"void f(int* i) {\n"
2385+
" int* i = nullptr;\n"
2386+
" if (h(i) && *i == 1) {}\n"
2387+
"}\n");
2388+
ASSERT_EQUALS("", errout.str());
2389+
2390+
check("bool h(int*);\n"
2391+
"void f(int* i) {\n"
2392+
" int* i = nullptr;\n"
2393+
" if (h(i))\n"
2394+
" if (*i == 1) {}\n"
2395+
"}\n");
2396+
ASSERT_EQUALS("", errout.str());
2397+
}
2398+
23802399
void nullpointer_addressOf() { // address of
23812400
check("void f() {\n"
23822401
" struct X *x = 0;\n"

0 commit comments

Comments
 (0)