Skip to content

Commit cc0a1aa

Browse files
pfultz2pfultz2
committed
Fix the false negative
1 parent db2b2f2 commit cc0a1aa

5 files changed

Lines changed: 129 additions & 13 deletions

File tree

‎lib/vf_analyzers.cpp‎

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,26 @@ static bool isDereferenceOp(const Token* tok)
5959
return tok->str() == "." && tok->originalName() == "->";
6060
}
6161

62+
// Every leaf is a constant or has an int value or constraint in the memory, so an undecided
63+
// condition can go either way.
64+
static bool stateCoversCondition(const Token* tok, const ProgramMemory& pm, int depth = 20)
65+
{
66+
if (!tok || depth < 0)
67+
return false;
68+
if (tok->hasKnownIntValue())
69+
return true;
70+
if (tok->varId() > 0 || Token::simpleMatch(tok, ".")) {
71+
const ProgramMemory::Values* values = pm.getValues(tok->exprId());
72+
return values && !values->empty() && std::all_of(values->cbegin(), values->cend(), [](const ValueFlow::Value& v) {
73+
return v.isIntValue();
74+
});
75+
}
76+
if (!tok->astOperand2())
77+
return Token::Match(tok, "!|-|~") && stateCoversCondition(tok->astOperand1(), pm, depth - 1);
78+
return tok->isConstOp() && stateCoversCondition(tok->astOperand1(), pm, depth - 1) &&
79+
stateCoversCondition(tok->astOperand2(), pm, depth - 1);
80+
}
81+
6282
struct ValueFlowAnalyzer : Analyzer {
6383
RefThunk<const Settings> settings;
6484
ProgramMemoryState pms;
@@ -383,6 +403,15 @@ struct ValueFlowAnalyzer : Analyzer {
383403
assert(false && "Internal update unimplemented.");
384404
}
385405

406+
// A conditional value is split only on a condition the state covers; an unknown one may be
407+
// correlated with the assumption.
408+
bool canSplitConditional(const Token* condTok) const
409+
{
410+
if (!condTok)
411+
return false;
412+
return stateCoversCondition(condTok, pms.get(condTok, nullptr, getProgramState()));
413+
}
414+
386415
private:
387416
// Returns Action::Match if its an exact match, return Action::Read if it partially matches the lifetime
388417
Action analyzeLifetime(const Token* tok) const
@@ -757,14 +786,13 @@ struct ValueFlowAnalyzer : Analyzer {
757786
pms.addState(anchor, getProgramState());
758787
pms.assume(tok, state, flags & Assume::ContainerEmpty, origin);
759788

760-
// The false path (the true path uses scopeEnd above): record the assumed state where control
761-
// continues - the end of the else block, or the closing brace when there is no else - so it
762-
// reaches the enclosing scope.
789+
// The false path: record the state where control continues, at the end of the else block or
790+
// past the closing brace (which still counts as inside the block).
763791
if (isCondBlock && !(flags & Assume::Pending) && !state) {
764792
if (Token::simpleMatch(endBlock, "} else {"))
765793
pms.addState(endBlock->linkAt(2)->previous(), getProgramState());
766794
else
767-
pms.addState(endBlock, getProgramState());
795+
pms.addState(endBlock->next(), getProgramState());
768796
}
769797

770798
if (!(flags & Assume::Quiet)) {
@@ -1051,7 +1079,7 @@ struct MultiValueFlowAnalyzer : ValueFlowAnalyzer {
10511079

10521080
bool stopOnCondition(const Token* condTok) const override {
10531081
if (isConditional())
1054-
return true;
1082+
return !canSplitConditional(condTok);
10551083
if (!condTok->hasKnownIntValue() && values.count(condTok->varId()) == 0) {
10561084
const auto& values_ = condTok->values();
10571085
return std::any_of(values_.cbegin(), values_.cend(), [](const ValueFlow::Value& v) {
@@ -1083,9 +1111,9 @@ struct MultiValueFlowAnalyzer : ValueFlowAnalyzer {
10831111
};
10841112
if (std::all_of(values.cbegin(), values.cend(), std::bind(pred, std::bind(SelectMapValues{}, std::placeholders::_1))))
10851113
return true;
1086-
if (isConditional())
1087-
return false;
10881114
const Token* condTok = getCondTokFromEnd(endBlock);
1115+
if (isConditional())
1116+
return canSplitConditional(condTok);
10891117
std::set<nonneg int> varids;
10901118
std::transform(getVars().cbegin(), getVars().cend(), std::inserter(varids, varids.begin()), SelectMapKeys{});
10911119
return bifurcate(condTok, varids, getSettings());
@@ -1232,9 +1260,9 @@ struct SingleValueFlowAnalyzer : ValueFlowAnalyzer {
12321260
if (value.isLifetimeValue())
12331261
return false;
12341262
// 'conditional' flag (uninit, or lowered after a modifying branch): may depend on a
1235-
// condition that doesn't mention the variable -> stop
1263+
// condition that doesn't mention the variable -> stop unless the state covers this one
12361264
if (value.conditional && !value.isKnown())
1237-
return true;
1265+
return !canSplitConditional(condTok);
12381266
if (value.isNonValue())
12391267
return false;
12401268
if (value.isSymbolicValue())
@@ -1272,9 +1300,9 @@ struct SingleValueFlowAnalyzer : ValueFlowAnalyzer {
12721300
return true;
12731301
if (value.isLifetimeValue())
12741302
return true;
1275-
if (isConditional())
1276-
return false;
12771303
const Token* condTok = getCondTokFromEnd(endBlock);
1304+
if (isConditional())
1305+
return canSplitConditional(condTok);
12781306
std::set<nonneg int> varids2;
12791307
std::transform(getVars().cbegin(), getVars().cend(), std::inserter(varids2, varids2.begin()), SelectMapKeys{});
12801308
return bifurcate(condTok, varids2, getSettings());

‎test/testnullpointer.cpp‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ class TestNullPointer : public TestFixture {
152152
TEST_CASE(nullpointer112); // unknown values in program memory
153153
TEST_CASE(nullpointer113); // a variable is clamped before the check
154154
TEST_CASE(nullpointer114); // the else block changes the value
155+
TEST_CASE(nullpointer115); // a range condition overlapping the assumed one
155156
TEST_CASE(nullpointer_addressOf); // address of
156157
TEST_CASE(nullpointerSwitch); // #2626
157158
TEST_CASE(nullpointer_cast); // #4692
@@ -3362,6 +3363,16 @@ class TestNullPointer : public TestFixture {
33623363
ASSERT_EQUALS("[test.cpp:4:13]: (warning) Possible null pointer dereference: p [nullPointer]\n", errout_str());
33633364
}
33643365

3366+
void nullpointer115() { // a range condition overlapping the assumed one
3367+
check("int f(int n, int *q) {\n"
3368+
" int *p = 0;\n"
3369+
" if (n > 5) p = q;\n"
3370+
" if (n > 3) return *p;\n"
3371+
" return 0;\n"
3372+
"}\n");
3373+
ASSERT_EQUALS("[test.cpp:4:24]: (warning) Possible null pointer dereference: p [nullPointer]\n", errout_str());
3374+
}
3375+
33653376
void nullpointer_addressOf() { // address of
33663377
check("void f() {\n"
33673378
" struct X *x = 0;\n"

‎test/testother.cpp‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -926,6 +926,15 @@ class TestOther : public TestFixture {
926926
" return 10 / x;\n"
927927
"}\n");
928928
ASSERT_EQUALS("[test.cpp:4:15]: (warning) Division by zero. [zerodivcond]\n", errout_str());
929+
930+
// a range condition overlapping the assumed one
931+
check("int f(int n) {\n"
932+
" int x = 0;\n"
933+
" if (n > 5) { x = 1; }\n"
934+
" if (n > 3) return 10 / x;\n"
935+
" return 0;\n"
936+
"}\n");
937+
ASSERT_EQUALS("[test.cpp:4:26]: (warning) Division by zero. [zerodivcond]\n", errout_str());
929938
}
930939

931940
void zeroDivErrorPath() {

‎test/testuninitvar.cpp‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4367,7 +4367,21 @@ class TestUninitVar : public TestFixture {
43674367
" if (x == 1)\n"
43684368
" v = value;\n"
43694369
"}\n");
4370-
TODO_ASSERT_EQUALS("[test.cpp:6:9]: (error) Uninitialized variable: value [legacyUninitvar]\n", "", errout_str());
4370+
ASSERT_EQUALS("[test.cpp:3:9] -> [test.cpp:5:9] -> [test.cpp:6:9]: (warning) Uninitialized variable: value [uninitvar]\n", errout_str());
4371+
4372+
// false path: 'flag' is false past the block
4373+
valueFlowUninit("void getX(int *p);\n"
4374+
"bool g();\n"
4375+
"void f() {\n"
4376+
" int x;\n"
4377+
" bool flag, ok;\n"
4378+
" ok = g();\n"
4379+
" flag = ok;\n"
4380+
" if (ok == true) { getX(&x); }\n"
4381+
" if (flag == true) { int a = x; }\n"
4382+
" if (flag == false) { int b = x; }\n"
4383+
"}\n");
4384+
ASSERT_EQUALS("[test.cpp:8:12] -> [test.cpp:10:14] -> [test.cpp:10:34]: (warning) Uninitialized variable: x [uninitvar]\n", errout_str());
43714385

43724386
valueFlowUninit("void f(int x) {\n"
43734387
" int value;\n"
@@ -6134,7 +6148,7 @@ class TestUninitVar : public TestFixture {
61346148
" if (b == 'x') {}\n"
61356149
" if (a) {}\n"
61366150
"}\n");
6137-
TODO_ASSERT_EQUALS("[test.cpp:8]: (error) Uninitialized variable: a\n", "", errout_str());
6151+
ASSERT_EQUALS("[test.cpp:3:11] -> [test.cpp:5:11] -> [test.cpp:7:11] -> [test.cpp:8:9]: (warning) Uninitialized variable: a [uninitvar]\n", errout_str());
61386152

61396153
valueFlowUninit("void h() {\n"
61406154
" int i;\n"

‎test/testvalueflow.cpp‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4587,6 +4587,60 @@ class TestValueFlow : public TestFixture {
45874587
"}\n";
45884588
ASSERT_EQUALS(true, testValueOfXImpossible(code, 4U, 0));
45894589

4590+
// 'n <= 5' leaves 'n > 3' undecided: the conditional value is split
4591+
code = "void f(int n) {\n"
4592+
" int x = 0;\n"
4593+
" if (n > 5) { x = 1; }\n"
4594+
" if (n > 3) { int a = x; }\n"
4595+
"}\n";
4596+
ASSERT_EQUALS(true, testValueOfX(code, 4U, 0));
4597+
ASSERT_EQUALS(true, testValueOfX(code, 4U, 1));
4598+
4599+
code = "void f(int n) {\n"
4600+
" int x = 0;\n"
4601+
" if (n > 5) { x = 1; }\n"
4602+
" if (n == 2) { int a = x; }\n"
4603+
"}\n";
4604+
ASSERT_EQUALS(true, testValueOfX(code, 4U, 0));
4605+
4606+
code = "void f(int n) {\n"
4607+
" int x = 0;\n"
4608+
" if (n > 5) { x = 1; }\n"
4609+
" if (!(n < 3)) { int a = x; }\n"
4610+
"}\n";
4611+
ASSERT_EQUALS(true, testValueOfX(code, 4U, 0));
4612+
4613+
code = "void f(int n) {\n"
4614+
" int x = 0;\n"
4615+
" if (n > 5) { x = 1; }\n"
4616+
" if (n - 3 > 0) { int a = x; }\n"
4617+
"}\n";
4618+
ASSERT_EQUALS(true, testValueOfX(code, 4U, 0));
4619+
4620+
// decided false: the block is not reached
4621+
code = "void f(int n) {\n"
4622+
" int x = 0;\n"
4623+
" if (n > 5) { x = 1; }\n"
4624+
" if (n == 7) { int a = x; }\n"
4625+
"}\n";
4626+
ASSERT_EQUALS(false, testValueOfX(code, 4U, 0));
4627+
4628+
// not covered by the state: no split
4629+
code = "void f(int n, int c) {\n"
4630+
" int x = 0;\n"
4631+
" if (n > 5) { x = 1; }\n"
4632+
" if (c) { int a = x; }\n"
4633+
"}\n";
4634+
ASSERT_EQUALS(false, testValueOfX(code, 4U, 0));
4635+
4636+
code = "int g(int);\n"
4637+
"void f(int n) {\n"
4638+
" int x = 0;\n"
4639+
" if (n > 5) { x = 1; }\n"
4640+
" if (g(n)) { int a = x; }\n"
4641+
"}\n";
4642+
ASSERT_EQUALS(false, testValueOfX(code, 5U, 0));
4643+
45904644
// no fork: the main path still knows the condition is false
45914645
/*const*/ Settings s(settings);
45924646
s.vfOptions.maxForwardConditionForkDepth = 0;

0 commit comments

Comments
 (0)