Fix 10033: false negative: danglingTemporaryLifetime with usage of reference from nested object not detected (#3542)

This commit is contained in:
Paul Fultz II 2021-11-01 13:23:15 -05:00 committed by GitHub
parent 7d7584b456
commit d3f0aa5b34
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 58 additions and 23 deletions

View File

@ -491,8 +491,11 @@ const Token* getParentMember(const Token * tok)
const Token * parent = tok->astParent(); const Token * parent = tok->astParent();
if (!Token::simpleMatch(parent, ".")) if (!Token::simpleMatch(parent, "."))
return tok; return tok;
if (tok == parent->astOperand2()) if (astIsRHS(tok)) {
if (Token::simpleMatch(parent->astOperand1(), "."))
return parent->astOperand1()->astOperand2();
return parent->astOperand1(); return parent->astOperand1();
}
const Token * gparent = parent->astParent(); const Token * gparent = parent->astParent();
if (!Token::simpleMatch(gparent, ".") || gparent->astOperand2() != parent) if (!Token::simpleMatch(gparent, ".") || gparent->astOperand2() != parent)
return tok; return tok;
@ -2534,6 +2537,8 @@ static void getLHSVariablesRecursive(std::vector<const Variable*>& vars, const T
} else if (Token::simpleMatch(tok, ".")) { } else if (Token::simpleMatch(tok, ".")) {
getLHSVariablesRecursive(vars, tok->astOperand1()); getLHSVariablesRecursive(vars, tok->astOperand1());
getLHSVariablesRecursive(vars, tok->astOperand2()); getLHSVariablesRecursive(vars, tok->astOperand2());
} else if (Token::simpleMatch(tok, "::")) {
getLHSVariablesRecursive(vars, tok->astOperand2());
} else if (tok->variable()) { } else if (tok->variable()) {
vars.push_back(tok->variable()); vars.push_back(tok->variable());
} }

View File

@ -597,7 +597,7 @@ void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token
break; break;
} else if (!tokvalue->variable() && } else if (!tokvalue->variable() &&
isDeadTemporary(mTokenizer->isCPP(), tokvalue, tok, &mSettings->library)) { isDeadTemporary(mTokenizer->isCPP(), tokvalue, tok, &mSettings->library)) {
errorDanglingTemporaryLifetime(tok, &val); errorDanglingTemporaryLifetime(tok, &val, tokvalue);
break; break;
} }
} }
@ -679,13 +679,19 @@ void CheckAutoVariables::errorInvalidLifetime(const Token *tok, const ValueFlow:
reportError(errorPath, Severity::error, "invalidLifetime", msg + " that is out of scope.", CWE562, inconclusive ? Certainty::inconclusive : Certainty::normal); reportError(errorPath, Severity::error, "invalidLifetime", msg + " that is out of scope.", CWE562, inconclusive ? Certainty::inconclusive : Certainty::normal);
} }
void CheckAutoVariables::errorDanglingTemporaryLifetime(const Token* tok, const ValueFlow::Value* val) void CheckAutoVariables::errorDanglingTemporaryLifetime(const Token* tok, const ValueFlow::Value* val, const Token* tempTok)
{ {
const bool inconclusive = val ? val->isInconclusive() : false; const bool inconclusive = val ? val->isInconclusive() : false;
ErrorPath errorPath = val ? val->errorPath : ErrorPath(); ErrorPath errorPath = val ? val->errorPath : ErrorPath();
std::string msg = "Using " + lifetimeMessage(tok, val, errorPath); std::string msg = "Using " + lifetimeMessage(tok, val, errorPath);
errorPath.emplace_back(tempTok, "Temporary created here.");
errorPath.emplace_back(tok, ""); errorPath.emplace_back(tok, "");
reportError(errorPath, Severity::error, "danglingTemporaryLifetime", msg + " to temporary.", CWE562, inconclusive ? Certainty::inconclusive : Certainty::normal); reportError(errorPath,
Severity::error,
"danglingTemporaryLifetime",
msg + " that is a temporary.",
CWE562,
inconclusive ? Certainty::inconclusive : Certainty::normal);
} }
void CheckAutoVariables::errorDanglngLifetime(const Token *tok, const ValueFlow::Value *val) void CheckAutoVariables::errorDanglngLifetime(const Token *tok, const ValueFlow::Value *val)

View File

