From 5f3333d86a86346a3e9d8dc01d0ac81f6ad77feb Mon Sep 17 00:00:00 2001 From: Paul Date: Fri, 17 Sep 2021 22:47:23 -0500 Subject: [PATCH 1/2] Fix 10331: wrong conditional value after assignment+return --- lib/forwardanalyzer.cpp | 26 +++++++++++++++++++++++--- lib/valueflow.cpp | 10 ++++++++++ test/testnullpointer.cpp | 21 +++++++++++++++++++++ test/testvalueflow.cpp | 15 ++++++++++++++- 4 files changed, 68 insertions(+), 4 deletions(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 529964df9c6..4af6bc51f8f 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -12,6 +12,14 @@ #include #include +struct OnExit { + std::function f; + + ~OnExit() { + f(); + } +}; + struct ForwardTraversal { enum class Progress { Continue, Break, Skip }; enum class Terminate { None, Bail, Escape, Modified, Inconclusive, Conditional }; @@ -25,6 +33,7 @@ struct ForwardTraversal { bool analyzeTerminate; Analyzer::Terminate terminate = Analyzer::Terminate::None; bool forked = false; + std::vector loopEnds = {}; Progress Break(Analyzer::Terminate t = Analyzer::Terminate::None) { if ((!analyzeOnly || analyzeTerminate) && t != Analyzer::Terminate::None) @@ -85,8 +94,15 @@ struct ForwardTraversal { template )> Progress traverseTok(T* tok, std::function f, bool traverseUnknown, T** out = nullptr) { - if (Token::Match(tok, "asm|goto|continue|setjmp|longjmp")) - return Break(); + 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); @@ -361,6 +377,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) @@ -632,7 +652,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); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 0127875b416..14949503f16 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -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]; @@ -5328,6 +5329,8 @@ 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 (")) { @@ -5338,6 +5341,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.. diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index b4586485c03..7cde18eab92 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -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 @@ -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" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index f8d417c9449..6f73e823210 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -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 @@ -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() { From 103c2bca19f684c083051f0e690fadcdf53cf00f Mon Sep 17 00:00:00 2001 From: Paul Date: Fri, 17 Sep 2021 22:49:10 -0500 Subject: [PATCH 2/2] Format --- lib/forwardanalyzer.cpp | 7 +++---- lib/valueflow.cpp | 3 ++- test/testvalueflow.cpp | 16 ++++++++-------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 4af6bc51f8f..750bb165bf5 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -102,8 +102,7 @@ struct ForwardTraversal { // 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)) { + } 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); @@ -379,8 +378,8 @@ struct ForwardTraversal { Progress updateInnerLoop(Token* endBlock, Token* stepTok, Token* condTok) { loopEnds.push_back(endBlock); OnExit oe{[&] { - loopEnds.pop_back(); - }}; + loopEnds.pop_back(); + }}; if (endBlock && updateScope(endBlock) == Progress::Break) return Break(); if (stepTok && updateRecursive(stepTok) == Progress::Break) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 14949503f16..0d7d37c71fc 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5329,7 +5329,8 @@ 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) + if (r.terminate != Analyzer::Terminate::None && r.terminate != Analyzer::Terminate::Escape && + r.terminate != Analyzer::Terminate::Modified) bailBlock = i; changeKnownToPossible(values); } diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 6f73e823210..61c9c71c863 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -2955,14 +2955,14 @@ class TestValueFlow : public TestFixture { 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"; + " 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)); }