From 3cb252bd99d7b6b95933bfd00d24069e247f79a0 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Wed, 6 Oct 2021 01:39:58 -0500 Subject: [PATCH] Fix 9873: False negative: null pointer when checking raw pointer (#3485) --- lib/astutils.cpp | 12 ++++++ lib/symboldatabase.cpp | 7 +++- lib/valueflow.cpp | 89 +++++++++++++++++++++++++++++++--------- test/testcondition.cpp | 2 +- test/testnullpointer.cpp | 14 +++++++ test/testvarid.cpp | 6 +-- 6 files changed, 105 insertions(+), 25 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 8b5eb8b07..0e39c77b3 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1525,6 +1525,18 @@ bool isConstFunctionCall(const Token* ftok, const Library& library) } else if (lf->argumentChecks.empty()) { return false; } + } else if (Token::Match(ftok->previous(), ". %name% (") && ftok->previous()->originalName() != "->" && + astIsSmartPointer(ftok->previous()->astOperand1())) { + return Token::Match(ftok, "get|get_deleter ( )"); + } else if (Token::Match(ftok->previous(), ". %name% (") && astIsContainer(ftok->previous()->astOperand1())) { + const Library::Container* container = ftok->previous()->astOperand1()->valueType()->container; + if (!container) + return false; + if (container->getYield(ftok->str()) != Library::Container::Yield::NO_YIELD) + return true; + if (container->getAction(ftok->str()) == Library::Container::Action::FIND) + return true; + return false; } else { bool memberFunction = Token::Match(ftok->previous(), ". %name% ("); bool constMember = !memberFunction; diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 587c76ea8..fd68673bd 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -1418,7 +1418,7 @@ void SymbolDatabase::createSymbolDatabaseEscapeFunctions() static bool isExpression(const Token* tok) { - if (!Token::Match(tok, "(|.|[|%cop%")) + if (!Token::Match(tok, "(|.|[|::|?|:|++|--|%cop%|%assign%")) return false; if (Token::Match(tok, "*|&|&&")) { const Token* vartok = findAstNode(tok, [&](const Token* tok2) { @@ -1444,6 +1444,7 @@ void SymbolDatabase::createSymbolDatabaseExprIds() } nonneg int id = base + 1; for (const Scope * scope : functionScopes) { + nonneg int thisId = 0; std::unordered_map> exprs; // Assign IDs @@ -1457,6 +1458,10 @@ void SymbolDatabase::createSymbolDatabaseExprIds() if (id == std::numeric_limits::max()) { throw InternalError(nullptr, "Ran out of expression ids.", InternalError::INTERNAL); } + } else if (isCPP() && Token::simpleMatch(tok, "this")) { + if (thisId == 0) + thisId = id++; + tok->exprId(thisId); } } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index be4b618e8..5ccae4187 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2191,6 +2191,8 @@ struct ValueFlowAnalyzer : Analyzer { { if (!useSymbolicValues()) return false; + if (Token::Match(tok, "%assign%")) + return false; for (const ValueFlow::Value& v : tok->values()) { if (!v.isSymbolicValue()) continue; @@ -2209,7 +2211,8 @@ struct ValueFlowAnalyzer : Analyzer { Action analyzeMatch(const Token* tok, Direction d) const { const Token* parent = tok->astParent(); - if (astIsPointer(tok) && (Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0) + if ((astIsPointer(tok) || astIsSmartPointer(tok)) && + (Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0) return Action::Read; Action w = isWritable(tok, d); @@ -2497,6 +2500,8 @@ struct SingleValueFlowAnalyzer : ValueFlowAnalyzer { return false; if (isConditional() && !value.isKnown() && !value.isImpossible()) return true; + if (value.isSymbolicValue()) + return false; ConditionState cs = analyzeCondition(condTok); return cs.isUnknownDependent(); } @@ -2563,14 +2568,18 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer { if (depth > maxDepth) return; visitAstNodes(start, [&](const Token* tok) { - for (const ValueFlow::Value& v : tok->values()) { - if (!(v.isLocalLifetimeValue() || (astIsPointer(tok) && v.isSymbolicValue() && v.isKnown()))) - continue; - if (!v.tokvalue) - continue; - if (v.tokvalue == tok) - continue; - setupExprVarIds(v.tokvalue, depth + 1); + const bool top = depth == 0 && tok == start; + const bool ispointer = astIsPointer(tok) || astIsSmartPointer(tok); + if (!top || !ispointer || value.indirect != 0) { + for (const ValueFlow::Value& v : tok->values()) { + if (!(v.isLocalLifetimeValue() || (ispointer && v.isSymbolicValue() && v.isKnown()))) + continue; + if (!v.tokvalue) + continue; + if (v.tokvalue == tok) + continue; + setupExprVarIds(v.tokvalue, depth + 1); + } } if (depth == 0 && tok->varId() == 0 && !tok->function() && tok->isName() && tok->previous()->str() != ".") { // unknown variable @@ -4312,6 +4321,7 @@ static bool isTruncated(const ValueType* src, const ValueType* dst, const Settin static void setSymbolic(ValueFlow::Value& value, const Token* tok) { + assert(tok && tok->exprId() > 0 && "Missing expr id for symbolic value"); value.valueType = ValueFlow::Value::ValueType::SYMBOLIC; value.tokvalue = tok; } @@ -4973,6 +4983,22 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat const bool init = vars.size() == 1 && vars.front()->nameToken() == tok->astOperand1(); valueFlowForwardAssign( tok->astOperand2(), tok->astOperand1(), vars, values, init, tokenlist, errorLogger, settings); + // Back propagate symbolic values + if (tok->astOperand1()->exprId() > 0) { + Token* start = nextAfterAstRightmostLeaf(tok); + const Token* end = scope->bodyEnd; + for (ValueFlow::Value value : values) { + if (!value.isSymbolicValue()) + continue; + const Token* expr = value.tokvalue; + value.intvalue = -value.intvalue; + value.tokvalue = tok->astOperand1(); + value.errorPath.emplace_back(tok, + tok->astOperand1()->expressionString() + " is assigned '" + + tok->astOperand2()->expressionString() + "' here."); + valueFlowForward(start, end, expr, {value}, tokenlist, settings); + } + } } } } @@ -5690,6 +5716,8 @@ struct SymbolicConditionHandler : SimpleConditionHandler { const bool lhs = i == 0; const Token* vartok = lhs ? tok->astOperand1() : tok->astOperand2(); const Token* valuetok = lhs ? tok->astOperand2() : tok->astOperand1(); + if (valuetok->exprId() == 0) + continue; if (valuetok->hasKnownSymbolicValue(vartok)) continue; if (vartok->hasKnownSymbolicValue(valuetok)) @@ -6842,6 +6870,17 @@ static bool isContainerSizeChanged(nonneg int varId, return false; } +std::vector getVariables(const Token* tok) +{ + std::vector result; + visitAstNodes(tok, [&](const Token* child) { + if (child->variable()) + result.push_back(child->variable()); + return ChildrenToVisit::op1_and_op2; + }); + return result; +} + static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogger, const Settings *settings) { for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { @@ -6849,7 +6888,9 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge continue; if (!tok->scope()->isExecutable()) continue; - if (tok->variable()) { + if (!astIsSmartPointer(tok)) + continue; + if (tok->variable() && Token::Match(tok, "%var% (|{|;")) { const Variable* var = tok->variable(); if (!var->isSmartPointer()) continue; @@ -6868,27 +6909,32 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge values.push_back(v); valueFlowForwardAssign(tok, var, values, false, true, tokenlist, errorLogger, settings); } - } else if (Token::Match(tok, "%var% . reset (") && tok->next()->originalName() != "->") { - if (Token::simpleMatch(tok->tokAt(3), "( )")) { + } + } else if (astIsLHS(tok) && Token::Match(tok->astParent(), ". %name% (") && + tok->astParent()->originalName() != "->") { + std::vector vars = getVariables(tok); + Token* ftok = tok->astParent()->tokAt(2); + if (Token::simpleMatch(tok->astParent(), ". reset (")) { + if (Token::simpleMatch(ftok, "( )")) { std::list values; ValueFlow::Value v(0); v.setKnown(); values.push_back(v); - valueFlowForwardAssign(tok->tokAt(3), var, values, false, false, tokenlist, errorLogger, settings); + valueFlowForwardAssign(ftok, tok, vars, values, false, tokenlist, errorLogger, settings); } else { tok->removeValues(std::mem_fn(&ValueFlow::Value::isIntValue)); - Token* inTok = tok->tokAt(3)->astOperand2(); + Token* inTok = ftok->astOperand2(); if (!inTok) continue; std::list values = inTok->values(); - const bool constValue = inTok->isNumber(); - valueFlowForwardAssign(inTok, var, values, constValue, false, tokenlist, errorLogger, settings); + valueFlowForwardAssign(inTok, tok, vars, values, false, tokenlist, errorLogger, settings); } - } else if (Token::Match(tok, "%var% . release ( )") && tok->next()->originalName() != "->") { - const Token* parent = tok->tokAt(3)->astParent(); + } else if (Token::simpleMatch(tok->astParent(), ". release ( )")) { + const Token* parent = ftok->astParent(); bool hasParentReset = false; while (parent) { - if (Token::Match(parent->tokAt(-3), "%varid% . release|reset (", tok->varId())) { + if (Token::Match(parent->tokAt(-2), ". release|reset (") && + parent->tokAt(-2)->astOperand1()->exprId() == tok->exprId()) { hasParentReset = true; break; } @@ -6900,7 +6946,10 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge ValueFlow::Value v(0); v.setKnown(); values.push_back(v); - valueFlowForwardAssign(tok->tokAt(3), var, values, false, false, tokenlist, errorLogger, settings); + valueFlowForwardAssign(ftok, tok, vars, values, false, tokenlist, errorLogger, settings); + } else if (Token::simpleMatch(tok->astParent(), ". get ( )")) { + ValueFlow::Value v = makeSymbolic(tok); + setTokenValue(tok->astParent()->tokAt(2), v, settings); } } else if (Token::Match(tok->previous(), "%name%|> (|{") && astIsSmartPointer(tok) && astIsSmartPointer(tok->astOperand1())) { diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 2ef5055be..a46e0b2d2 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -1573,7 +1573,7 @@ private: " ;\n" " }\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:200] -> [test.cpp:200]: (style) Condition 'g' is always true\n", errout.str()); } void incorrectLogicOperator15() { diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 11f33779f..ce26a2046 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -123,6 +123,7 @@ private: TEST_CASE(nullpointer81); // #8724 TEST_CASE(nullpointer82); // #10331 TEST_CASE(nullpointer83); // #9870 + TEST_CASE(nullpointer84); // #9873 TEST_CASE(nullpointer_addressOf); // address of TEST_CASE(nullpointerSwitch); // #2626 TEST_CASE(nullpointer_cast); // #4692 @@ -2511,6 +2512,19 @@ private: ASSERT_EQUALS("[test.cpp:8]: (warning) Possible null pointer dereference: p\n", errout.str()); } + void nullpointer84() // #9873 + { + check("void f(std::unique_ptr P) {\n" + " A *RP = P.get();\n" + " if (!RP) {\n" + " P->foo();\n" + " }\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:4]: (warning) Either the condition '!RP' is redundant or there is possible null pointer dereference: P.\n", + errout.str()); + } + void nullpointer_addressOf() { // address of check("void f() {\n" " struct X *x = 0;\n" diff --git a/test/testvarid.cpp b/test/testvarid.cpp index 27674402f..1b97cabe9 100644 --- a/test/testvarid.cpp +++ b/test/testvarid.cpp @@ -3423,9 +3423,9 @@ private: "2: int x ; int y ;\n" "3: } ;\n" "4: int f ( A a , A b ) {\n" - "5: int x@5 ; x@5 = a@3 .@9 x@6 +@10 b@4 .@11 x@7 ;\n" - "6: int y@8 ; y@8 = b@4 .@11 x@7 +@10 a@3 .@9 x@6 ;\n" - "7: return x@5 +@15 y@8 +@16 a@3 .@17 y@9 +@18 b@4 .@19 y@10 ;\n" + "5: int x@5 ; x@5 =@9 a@3 .@10 x@6 +@11 b@4 .@12 x@7 ;\n" + "6: int y@8 ; y@8 =@13 b@4 .@12 x@7 +@11 a@3 .@10 x@6 ;\n" + "7: return x@5 +@17 y@8 +@18 a@3 .@19 y@9 +@20 b@4 .@21 y@10 ;\n" "8: }\n"; ASSERT_EQUALS(expected, actual);