diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 727c64b076d..6b05c2d0f42 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -407,6 +407,27 @@ namespace { return p; } + // Update the branch that the evaluated condition takes + Progress updateTakenBranch(Branch& branch, const Token* skippedBlock, const Token* condTok, int depth) + { + // The condition is only "known" because of an earlier assumption, so the + // skipped block could still modify the value -> lower to possible + if (!condTok->hasKnownIntValue() && skippedBlock && analyzeScope(skippedBlock).isModified() && + !analyzer->lowerToPossible()) + return Break(Analyzer::Terminate::Bail); + if (!branch.endBlock) + return Progress::Continue; + updateScopeState(branch.endBlock); + if (updateBranch(branch, depth - 1) == Progress::Break) + return Progress::Break; + // The branch was entered because of the tracked value; if it might not + // return (it ends in a call to an unknown, possibly noreturn function) + // then the value might not flow past the branch. + if (!condTok->hasKnownIntValue() && !branch.escape && branch.escapeUnknown && !analyzer->lowerToInconclusive()) + return Break(Analyzer::Terminate::Bail); + return Progress::Continue; + } + bool reentersLoop(Token* endBlock, const Token* condTok, const Token* stepTok) const { if (!condTok) return true; @@ -558,16 +579,21 @@ namespace { return updateLoop(endToken, endBlock, condTok, initTok, stepTok, true); } - Progress updateScope(Token* endBlock, int depth = 20) + void updateScopeState(const Token* endBlock) { - if (!endBlock) - return Break(); assert(endBlock->link()); - Token* ctx = endBlock->link()->previous(); + const Token* ctx = endBlock->link()->previous(); if (Token::simpleMatch(ctx, ")")) ctx = ctx->link()->previous(); if (ctx) analyzer->updateState(ctx); + } + + Progress updateScope(Token* endBlock, int depth = 20) + { + if (!endBlock) + return Break(); + updateScopeState(endBlock); return updateRange(endBlock->link(), endBlock, depth); } @@ -738,19 +764,11 @@ namespace { const bool hasElse = Token::simpleMatch(endBlock, "} else {"); tok = hasElse ? endBlock->linkAt(2) : endBlock; if (thenBranch.check) { - // The condition is only "known" because of an earlier assumption, so the - // skipped else block could still modify the value -> lower to possible - if (!condTok->hasKnownIntValue() && hasElse && - analyzeScope(elseBranch.endBlock).isModified() && !analyzer->lowerToPossible()) - return Break(Analyzer::Terminate::Bail); - if (updateScope(thenBranch.endBlock, depth - 1) == Progress::Break) + if (updateTakenBranch(thenBranch, hasElse ? elseBranch.endBlock : nullptr, condTok, depth) == + Progress::Break) return Break(); } else if (elseBranch.check) { - // Likewise the skipped then block could still modify the value - if (!condTok->hasKnownIntValue() && analyzeScope(thenBranch.endBlock).isModified() && - !analyzer->lowerToPossible()) - return Break(Analyzer::Terminate::Bail); - if (elseBranch.endBlock && updateScope(elseBranch.endBlock, depth - 1) == Progress::Break) + if (updateTakenBranch(elseBranch, thenBranch.endBlock, condTok, depth) == Progress::Break) return Break(); } else { const bool conditional = stopOnCondition(condTok); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index ef81fc772ac..8c77b17cf94 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -4650,6 +4650,14 @@ struct ConditionHandler { }); } + static void lowerToInconclusive(std::list& values) + { + for (ValueFlow::Value& v : values) { + if (!v.isImpossible()) + v.setInconclusive(); + } + } + void afterCondition(TokenList& tokenlist, const SymbolDatabase& symboldatabase, ErrorLogger& errorLogger, @@ -4882,10 +4890,13 @@ struct ConditionHandler { else if (!dead_if) dead_if = isReturnScope(after, settings.library, &unknownFunction); + // If the taken branch might not return (it ends in a call to an unknown, + // possibly noreturn function) then its values might not flow past the + // conditional code -> lower them to inconclusive. if (!dead_if && unknownFunction) { if (settings.debugwarnings) bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope"); - return; + lowerToInconclusive(thenValues); } if (Token::simpleMatch(after, "} else {")) { @@ -4896,7 +4907,7 @@ struct ConditionHandler { if (!dead_else && unknownFunction) { if (settings.debugwarnings) bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope"); - return; + lowerToInconclusive(elseValues); } } @@ -4912,11 +4923,15 @@ struct ConditionHandler { std::copy_if(thenValues.cbegin(), thenValues.cend(), std::back_inserter(values), - std::mem_fn(&ValueFlow::Value::isPossible)); + [](const ValueFlow::Value& v) { + return v.isPossible() || v.isInconclusive(); + }); std::copy_if(elseValues.cbegin(), elseValues.cend(), std::back_inserter(values), - std::mem_fn(&ValueFlow::Value::isPossible)); + [](const ValueFlow::Value& v) { + return v.isPossible() || v.isInconclusive(); + }); } if (values.empty()) diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 2483a967947..02810906fb2 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -4468,6 +4468,59 @@ class TestNullPointer : public TestFixture { "[test.cpp:3:13]: (warning) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n" "[test.cpp:4:12]: (warning) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n", errout_str()); + + // the guard might call an unknown, possibly noreturn function -> no warning + check("void f() {\n" + " FILE* fid = fopen(\"x.txt\", \"w\");\n" + " if (fid == NULL)\n" + " g();\n" + " fclose(fid);\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // .. but an inconclusive warning is reported with --inconclusive + check("void f() {\n" + " FILE* fid = fopen(\"x.txt\", \"w\");\n" + " if (fid == NULL)\n" + " g();\n" + " fclose(fid);\n" + "}\n", + dinit(CheckOptions, $.inconclusive = true)); + ASSERT_EQUALS( + "[test.cpp:5:12]: (warning, inconclusive) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n", + errout_str()); + + check("int f(const int* p) {\n" + " if (p == nullptr)\n" + " g();\n" + " return *p;\n" + "}\n", + dinit(CheckOptions, $.inconclusive = true)); + ASSERT_EQUALS( + "[test.cpp:2:11] -> [test.cpp:4:13]: (warning, inconclusive) Either the condition 'p==nullptr' is redundant or there is possible null pointer dereference: p. [nullPointerRedundantCheck]\n", + errout_str()); + + check("void f() {\n" + " FILE* fid = fopen(\"x.txt\", \"w\");\n" + " if (fid != NULL)\n" + " ;\n" + " else\n" + " g();\n" + " fclose(fid);\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // guard function is known to return -> warning + check("void g() {}\n" + "void f() {\n" + " FILE* fid = fopen(\"x.txt\", \"w\");\n" + " if (fid == NULL)\n" + " g();\n" + " fclose(fid);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:6:12]: (warning) If resource allocation fails, then there is a possible null pointer dereference: fid [nullPointerOutOfResources]\n", + errout_str()); } void functioncalllibrary() { diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 888710ce57d..14d1f9e8fa0 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -3834,6 +3834,44 @@ class TestValueFlow : public TestFixture { " return x;\n" "}\n"; ASSERT_EQUALS(true, testValueOfX(code, 4U, 0)); + + // if the guarded block calls an unknown, possibly noreturn function + // then the condition value is lowered to inconclusive after the block + code = "int f(int x) {\n" + " if (x == 0)\n" + " g();\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXInconclusive(code, 4U, 0)); + + // .. also when the guard is in the else branch + code = "int f(int x) {\n" + " if (x != 0)\n" + " ;\n" + " else\n" + " g();\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXInconclusive(code, 6U, 0)); + + // a declared function is assumed to return + code = "void g();\n" + "int f(int x) {\n" + " if (x == 0)\n" + " g();\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfX(code, 5U, 0)); + ASSERT_EQUALS(false, testValueOfXInconclusive(code, 5U, 0)); + + // a noreturn function conclusively escapes + code = "int f(int x) {\n" + " if (x == 0)\n" + " abort();\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(false, testValueOfX(code, 4U, 0)); + ASSERT_EQUALS(true, testValueOfXImpossible(code, 4U, 0)); } void valueFlowAfterConditionTernary()