@ -79,7 +79,7 @@ private:
void errorReturnDanglingLifetime(const Token *tok, const ValueFlow::Value* val); void errorReturnDanglingLifetime(const Token *tok, const ValueFlow::Value* val);
void errorInvalidLifetime(const Token *tok, const ValueFlow::Value* val); void errorInvalidLifetime(const Token *tok, const ValueFlow::Value* val);
void errorDanglngLifetime(const Token *tok, const ValueFlow::Value *val); void errorDanglngLifetime(const Token *tok, const ValueFlow::Value *val);
void errorDanglingTemporaryLifetime(const Token* tok, const ValueFlow::Value* val); void errorDanglingTemporaryLifetime(const Token* tok, const ValueFlow::Value* val, const Token* tempTok);
void errorReturnReference(const Token* tok, ErrorPath errorPath, bool inconclusive); void errorReturnReference(const Token* tok, ErrorPath errorPath, bool inconclusive);
void errorDanglingReference(const Token *tok, const Variable *var, ErrorPath errorPath); void errorDanglingReference(const Token *tok, const Variable *var, ErrorPath errorPath);
void errorDanglingTempReference(const Token* tok, ErrorPath errorPath, bool inconclusive); void errorDanglingTempReference(const Token* tok, ErrorPath errorPath, bool inconclusive);
@ -106,7 +106,7 @@ private:
c.errorReturnDanglingLifetime(nullptr, nullptr); c.errorReturnDanglingLifetime(nullptr, nullptr);
c.errorInvalidLifetime(nullptr, nullptr); c.errorInvalidLifetime(nullptr, nullptr);
c.errorDanglngLifetime(nullptr, nullptr); c.errorDanglngLifetime(nullptr, nullptr);
c.errorDanglingTemporaryLifetime(nullptr, nullptr); c.errorDanglingTemporaryLifetime(nullptr, nullptr, nullptr);
} }
static std::string myName() { static std::string myName() {

View File

@ -2341,8 +2341,12 @@ struct ValueFlowAnalyzer : Analyzer {
// Follow references // Follow references
std::vector<ReferenceToken> refs = followAllReferences(tok); std::vector<ReferenceToken> refs = followAllReferences(tok);
const bool inconclusiveRefs = refs.size() != 1; const bool inconclusiveRefs = refs.size() != 1;
if (std::none_of(refs.begin(), refs.end(), [&](const ReferenceToken& ref) {
return tok == ref.token;
}))
refs.push_back(ReferenceToken{tok, {}});
for (const ReferenceToken& ref:refs) { for (const ReferenceToken& ref:refs) {
Action a = analyzeToken(ref.token, tok, d, inconclusiveRefs); Action a = analyzeToken(ref.token, tok, d, inconclusiveRefs && ref.token != tok);
if (internalMatch(ref.token)) if (internalMatch(ref.token))
a |= Action::Internal; a |= Action::Internal;
if (a != Action::None) if (a != Action::None)
@ -2944,29 +2948,34 @@ std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, Error
const Token *tokvalue = val ? val->tokvalue : nullptr; const Token *tokvalue = val ? val->tokvalue : nullptr;
const Variable *tokvar = tokvalue ? tokvalue->variable() : nullptr; const Variable *tokvar = tokvalue ? tokvalue->variable() : nullptr;
const Token *vartok = tokvar ? tokvar->nameToken() : nullptr; const Token *vartok = tokvar ? tokvar->nameToken() : nullptr;
const bool classVar = tokvar ? (!tokvar->isLocal() && !tokvar->isArgument() && !tokvar->isGlobal()) : false;
std::string type = lifetimeType(tok, val); std::string type = lifetimeType(tok, val);
std::string msg = type; std::string msg = type;
if (vartok) { if (vartok) {
errorPath.emplace_back(vartok, "Variable created here."); if (!classVar)
errorPath.emplace_back(vartok, "Variable created here.");
const Variable * var = vartok->variable(); const Variable * var = vartok->variable();
std::string submessage;
if (var) { if (var) {
switch (val->lifetimeKind) { switch (val->lifetimeKind) {
case ValueFlow::Value::LifetimeKind::SubObject: case ValueFlow::Value::LifetimeKind::SubObject:
case ValueFlow::Value::LifetimeKind::Object: case ValueFlow::Value::LifetimeKind::Object:
case ValueFlow::Value::LifetimeKind::Address: case ValueFlow::Value::LifetimeKind::Address:
if (type == "pointer") if (type == "pointer")
msg += " to local variable"; submessage = " to local variable";
else else
msg += " that points to local variable"; submessage = " that points to local variable";
break; break;
case ValueFlow::Value::LifetimeKind::Lambda: case ValueFlow::Value::LifetimeKind::Lambda:
msg += " that captures local variable"; submessage = " that captures local variable";
break; break;
case ValueFlow::Value::LifetimeKind::Iterator: case ValueFlow::Value::LifetimeKind::Iterator:
msg += " to local container"; submessage = " to local container";
break; break;
} }
msg += " '" + var->name() + "'"; if (classVar)
submessage.replace(submessage.find("local"), 5, "member");
msg += submessage + " '" + var->name() + "'";
} }
} }
return msg; return msg;

View File

@ -1767,8 +1767,10 @@ private:
" int& x = h();\n" " int& x = h();\n"
" g(&x);\n" " g(&x);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5] -> [test.cpp:5]: (error) Using pointer to temporary.\n" ASSERT_EQUALS(
"[test.cpp:4] -> [test.cpp:5]: (error) Using reference to dangling temporary.\n", errout.str()); "[test.cpp:4] -> [test.cpp:5] -> [test.cpp:4] -> [test.cpp:5]: (error) Using pointer that is a temporary.\n"
"[test.cpp:4] -> [test.cpp:5]: (error) Using reference to dangling temporary.\n",
errout.str());
check("void g(int*);\n" check("void g(int*);\n"
"int h();\n" "int h();\n"
@ -3120,7 +3122,7 @@ private:
" i += *x;\n" " i += *x;\n"
"}"); "}");
ASSERT_EQUALS( ASSERT_EQUALS(
"[test.cpp:1] -> [test.cpp:2] -> [test.cpp:5] -> [test.cpp:5] -> [test.cpp:6]: (error) Using pointer to temporary.\n", "[test.cpp:1] -> [test.cpp:2] -> [test.cpp:5] -> [test.cpp:5] -> [test.cpp:5] -> [test.cpp:6]: (error) Using pointer that is a temporary.\n",
errout.str()); errout.str());
check("QString f() {\n" check("QString f() {\n"
@ -3129,18 +3131,16 @@ private:
" QString c = b;\n" " QString c = b;\n"
" return c;\n" " return c;\n"
"}"); "}");
ASSERT_EQUALS( ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3] -> [test.cpp:4]: (error) Using pointer that is a temporary.\n",
"[test.cpp:3] -> [test.cpp:4]: (error) Using pointer to temporary.\n", errout.str());
errout.str());
check("auto f(std::string s) {\n" check("auto f(std::string s) {\n"
" const char *x = s.substr(1,2).c_str();\n" " const char *x = s.substr(1,2).c_str();\n"
" auto i = s.substr(4,5).begin();\n" " auto i = s.substr(4,5).begin();\n"
" return *i;\n" " return *i;\n"
"}"); "}");
ASSERT_EQUALS( ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:3] -> [test.cpp:4]: (error) Using iterator that is a temporary.\n",
"[test.cpp:3] -> [test.cpp:4]: (error) Using iterator to temporary.\n", errout.str());
errout.str());
check("std::string f() {\n" check("std::string f() {\n"
" std::stringstream tmp;\n" " std::stringstream tmp;\n"
@ -3183,6 +3183,21 @@ private:
" a->fun();\n" " a->fun();\n"
"}\n"); "}\n");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
check("struct A {\n"
" std::map<int, int> m_;\n"
"};\n"
"struct B {\n"
" A a_;\n"
"};\n"
"B func();\n"
"void f() {\n"
" const std::map<int, int>::iterator& m = func().a_.m_.begin();\n"
" (void)m->first;\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:9] -> [test.cpp:9] -> [test.cpp:10]: (error) Using object that points to member variable 'm_' that is a temporary.\n",
errout.str());
} }
void invalidLifetime() { void invalidLifetime() {

View File

@ -4833,7 +4833,7 @@ private:
"}\n", "}\n",
true); true);
ASSERT_EQUALS( ASSERT_EQUALS(
"[test.cpp:7] -> [test.cpp:8] -> [test.cpp:12] -> [test.cpp:2] -> [test.cpp:9]: (error) Using iterator to local container 'v' that may be invalid.\n", "[test.cpp:7] -> [test.cpp:8] -> [test.cpp:12] -> [test.cpp:9]: (error) Using iterator to member container 'v' that may be invalid.\n",
errout.str()); errout.str());
check("void foo(std::vector<int>& v) {\n" check("void foo(std::vector<int>& v) {\n"