walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Fix 10314: Possible nullPointerRedundantCheck false positive (#3298)

This commit is contained in:
Paul Fultz II 2021-06-19 06:59:48 -05:00 committed by GitHub
parent 5922d5178b
commit dd178c3ad9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 167 additions and 122 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
lib/analyzer.h
View File

@ -111,6 +111,16 @@ struct Analyzer {
unsigned int mFlag;
};
enum class Terminate { None, Bail, Escape, Modified, Inconclusive, Conditional };
struct Result {
Result(Action action = Action::None, Terminate terminate = Terminate::None)
: action(action), terminate(terminate)
{}
Action action;
Terminate terminate;
};
enum class Direction { Forward, Reverse };
struct Assume {

3
lib/astutils.cpp
View File

@ -1563,7 +1563,8 @@ bool isReturnScope(const Token* const endToken, const Library* library, const To
if (Token::simpleMatch(prev->link()->tokAt(-2), "} else {"))
return isReturnScope(prev, library, unknownFunc, functionScope) &&
isReturnScope(prev->link()->tokAt(-2), library, unknownFunc, functionScope);
if (Token::simpleMatch(prev->link()->previous(), ") {") &&
// TODO: Check all cases
if (!functionScope && Token::simpleMatch(prev->link()->previous(), ") {") &&
Token::simpleMatch(prev->link()->linkAt(-1)->previous(), "switch (") &&
!Token::findsimplematch(prev->link(), "break", prev)) {
return true;

74
lib/forwardanalyzer.cpp
View File

@ -22,11 +22,12 @@ struct ForwardTraversal {
Analyzer::Action actions;
bool analyzeOnly;
bool analyzeTerminate;
Terminate terminate = Terminate::None;
Analyzer::Terminate terminate = Analyzer::Terminate::None;
bool forked = false;
Progress Break(Terminate t = Terminate::None) {
if ((!analyzeOnly || analyzeTerminate) && t != Terminate::None)
Progress Break(Analyzer::Terminate t = Analyzer::Terminate::None)
{
if ((!analyzeOnly || analyzeTerminate) && t != Analyzer::Terminate::None)
terminate = t;
return Progress::Break;
}
@ -89,7 +90,7 @@ struct ForwardTraversal {
else if (Token::Match(tok, "return|throw") || isEscapeFunction(tok, &settings->library)) {
traverseRecursive(tok->astOperand1(), f, traverseUnknown);
traverseRecursive(tok->astOperand2(), f, traverseUnknown);
return Break(Terminate::Escape);
return Break(Analyzer::Terminate::Escape);
} else if (isUnevaluated(tok)) {
if (out)
*out = tok->link();
@ -103,7 +104,7 @@ struct ForwardTraversal {
// Skip lambdas
} else if (T* lambdaEndToken = findLambdaEndToken(tok)) {
if (checkScope(lambdaEndToken).isModified())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
if (out)
*out = lambdaEndToken->next();
// Skip class scope
@ -152,7 +153,7 @@ struct ForwardTraversal {
if (!checkThen && !checkElse) {
// Stop if the value is conditional
if (!traverseUnknown && analyzer->isConditional() && stopUpdates()) {
return Break(Terminate::Conditional);
return Break(Analyzer::Terminate::Conditional);
}
checkThen = true;
checkElse = true;
@ -180,12 +181,12 @@ struct ForwardTraversal {
if (!action.isNone() && !analyzeOnly)
analyzer->update(tok, action, Analyzer::Direction::Forward);
if (action.isInconclusive() && !analyzer->lowerToInconclusive())
return Break(Terminate::Inconclusive);
return Break(Analyzer::Terminate::Inconclusive);
if (action.isInvalid())
return Break(Terminate::Modified);
return Break(Analyzer::Terminate::Modified);
if (action.isWrite() && !action.isRead())
// Analysis of this write will continue separately
return Break(Terminate::Modified);
return Break(Analyzer::Terminate::Modified);
return Progress::Continue;
}
@ -320,13 +321,13 @@ struct ForwardTraversal {
if (!branch.escape && hasInnerReturnScope(branch.endBlock->previous(), branch.endBlock->link())) {
ForwardTraversal ft2 = fork(true);
ft2.updateScope(branch.endBlock);
if (ft2.terminate == Terminate::Escape) {
if (ft2.terminate == Analyzer::Terminate::Escape) {
branch.escape = true;
branch.escapeUnknown = false;
}
}
} else {
if (ft1.front().terminate == Terminate::Escape) {
if (ft1.front().terminate == Analyzer::Terminate::Escape) {
branch.escape = true;
branch.escapeUnknown = false;
}
@ -387,10 +388,10 @@ struct ForwardTraversal {
actions |= allAnalysis;
if (allAnalysis.isInconclusive()) {
if (!analyzer->lowerToInconclusive())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (allAnalysis.isModified()) {
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
}
bool checkThen = true;
bool checkElse = false;
@ -409,9 +410,9 @@ struct ForwardTraversal {
return Break();
// If loop re-enters then it could be modified again
if (allAnalysis.isModified() && reentersLoop(endBlock, condTok, stepTok))
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
if (allAnalysis.isIncremental())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else {
std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, allAnalysis.isModified());
bool forkContinue = true;
@ -425,9 +426,9 @@ struct ForwardTraversal {
if (allAnalysis.isModified() || !forkContinue) {
// TODO: Dont bail on missing condition
if (!condTok)
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
if (analyzer->isConditional() && stopUpdates())
return Break(Terminate::Conditional);
return Break(Analyzer::Terminate::Conditional);
analyzer->assume(condTok, false);
}
if (forkContinue) {
@ -437,7 +438,7 @@ struct ForwardTraversal {
}
}
if (allAnalysis.isIncremental())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
}
return Progress::Continue;
}
@ -451,7 +452,7 @@ struct ForwardTraversal {
Progress updateRange(Token* start, const Token* end, int depth = 20) {
forked = false;
if (depth < 0)
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
std::size_t i = 0;
for (Token* tok = start; tok && tok != end; tok = tok->next()) {
Token* next = nullptr;
@ -485,13 +486,13 @@ struct ForwardTraversal {
return Break();
tok = skipTo(tok, scopeEndToken, end);
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
// TODO: Don't break, instead move to the outer scope
if (!tok)
return Break();
} else if (Token::Match(tok, "%name% :") || tok->str() == "case") {
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (tok->link() && tok->str() == "}") {
const Scope* scope = tok->scope();
if (!scope)
@ -505,7 +506,7 @@ struct ForwardTraversal {
return Break();
if (!condTok->hasKnownIntValue() || inLoop) {
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (condTok->values().front().intvalue == inElse) {
return Break();
}
@ -524,7 +525,7 @@ struct ForwardTraversal {
tok = tok->linkAt(2);
} else if (scope->type == Scope::eTry) {
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (scope->type == Scope::eLambda) {
return Break();
} else if (scope->type == Scope::eDo && Token::simpleMatch(tok, "} while (")) {
@ -595,32 +596,32 @@ struct ForwardTraversal {
return Break();
if (thenBranch.isDead() && elseBranch.isDead()) {
if (thenBranch.isModified() && elseBranch.isModified())
return Break(Terminate::Modified);
return Break(Analyzer::Terminate::Modified);
if (thenBranch.isConclusiveEscape() && elseBranch.isConclusiveEscape())
return Break(Terminate::Escape);
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Escape);
return Break(Analyzer::Terminate::Bail);
}
// Conditional return
if (thenBranch.isEscape() && !hasElse) {
if (!thenBranch.isConclusiveEscape()) {
if (!analyzer->lowerToInconclusive())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (thenBranch.check) {
return Break();
} else {
if (analyzer->isConditional() && stopUpdates())
return Break(Terminate::Conditional);
return Break(Analyzer::Terminate::Conditional);
analyzer->assume(condTok, false);
}
}
if (thenBranch.isInconclusive() || elseBranch.isInconclusive()) {
if (!analyzer->lowerToInconclusive())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
} else if (thenBranch.isModified() || elseBranch.isModified()) {
if (!hasElse && analyzer->isConditional() && stopUpdates())
return Break(Terminate::Conditional);
return Break(Analyzer::Terminate::Conditional);
if (!analyzer->lowerToPossible())
return Break(Terminate::Bail);
return Break(Analyzer::Terminate::Bail);
analyzer->assume(condTok, elseBranch.isModified());
}
}
@ -742,19 +743,16 @@ struct ForwardTraversal {
};
Analyzer::Action valueFlowGenericForward(Token* start,
const Token* end,
const ValuePtr<Analyzer>& a,
const Settings* settings)
Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr<Analyzer>& a, const Settings* settings)
{
ForwardTraversal ft{a, settings};
ft.updateRange(start, end);
return ft.actions;
return {ft.actions, ft.terminate};
}
Analyzer::Action valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings)
Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings)
{
ForwardTraversal ft{a, settings};
ft.updateRecursive(start);
return ft.actions;
return {ft.actions, ft.terminate};
}

10
lib/forwardanalyzer.h
View File

@ -25,11 +25,11 @@ class Settings;
class Token;
template <class T> class ValuePtr;
Analyzer::Action valueFlowGenericForward(Token* start,
const Token* end,
const ValuePtr<Analyzer>& a,
const Settings* settings);
Analyzer::Result valueFlowGenericForward(Token* start,
const Token* end,
const ValuePtr<Analyzer>& a,
const Settings* settings);
Analyzer::Action valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings);
Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr<Analyzer>& a, const Settings* settings);
#endif

169
lib/valueflow.cpp
View File

@ -1664,12 +1664,12 @@ static void valueFlowGlobalStaticVar(TokenList *tokenList, const Settings *setti
}
}
static Analyzer::Action valueFlowForwardVariable(Token* const startToken,
const Token* const endToken,
const Variable* const var,
std::list<ValueFlow::Value> values,
TokenList* const tokenlist,
const Settings* const settings);
static Analyzer::Result valueFlowForwardVariable(Token* const startToken,
const Token* const endToken,
const Variable* const var,
std::list<ValueFlow::Value> values,
TokenList* const tokenlist,
const Settings* const settings);
static void valueFlowReverse(TokenList* tokenlist,
Token* tok,
@ -2407,28 +2407,32 @@ static std::vector<const Variable*> getAliasesFromValues(std::list<ValueFlow::Va
return aliases;
}
static Analyzer::Action valueFlowForwardVariable(Token* const startToken,
const Token* const endToken,
const Variable* const var,
std::list<ValueFlow::Value> values,
std::vector<const Variable*> aliases,
TokenList* const tokenlist,
const Settings* const settings)
static Analyzer::Result valueFlowForwardVariable(Token* const startToken,
const Token* const endToken,
const Variable* const var,
std::list<ValueFlow::Value> values,
std::vector<const Variable*> aliases,
TokenList* const tokenlist,
const Settings* const settings)
{
Analyzer::Action actions;
Analyzer::Terminate terminate = Analyzer::Terminate::None;
for (ValueFlow::Value& v : values) {
VariableAnalyzer a(var, v, aliases, tokenlist);
actions |= valueFlowGenericForward(startToken, endToken, a, settings);
Analyzer::Result r = valueFlowGenericForward(startToken, endToken, a, settings);
actions |= r.action;
if (terminate == Analyzer::Terminate::None)
terminate = r.terminate;
}
return actions;
return {actions, terminate};
}
static Analyzer::Action valueFlowForwardVariable(Token* const startToken,
const Token* const endToken,
const Variable* const var,
std::list<ValueFlow::Value> values,
TokenList* const tokenlist,
const Settings* const settings)
static Analyzer::Result valueFlowForwardVariable(Token* const startToken,
const Token* const endToken,
const Variable* const var,
std::list<ValueFlow::Value> values,
TokenList* const tokenlist,
const Settings* const settings)
{
auto aliases = getAliasesFromValues(values);
return valueFlowForwardVariable(
@ -2527,19 +2531,23 @@ struct OppositeExpressionAnalyzer : ExpressionAnalyzer {
}
};
static Analyzer::Action valueFlowForwardExpression(Token* startToken,
const Token* endToken,
const Token* exprTok,
const std::list<ValueFlow::Value>& values,
const TokenList* const tokenlist,
const Settings* settings)
static Analyzer::Result valueFlowForwardExpression(Token* startToken,
const Token* endToken,
const Token* exprTok,
const std::list<ValueFlow::Value>& values,
const TokenList* const tokenlist,
const Settings* settings)
{
Analyzer::Action actions;
Analyzer::Terminate terminate = Analyzer::Terminate::None;
for (const ValueFlow::Value& v : values) {
ExpressionAnalyzer a(exprTok, v, tokenlist);
actions |= valueFlowGenericForward(startToken, endToken, a, settings);
Analyzer::Result r = valueFlowGenericForward(startToken, endToken, a, settings);
actions |= r.action;
if (terminate == Analyzer::Terminate::None)
terminate = r.terminate;
}
return actions;
return {actions, terminate};
}
static const Token* parseBinaryIntOp(const Token* expr, MathLib::bigint& known)
@ -2614,12 +2622,12 @@ ValuePtr<Analyzer> makeAnalyzer(Token* exprTok, const ValueFlow::Value& value, c
}
}
static Analyzer::Action valueFlowForward(Token* startToken,
const Token* endToken,
const Token* exprTok,
std::list<ValueFlow::Value> values,
TokenList* const tokenlist,
const Settings* settings)
static Analyzer::Result valueFlowForward(Token* startToken,
const Token* endToken,
const Token* exprTok,
std::list<ValueFlow::Value> values,
TokenList* const tokenlist,
const Settings* settings)
{
const Token* expr = solveExprValues(exprTok, values);
if (expr->variable()) {
@ -4285,12 +4293,12 @@ struct ConditionHandler {
Condition() : vartok(nullptr), true_values(), false_values(), inverted(false) {}
};
virtual bool forward(Token* start,
const Token* stop,
const Token* exprTok,
const std::list<ValueFlow::Value>& values,
TokenList* tokenlist,
const Settings* settings) const = 0;
virtual Analyzer::Result forward(Token* start,
const Token* stop,
const Token* exprTok,
const std::list<ValueFlow::Value>& values,
TokenList* tokenlist,
const Settings* settings) const = 0;
virtual void reverse(Token* start,
const Token* endToken,
@ -4577,9 +4585,6 @@ struct ConditionHandler {
return;
}
// start token of conditional code
Token* startTokens[] = {nullptr, nullptr};
// if astParent is "!" we need to invert codeblock
{
const Token* tok2 = tok;
@ -4593,6 +4598,9 @@ struct ConditionHandler {
}
}
bool deadBranch[] = {false, false};
// start token of conditional code
Token* startTokens[] = {nullptr, nullptr};
// determine startToken(s)
if (Token::simpleMatch(top->link(), ") {"))
startTokens[0] = top->link()->next();
@ -4608,12 +4616,13 @@ struct ConditionHandler {
std::list<ValueFlow::Value>& values = (i == 0 ? thenValues : elseValues);
valueFlowSetConditionToKnown(tok, values, i == 0);
// TODO: The endToken should not be startTokens[i]->link() in the valueFlowForwardVariable call
if (forward(startTokens[i], startTokens[i]->link(), cond.vartok, values, tokenlist, settings))
Analyzer::Result r =
forward(startTokens[i], startTokens[i]->link(), cond.vartok, values, tokenlist, settings);
deadBranch[i] = r.terminate == Analyzer::Terminate::Escape;
if (r.action.isModified() && !deadBranch[i])
changeBlock = i;
changeKnownToPossible(values);
}
// TODO: Values changed in noreturn blocks should not bail
if (changeBlock >= 0 && !Token::simpleMatch(top->previous(), "while (")) {
if (settings->debugwarnings)
bailout(tokenlist,
@ -4627,12 +4636,13 @@ struct ConditionHandler {
// After conditional code..
if (Token::simpleMatch(top->link(), ") {")) {
Token* after = top->link()->linkAt(1);
bool dead_if = deadBranch[0];
bool dead_else = deadBranch[1];
const Token* unknownFunction = nullptr;
const bool isWhile =
tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while (");
bool dead_if = (!isBreakScope(after) && isWhile) ||
(isReturnScope(after, &settings->library, &unknownFunction) && !isWhile);
bool dead_else = false;
if (tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while ("))
dead_if = !isBreakScope(after);
else if (!dead_if)
dead_if = isReturnScope(after, &settings->library, &unknownFunction);
if (!dead_if && unknownFunction) {
if (settings->debugwarnings)
@ -4643,7 +4653,8 @@ struct ConditionHandler {
if (Token::simpleMatch(after, "} else {")) {
after = after->linkAt(2);
unknownFunction = nullptr;
dead_else = isReturnScope(after, &settings->library, &unknownFunction);
if (!dead_else)
dead_else = isReturnScope(after, &settings->library, &unknownFunction);
if (!dead_else && unknownFunction) {
if (settings->debugwarnings)
bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope");
@ -4717,13 +4728,14 @@ static void valueFlowCondition(const ValuePtr<ConditionHandler>& handler,
}
struct SimpleConditionHandler : ConditionHandler {
virtual bool forward(Token* start,
const Token* stop,
const Token* exprTok,
const std::list<ValueFlow::Value>& values,
TokenList* tokenlist,
const Settings* settings) const OVERRIDE {
return valueFlowForward(start->next(), stop, exprTok, values, tokenlist, settings).isModified();
virtual Analyzer::Result forward(Token* start,
const Token* stop,
const Token* exprTok,
const std::list<ValueFlow::Value>& values,
TokenList* tokenlist,
const Settings* settings) const OVERRIDE
{
return valueFlowForward(start->next(), stop, exprTok, values, tokenlist, settings);
}
virtual void reverse(Token* start,
@ -6081,19 +6093,19 @@ struct ContainerVariableAnalyzer : VariableAnalyzer {
}
};
static Analyzer::Action valueFlowContainerForward(Token* tok,
const Token* endToken,
const Variable* var,
ValueFlow::Value value,
TokenList* tokenlist)
static Analyzer::Result valueFlowContainerForward(Token* tok,
const Token* endToken,
const Variable* var,
ValueFlow::Value value,
TokenList* tokenlist)
{
ContainerVariableAnalyzer a(var, value, getAliasesFromValues({value}), tokenlist);
return valueFlowGenericForward(tok, endToken, a, tokenlist->getSettings());
}
static Analyzer::Action valueFlowContainerForward(Token* tok,
const Variable* var,
ValueFlow::Value value,
TokenList* tokenlist)
static Analyzer::Result valueFlowContainerForward(Token* tok,
const Variable* var,
ValueFlow::Value value,
TokenList* tokenlist)
{
const Token * endOfVarScope = nullptr;
if (var->isLocal() || var->isArgument())
@ -6506,19 +6518,20 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold
}
struct ContainerConditionHandler : ConditionHandler {
virtual bool forward(Token* start,
const Token* stop,
const Token* exprTok,
const std::list<ValueFlow::Value>& values,
TokenList* tokenlist,
const Settings*) const OVERRIDE {
virtual Analyzer::Result forward(Token* start,
const Token* stop,
const Token* exprTok,
const std::list<ValueFlow::Value>& values,
TokenList* tokenlist,
const Settings*) const OVERRIDE
{
// TODO: Forward multiple values
if (values.empty())
return false;
return {};
const Variable* var = exprTok->variable();
if (!var)
return false;
return valueFlowContainerForward(start->next(), stop, var, values.front(), tokenlist).isModified();
return {};
return valueFlowContainerForward(start->next(), stop, var, values.front(), tokenlist);
}
virtual void reverse(Token* start,

23
test/testvalueflow.cpp
View File

@ -2718,6 +2718,29 @@ private:
" return x;\n"
"}\n";
ASSERT_EQUALS(false, testValueOfXImpossible(code, 4U, 0));
code = "int g(int x) {\n"
" switch (x) {\n"
" case 1:\n"
" return 1;\n"
" default:\n"
" return 2;\n"
" }\n"
"}\n"
"void f(int x) {\n"
" if (x == 3)\n"
" x = g(0);\n"
" int a = x;\n"
"}\n";
ASSERT_EQUALS(false, testValueOfX(code, 12U, 3));
code = "int g(int x) { throw 0; }\n"
"void f(int x) {\n"
" if (x == 3)\n"
" x = g(0);\n"
" int a = x;\n"
"}\n";
ASSERT_EQUALS(true, testValueOfXImpossible(code, 5U, 3));
}
void valueFlowAfterConditionExpr() {