Skip to content

Fix 10331: wrong conditional value after assignment+return #3461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danmar merged 2 commits into from
Sep 21, 2021
Merged

Fix 10331: wrong conditional value after assignment+return #3461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f3333d
Fix 10331: wrong conditional value after assignment+return
pfultz2 Sep 18, 2021
103c2bc
Format
pfultz2 Sep 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions lib/forwardanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@
#include <tuple>
#include <utility>

struct OnExit {
std::function<void()> f;

~OnExit() {
f();
}
};

struct ForwardTraversal {
enum class Progress { Continue, Break, Skip };
enum class Terminate { None, Bail, Escape, Modified, Inconclusive, Conditional };
Expand All @@ -25,6 +33,7 @@ struct ForwardTraversal {
bool analyzeTerminate;
Analyzer::Terminate terminate = Analyzer::Terminate::None;
bool forked = false;
std::vector<Token*> loopEnds = {};

Progress Break(Analyzer::Terminate t = Analyzer::Terminate::None) {
if ((!analyzeOnly || analyzeTerminate) && t != Analyzer::Terminate::None)
Expand Down Expand Up @@ -85,9 +94,15 @@ struct ForwardTraversal {

template<class T, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*> )>
Progress traverseTok(T* tok, std::function<Progress(T*)> f, bool traverseUnknown, T** out = nullptr) {
if (Token::Match(tok, "asm|goto|continue|setjmp|longjmp"))
return Break();
else if (Token::Match(tok, "return|throw") || isEscapeFunction(tok, &settings->library)) {
if (Token::Match(tok, "asm|goto|setjmp|longjmp"))
return Break(Analyzer::Terminate::Bail);
else if (Token::simpleMatch(tok, "continue")) {
if (loopEnds.empty())
return Break(Analyzer::Terminate::Escape);
// If we are in a loop then jump to the end
if (out)
*out = loopEnds.back();
} else if (Token::Match(tok, "return|throw") || isEscapeFunction(tok, &settings->library)) {
traverseRecursive(tok->astOperand1(), f, traverseUnknown);
traverseRecursive(tok->astOperand2(), f, traverseUnknown);
return Break(Analyzer::Terminate::Escape);
Expand Down Expand Up @@ -361,6 +376,10 @@ struct ForwardTraversal {
}

Progress updateInnerLoop(Token* endBlock, Token* stepTok, Token* condTok) {
loopEnds.push_back(endBlock);
OnExit oe{[&] {
loopEnds.pop_back();
}};
if (endBlock && updateScope(endBlock) == Progress::Break)
return Break();
if (stepTok && updateRecursive(stepTok) == Progress::Break)
Expand Down Expand Up @@ -632,7 +651,7 @@ struct ForwardTraversal {
}
actions |= (thenBranch.action | elseBranch.action);
if (bail)
return Break();
return Break(Analyzer::Terminate::Bail);
if (thenBranch.isDead() && elseBranch.isDead()) {
if (thenBranch.isModified() && elseBranch.isModified())
return Break(Analyzer::Terminate::Modified);
Expand Down
11 changes: 11 additions & 0 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5315,6 +5315,7 @@ struct ConditionHandler {
startTokens[1] = top->link()->linkAt(1)->tokAt(2);

int changeBlock = -1;
int bailBlock = -1;

for (int i = 0; i < 2; i++) {
const Token* const startToken = startTokens[i];
Expand All @@ -5328,6 +5329,9 @@ struct ConditionHandler {
deadBranch[i] = r.terminate == Analyzer::Terminate::Escape;
if (r.action.isModified() && !deadBranch[i])
changeBlock = i;
if (r.terminate != Analyzer::Terminate::None && r.terminate != Analyzer::Terminate::Escape &&
r.terminate != Analyzer::Terminate::Modified)
bailBlock = i;
changeKnownToPossible(values);
}
if (changeBlock >= 0 && !Token::simpleMatch(top->previous(), "while (")) {
Expand All @@ -5338,6 +5342,13 @@ struct ConditionHandler {
"valueFlowAfterCondition: " + cond.vartok->expressionString() +
" is changed in conditional block");
return;
} else if (bailBlock >= 0) {
if (settings->debugwarnings)
bailout(tokenlist,
errorLogger,
startTokens[bailBlock]->link(),
"valueFlowAfterCondition: bailing in conditional block");
return;
}

// After conditional code..
Expand Down
21 changes: 21 additions & 0 deletions test/testnullpointer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ class TestNullPointer : public TestFixture {
TEST_CASE(nullpointer79); // #10400
TEST_CASE(nullpointer80); // #10410
TEST_CASE(nullpointer81); // #8724
TEST_CASE(nullpointer82); // #10331
TEST_CASE(nullpointer_addressOf); // address of
TEST_CASE(nullpointerSwitch); // #2626
TEST_CASE(nullpointer_cast); // #4692
Expand Down Expand Up @@ -2474,6 +2475,26 @@ class TestNullPointer : public TestFixture {
ASSERT_EQUALS("", errout.str());
}

void nullpointer82() // #10331
{
check("bool g();\n"
"int* h();\n"
"void f(int* ptr) {\n"
" if (!ptr) {\n"
" if (g())\n"
" goto done;\n"
" ptr = h();\n"
" if (!ptr)\n"
" return;\n"
" }\n"
" if (*ptr == 1)\n"
" return;\n"
"\n"
"done:\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}

void nullpointer_addressOf() { // address of
check("void f() {\n"
" struct X *x = 0;\n"
Expand Down
15 changes: 14 additions & 1 deletion test/testvalueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,7 +1703,8 @@ class TestValueFlow : public TestFixture {
" if (x==123){}\n"
"}");
ASSERT_EQUALS_WITHOUT_LINENUMBERS(
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n",
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n"
"[test.cpp:2]: (debug) valueflow.cpp::(valueFlow) bailout: valueFlowAfterCondition: bailing in conditional block\n",
errout.str());

// #5721 - FP
Expand Down Expand Up @@ -2952,6 +2953,18 @@ class TestValueFlow : public TestFixture {
" return 0;\n"
"}\n";
ASSERT_EQUALS(true, testValueOfXKnown(code, 5U, 1));

code = "int f(int x) {\n"
" if (x == 1) {\n"
" for(int i=0;i<1;i++) {\n"
" if (x == 1)\n"
" continue;\n"
" }\n"
" }\n"
" return x;\n"
"}\n";
ASSERT_EQUALS(true, testValueOfX(code, 8U, 1));
ASSERT_EQUALS(false, testValueOfXImpossible(code, 8U, 1));
}

void valueFlowAfterConditionExpr() {
Expand Down