diff --git a/lib/astutils.cpp b/lib/astutils.cpp index e2dbeca2f..c2a3403fb 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1442,9 +1442,16 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, if (!tok) return false; const Token *tok2 = tok; - while (Token::simpleMatch(tok2->astParent(), "*") || (Token::simpleMatch(tok2->astParent(), ".") && !Token::simpleMatch(tok2->astParent()->astParent(), "(")) || - (Token::simpleMatch(tok2->astParent(), "[") && tok2 == tok2->astParent()->astOperand1())) + int derefs = 0; + while (Token::simpleMatch(tok2->astParent(), "*") || + (Token::simpleMatch(tok2->astParent(), ".") && !Token::simpleMatch(tok2->astParent()->astParent(), "(")) || + (Token::simpleMatch(tok2->astParent(), "[") && tok2 == tok2->astParent()->astOperand1())) { + if (Token::simpleMatch(tok2->astParent(), "*") || tok2->astParent()->originalName() == "->") + derefs++; + if (derefs > indirect) + break; tok2 = tok2->astParent(); + } while (Token::simpleMatch(tok2->astParent(), "?") || (Token::simpleMatch(tok2->astParent(), ":") && Token::simpleMatch(tok2->astParent()->astParent(), "?"))) tok2 = tok2->astParent(); @@ -1526,6 +1533,11 @@ bool isVariableChanged(const Token *start, const Token *end, const nonneg int va return findVariableChanged(start, end, 0, varid, globalvar, settings, cpp, depth) != nullptr; } +bool isVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth) +{ + return findVariableChanged(start, end, indirect, varid, globalvar, settings, cpp, depth) != nullptr; +} + Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth) { if (!precedes(start, end)) diff --git a/lib/astutils.h b/lib/astutils.h index 2e8bdef42..56212985d 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -183,6 +183,7 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti /** Is variable changed in block of code? */ bool isVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); +bool isVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, bool cpp, int depth = 20); diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index c7d5508f6..889b22569 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -274,7 +274,9 @@ void CheckAutoVariables::autoVariables() errorInvalidDeallocation(tok, nullptr); else if (tok && tok->variable() && tok->variable()->isPointer()) { for (const ValueFlow::Value &v : tok->values()) { - if (v.isTokValue() && isArrayVar(v.tokvalue)) { + if (!(v.isTokValue())) + continue; + if (isArrayVar(v.tokvalue)) { errorInvalidDeallocation(tok, &v); break; } diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index fd4b2b471..7b17dba2a 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1119,7 +1119,7 @@ void CheckCondition::checkIncorrectLogicOperator() // Comparison #1 (LHS) const Token *comp1 = tok->astOperand1(); - if (comp1 && comp1->str() == tok->str()) + if (comp1->str() == tok->str()) comp1 = comp1->astOperand2(); // Comparison #2 (RHS) diff --git a/lib/ctu.cpp b/lib/ctu.cpp index 38996dd73..fa9ffda39 100644 --- a/lib/ctu.cpp +++ b/lib/ctu.cpp @@ -424,7 +424,10 @@ static std::list> getUnsafeFunction(co tok2 = tok2->linkAt(1); if (Token::findmatch(tok2->link(), "return|throw", tok2)) return ret; - if (isVariableChanged(tok2->link(), tok2, argvar->declarationId(), false, settings, tokenizer->isCPP())) + int indirect = 0; + if(argvar->valueType()) + indirect = argvar->valueType()->pointer; + if (isVariableChanged(tok2->link(), tok2, indirect, argvar->declarationId(), false, settings, tokenizer->isCPP())) return ret; } if (Token::Match(tok2, "%oror%|&&|?")) { diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index c830e76c4..031e9aa28 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1256,7 +1256,9 @@ static void valueFlowPointerAliasDeref(TokenList *tokenlist) if (!var->isConst() && isVariableChanged(lifeTok->next(), tok, lifeTok->varId(), !var->isLocal(), tokenlist->getSettings(), tokenlist->isCPP())) continue; for (const ValueFlow::Value& v:lifeTok->values()) { - if (v.isLifetimeValue()) + // TODO: Move container size values to generic forward + // Forward uninit values since not all values can be forwarded directly + if (!(v.isContainerSizeValue() || v.isUninitValue())) continue; ValueFlow::Value value = v; value.errorPath.insert(value.errorPath.begin(), errorPath.begin(), errorPath.end()); @@ -2267,6 +2269,9 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer { virtual ProgramState getProgramState() const = 0; + virtual const ValueType* getValueType(const Token*) const { + return nullptr; + } virtual int getIndirect(const Token* tok) const { const ValueFlow::Value* value = getValue(tok); if (value) @@ -2314,8 +2319,12 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer { virtual Action isAliasModified(const Token* tok) const { int indirect = 0; + int baseIndirect = 0; + const ValueType* vt = getValueType(tok); + if (vt) + baseIndirect = vt->pointer; if (tok->valueType()) - indirect = tok->valueType()->pointer; + indirect = std::max(0, tok->valueType()->pointer - baseIndirect); if (isVariableChanged(tok, indirect, getSettings(), isCPP())) return Action::Invalid; return Action::None; @@ -2350,6 +2359,25 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer { } // Check for modifications by function calls return isModified(tok); + } else if (tok->isUnaryOp("*")) { + const Token* lifeTok = nullptr; + for (const ValueFlow::Value& v:tok->astOperand1()->values()) { + if (!v.isLocalLifetimeValue()) + continue; + if (lifeTok) + return Action::None; + lifeTok = v.tokvalue; + } + if (lifeTok && match(lifeTok)) { + Action a = Action::Read; + if (isModified(tok).isModified()) + a = Action::Invalid; + if (Token::Match(tok->astParent(), "%assign%") && astIsLHS(tok)) + a |= Action::Read; + return a; + } + return Action::None; + } else if (isAlias(tok)) { return isAliasModified(tok); } else if (Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) { @@ -2425,6 +2453,7 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer { struct SingleValueFlowForwardAnalyzer : ValueFlowForwardAnalyzer { std::unordered_map varids; + std::unordered_map aliases; ValueFlow::Value value; SingleValueFlowForwardAnalyzer() @@ -2439,6 +2468,10 @@ struct SingleValueFlowForwardAnalyzer : ValueFlowForwardAnalyzer { return varids; } + const std::unordered_map& getAliasedVars() const { + return aliases; + } + virtual const ValueFlow::Value* getValue(const Token*) const OVERRIDE { return &value; } @@ -2457,13 +2490,15 @@ struct SingleValueFlowForwardAnalyzer : ValueFlowForwardAnalyzer { virtual bool isAlias(const Token* tok) const OVERRIDE { if (value.isLifetimeValue()) return false; - for (const auto& p:getVars()) { - nonneg int varid = p.first; - const Variable* var = p.second; - if (tok->varId() == varid) - return true; - if (isAliasOf(var, tok, varid, value)) - return true; + for(const auto& m:{std::ref(getVars()), std::ref(getAliasedVars())}) { + for (const auto& p:m.get()) { + nonneg int varid = p.first; + const Variable* var = p.second; + if (tok->varId() == varid) + return true; + if (isAliasOf(var, tok, varid, value)) + return true; + } } return false; } @@ -2529,9 +2564,18 @@ struct VariableForwardAnalyzer : SingleValueFlowForwardAnalyzer { : SingleValueFlowForwardAnalyzer(), var(nullptr) {} - VariableForwardAnalyzer(const Variable* v, const ValueFlow::Value& val, const TokenList* t) + VariableForwardAnalyzer(const Variable* v, const ValueFlow::Value& val, std::vector paliases, const TokenList* t) : SingleValueFlowForwardAnalyzer(val, t), var(v) { varids[var->declarationId()] = var; + for(const Variable* av:paliases) { + if (!av) + continue; + aliases[av->declarationId()] = av; + } + } + + virtual const ValueType* getValueType(const Token*) const OVERRIDE { + return var->valueType(); } virtual bool match(const Token* tok) const OVERRIDE { @@ -2545,6 +2589,20 @@ struct VariableForwardAnalyzer : SingleValueFlowForwardAnalyzer { } }; +static void valueFlowForwardVariable(Token* const startToken, + const Token* const endToken, + const Variable* const var, + std::list values, + std::vector aliases, + TokenList* const tokenlist, + const Settings* const settings) +{ + for (ValueFlow::Value& v : values) { + VariableForwardAnalyzer a(var, v, aliases, tokenlist); + valueFlowGenericForward(startToken, endToken, a, settings); + } +} + static bool valueFlowForwardVariable(Token* const startToken, const Token* const endToken, const Variable* const var, @@ -2556,10 +2614,25 @@ static bool valueFlowForwardVariable(Token* const startToken, ErrorLogger* const, const Settings* const settings) { - for (ValueFlow::Value& v : values) { - VariableForwardAnalyzer a(var, v, tokenlist); - valueFlowGenericForward(startToken, endToken, a, settings); + std::vector aliases; + for (const ValueFlow::Value& v : values) { + if (!v.tokvalue) + continue; + const Token* lifeTok = nullptr; + for(const ValueFlow::Value& lv:v.tokvalue->values()) { + if (!lv.isLocalLifetimeValue()) + continue; + if (lifeTok) { + lifeTok = nullptr; + break; + } + lifeTok = lv.tokvalue; + } + if (lifeTok && lifeTok->variable()) { + aliases.push_back(lifeTok->variable()); + } } + valueFlowForwardVariable(startToken, endToken, var, std::move(values), aliases, tokenlist, settings); return true; } @@ -2578,6 +2651,10 @@ struct ExpressionForwardAnalyzer : SingleValueFlowForwardAnalyzer { setupExprVarIds(); } + virtual const ValueType* getValueType(const Token*) const OVERRIDE { + return expr->valueType(); + } + static bool nonLocal(const Variable* var, bool deref) { return !var || (!var->isLocal() && !var->isArgument()) || (deref && var->isArgument() && var->isPointer()) || var->isStatic() || var->isReference() || var->isExtern(); } diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index c27b785cc..952b75560 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -98,6 +98,7 @@ private: TEST_CASE(nullpointer55); // #8144 TEST_CASE(nullpointer56); // #9701 TEST_CASE(nullpointer57); // #9751 + TEST_CASE(nullpointer58); // #9807 TEST_CASE(nullpointer_addressOf); // address of TEST_CASE(nullpointerSwitch); // #2626 TEST_CASE(nullpointer_cast); // #4692 @@ -1840,6 +1841,16 @@ private: ASSERT_EQUALS("", errout.str()); } + void nullpointer58() { + check("struct myStruct { char entry[0]; };\n" + "void f() {\n" + " struct myStruct* sPtr = NULL;\n" + " int sz = (!*(&sPtr) || ((*(&sPtr))->entry[0] > 15)) ?\n" + " sizeof((*(&sPtr))->entry[0]) : 123456789;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void nullpointer_addressOf() { // address of check("void f() {\n" " struct X *x = 0;\n"