Skip to content

Commit bcd74c7

Browse files
pfultz2Your Name
andauthored
Partial fix for 9049: False negative: uninitialized variable with nested ifs (#8680)
This fixes the case for this: ```cpp unsigned int g(); void f(bool a) { unsigned int dimensions = 0; bool mightBeLarger; if (a) { dimensions = g(); if (dimensions >= 1) mightBeLarger = false; } else { mightBeLarger = false; } if (dimensions == 1) return; if (!mightBeLarger) {} } ``` Which doesnt use a compund condition `dimensions >= 1 && b)` and it also requires complete variables and functions. The compund condition could be handled in the future by forking within the condition, so `if(a && b) { ... }` can be treated as `if(a) { if(b) { ... }}`. Here is a summary of the changes: 1. Fork-based condition handling — lib/forwardanalyzer.cpp (the largest change) - When a condition can't be resolved, the traversal now forks: the then-branch is walked by a separate ForwardTraversal, in analyze-only mode when the value can't actually flow into it (opaque/correlated conditions like if (f(x))), so the branch's effect is tracked but nothing is reported there. - Branch breaks are deferred: if the else kills the value on the main path, the then-fork can still carry it forward. - Escapes the traversal didn't flag (e.g. unknown noreturn calls) are detected via isEscapeScope, and exit/abort are now recognized as escape functions (lib/astutils.cpp). 2. Program-state anchoring at block boundaries — lib/vf_analyzers.cpp + lib/analyzer.h - assume() now anchors the assumed state at the block's end (not the condition) when control is leaving an already-traversed branch. This keeps an assumption on a variable modified inside the block (e.g. a nested if narrowing a value computed there) from being discarded as "modified" once control leaves the block. - New Assume::Pending flag marks the pre-traversal assume (branch walked separately) so it doesn't record premature boundary state. - ProgramMemoryState::assume gained an optional origin parameter so the anchor point can be overridden. 3. vars-aware execute — lib/programmemory.cpp / .h - execute/conditionIsTrue/conditionIsFalse now take the tracked values (vars). A tracked value is the authoritative current value of its expression (returned and written back into the program memory), and any cached compound that depends on a tracked value is re-evaluated instead of served stale (getTrackedValue / dependsOnTrackedValue). - The per-assignment substitution was removed from fillProgramMemoryFromAssignments and now lives entirely in execute so it can be used for all executions. --------- Co-authored-by: Your Name <you@example.com>
1 parent e039c08 commit bcd74c7

15 files changed

Lines changed: 514 additions & 207 deletions

lib/analyzer.h

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,13 @@ struct Analyzer {
153153
struct Assume {
154154
enum Flags : std::uint8_t {
155155
None = 0,
156-
Quiet = (1 << 0),
157-
Absolute = (1 << 1),
158-
ContainerEmpty = (1 << 2),
156+
Quiet = (1u << 0),
157+
Absolute = (1u << 1),
158+
ContainerEmpty = (1u << 2),
159+
// The branch this condition guards is not traversed yet (a separate path walks it), so
160+
// the assume must not record the program state at the branch boundaries - they would be
161+
// premature. When unset, the branch has been traversed and control is leaving it.
162+
Pending = (1u << 3),
159163
};
160164
};
161165

lib/astutils.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2230,6 +2230,8 @@ bool isEscapeFunction(const Token* ftok, const Library& library)
22302230
{
22312231
if (!Token::Match(ftok, "%name% ("))
22322232
return false;
2233+
if (Token::Match(ftok, "exit|abort"))
2234+
return true;
22332235
const Function* function = ftok->function();
22342236
if (function) {
22352237
if (function->isEscapeFunction())

lib/forwardanalyzer.cpp

Lines changed: 121 additions & 104 deletions
Large diffs are not rendered by default.

lib/programmemory.cpp

Lines changed: 144 additions & 65 deletions
Large diffs are not rendered by default.

lib/programmemory.h

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include <map>
2929
#include <memory>
3030
#include <string>
31+
#include <tuple>
3132
#include <unordered_map>
3233
#include <utility>
3334
#include <vector>
@@ -161,20 +162,40 @@ struct CPPCHECKLIB ProgramMemory {
161162
};
162163

163164
struct ProgramMemoryState {
165+
struct ChangedKeyHash {
166+
std::size_t operator()(const std::tuple<const Token*, const Token*, const Token*>& t) const
167+
{
168+
const std::hash<const Token*> h;
169+
std::size_t seed = h(std::get<0>(t));
170+
seed ^= h(std::get<1>(t)) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
171+
seed ^= h(std::get<2>(t)) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
172+
return seed;
173+
}
174+
};
175+
using ChangedCache =
176+
std::unordered_map<std::tuple<const Token*, const Token*, const Token*>, const Token*, ChangedKeyHash>;
177+
// The token modifying expr between start and end, or nullptr.
178+
using FindChangedFn = std::function<const Token*(const Token* expr, const Token* start, const Token* end)>;
179+
164180
ProgramMemory state;
165181
std::map<nonneg int, const Token*> origins;
166182
const Settings& settings;
183+
// Memoized findExpressionChanged() pre-filter; structural, so never invalidated.
184+
std::shared_ptr<ChangedCache> changedCache;
167185

168186
explicit ProgramMemoryState(const Settings& s);
169187

170188
void replace(ProgramMemory pm, const Token* origin = nullptr);
171189

172190
void addState(const Token* tok, const ProgramMemory::Map& vars);
173191

174-
void assume(const Token* tok, bool b, bool isEmpty = false);
192+
void assume(const Token* tok, bool b, bool isEmpty = false, const Token* origin = nullptr);
175193

176194
void removeModifiedVars(const Token* tok);
177195

196+
// A findExpressionChanged() closure memoized in changedCache
197+
FindChangedFn getCachedFindExpressionChanged(bool skipDeadCode) const;
198+
178199
ProgramMemory get(const Token* tok, const Token* ctx, const ProgramMemory::Map& vars) const;
179200
};
180201

@@ -184,21 +205,30 @@ void execute(const Token* expr,
184205
ProgramMemory& programMemory,
185206
MathLib::bigint* result,
186207
bool* error,
187-
const Settings& settings);
208+
const Settings& settings,
209+
const ProgramMemory::Map& vars = {});
188210

189211
/**
190212
* Is condition always false when variable has given value?
191213
* \param condition top ast token in condition
192214
* \param pm program memory
215+
* \param vars optional tracked values that take precedence over the program memory
193216
*/
194-
bool conditionIsFalse(const Token* condition, ProgramMemory pm, const Settings& settings);
217+
bool conditionIsFalse(const Token* condition,
218+
ProgramMemory pm,
219+
const Settings& settings,
220+
const ProgramMemory::Map& vars = {});
195221

196222
/**
197223
* Is condition always true when variable has given value?
198224
* \param condition top ast token in condition
199225
* \param pm program memory
226+
* \param vars optional tracked values that take precedence over the program memory
200227
*/
201-
bool conditionIsTrue(const Token* condition, ProgramMemory pm, const Settings& settings);
228+
bool conditionIsTrue(const Token* condition,
229+
ProgramMemory pm,
230+
const Settings& settings,
231+
const ProgramMemory::Map& vars = {});
202232

203233
/**
204234
* Get program memory by looking backwards from given token.

lib/settings.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,7 @@ void Settings::setCheckLevel(CheckLevel level)
335335
vfOptions.maxIfCount = 100;
336336
vfOptions.doConditionExpressionAnalysis = false;
337337
vfOptions.maxForwardBranches = 4;
338+
vfOptions.maxForwardConditionForkDepth = 0;
338339
vfOptions.maxIterations = 1;
339340
}
340341
else if (level == CheckLevel::normal) {
@@ -344,6 +345,7 @@ void Settings::setCheckLevel(CheckLevel level)
344345
vfOptions.maxIfCount = 100;
345346
vfOptions.doConditionExpressionAnalysis = false;
346347
vfOptions.maxForwardBranches = 4;
348+
vfOptions.maxForwardConditionForkDepth = 1;
347349
}
348350
else if (level == CheckLevel::exhaustive) {
349351
// Checking can take a little while. ~ 10 times slower than normal analysis is OK.
@@ -352,6 +354,8 @@ void Settings::setCheckLevel(CheckLevel level)
352354
vfOptions.maxSubFunctionArgs = 256;
353355
vfOptions.doConditionExpressionAnalysis = true;
354356
vfOptions.maxForwardBranches = -1;
357+
vfOptions.maxForwardConditionForkDepth = 4;
358+
vfOptions.maxForwardConditionForks = 256;
355359
}
356360
}
357361

lib/settings.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -512,6 +512,15 @@ class CPPCHECKLIB WARN_UNUSED Settings {
512512
/** @brief Maximum performed forward branches */
513513
int maxForwardBranches = -1;
514514

515+
/** @brief Maximum depth of nested condition-fork continuations in the forward analyzer.
516+
Bounds the exponential fan-out of carrying condition state forward at exhaustive level (where
517+
maxForwardBranches is unlimited); past it the forward analysis continues on a single linear path
518+
without skipping any branch. 0 disables forking (linear); a negative value means unlimited. */
519+
int maxForwardConditionForkDepth = 4;
520+
521+
/** @brief Maximum total condition-fork continuations in one forward traversal. */
522+
int maxForwardConditionForks = 256;
523+
515524
/** @brief Maximum performed alignof recursion */
516525
int maxAlignOfRecursion = 100;
517526

lib/vf_analyzers.cpp

Lines changed: 39 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -678,16 +678,20 @@ struct ValueFlowAnalyzer : Analyzer {
678678
if (const ValueFlow::Value* v = tok->getKnownValue(ValueFlow::Value::ValueType::INT))
679679
return {v->intvalue};
680680
std::vector<MathLib::bigint> result;
681-
ProgramMemory pm = getProgramMemoryFunc();
681+
// Pass the tracked values so a cached program-memory value that depends on one (e.g. 'h(p)'
682+
// after 'p' was reassigned) is re-evaluated rather than served stale. The memory is built
683+
// from the same state, so compute it once and hand it to the builder.
684+
const ProgramState vars = getProgramState();
685+
ProgramMemory pm = getProgramMemoryFunc(vars);
682686
if (Token::Match(tok, "&&|%oror%")) {
683-
if (conditionIsTrue(tok, pm, getSettings()))
687+
if (conditionIsTrue(tok, pm, getSettings(), vars))
684688
result.push_back(1);
685-
if (conditionIsFalse(tok, std::move(pm), getSettings()))
689+
if (conditionIsFalse(tok, std::move(pm), getSettings(), vars))
686690
result.push_back(0);
687691
} else {
688692
MathLib::bigint out = 0;
689693
bool error = false;
690-
execute(tok, pm, &out, &error, getSettings());
694+
execute(tok, pm, &out, &error, getSettings(), vars);
691695
if (!error)
692696
result.push_back(out);
693697
}
@@ -696,16 +700,16 @@ struct ValueFlowAnalyzer : Analyzer {
696700

697701
std::vector<MathLib::bigint> evaluateInt(const Token* tok) const
698702
{
699-
return evaluateInt(tok, [&] {
700-
return ProgramMemory{getProgramState()};
703+
return evaluateInt(tok, [](const ProgramState& vars) {
704+
return ProgramMemory{vars};
701705
});
702706
}
703707

704708
std::vector<MathLib::bigint> evaluate(Evaluate e, const Token* tok, const Token* ctx = nullptr) const override
705709
{
706710
if (e == Evaluate::Integral) {
707-
return evaluateInt(tok, [&] {
708-
return pms.get(tok, ctx, getProgramState());
711+
return evaluateInt(tok, [&](const ProgramState& vars) {
712+
return pms.get(tok, ctx, vars);
709713
});
710714
}
711715
if (e == Evaluate::ContainerEmpty) {
@@ -723,30 +727,43 @@ struct ValueFlowAnalyzer : Analyzer {
723727
return {};
724728
}
725729

726-
void assume(const Token* tok, bool state, unsigned int flags) override {
727-
// Update program state
728-
pms.removeModifiedVars(tok);
729-
pms.addState(tok, getProgramState());
730-
pms.assume(tok, state, flags & Assume::ContainerEmpty);
731-
730+
void assume(const Token* tok, bool state, unsigned int flags) override
731+
{
732732
bool isCondBlock = false;
733733
const Token* parent = tok->astParent();
734734
if (parent) {
735735
isCondBlock = Token::Match(parent->previous(), "if|while (");
736736
}
737737

738+
const Token* endBlock = nullptr;
738739
if (isCondBlock) {
739740
const Token* startBlock = parent->link()->next();
740741
if (Token::simpleMatch(startBlock, ";") && Token::simpleMatch(parent->tokAt(-2), "} while ("))
741742
startBlock = parent->linkAt(-2);
742-
const Token* endBlock = startBlock->link();
743-
if (state) {
744-
pms.removeModifiedVars(endBlock);
745-
pms.addState(endBlock->previous(), getProgramState());
746-
} else {
747-
if (Token::simpleMatch(endBlock, "} else {"))
748-
pms.addState(endBlock->linkAt(2)->previous(), getProgramState());
749-
}
743+
endBlock = startBlock->link();
744+
}
745+
746+
// Without Pending the 'then' block has been traversed and control is leaving it, so anchor
747+
// the assumed state at the block end instead of the condition. That keeps assumptions on
748+
// variables modified inside the block (e.g. an 'if' narrowing a value computed there) from
749+
// being discarded as "modified" once control leaves the block.
750+
const bool scopeEnd = !(flags & Assume::Pending) && state && endBlock;
751+
const Token* anchor = scopeEnd ? endBlock : tok;
752+
const Token* origin = scopeEnd ? endBlock : nullptr;
753+
754+
// Update program state
755+
pms.removeModifiedVars(anchor);
756+
pms.addState(anchor, getProgramState());
757+
pms.assume(tok, state, flags & Assume::ContainerEmpty, origin);
758+
759+
// The false path (the true path uses scopeEnd above): record the assumed state where control
760+
// continues - the end of the else block, or the closing brace when there is no else - so it
761+
// reaches the enclosing scope.
762+
if (isCondBlock && !(flags & Assume::Pending) && !state) {
763+
if (Token::simpleMatch(endBlock, "} else {"))
764+
pms.addState(endBlock->linkAt(2)->previous(), getProgramState());
765+
else
766+
pms.addState(endBlock, getProgramState());
750767
}
751768

752769
if (!(flags & Assume::Quiet)) {

test/testautovariables.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4941,7 +4941,9 @@ class TestAutoVariables : public TestFixture {
49414941
" return *iPtr;\n"
49424942
" return 0;\n"
49434943
"}");
4944-
ASSERT_EQUALS("[test.cpp:5:16] -> [test.cpp:4:13] -> [test.cpp:8:17]: (error) Using pointer to local variable 'x' that is out of scope. [invalidLifetime]\n", errout_str());
4944+
ASSERT_EQUALS(
4945+
"[test.cpp:5:16] -> [test.cpp:7:10] -> [test.cpp:4:13] -> [test.cpp:8:17]: (error) Using pointer to local variable 'x' that is out of scope. [invalidLifetime]\n",
4946+
errout_str());
49454947

49464948
// #11753
49474949
check("int main(int argc, const char *argv[]) {\n"

test/testcondition.cpp

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4911,6 +4911,30 @@ class TestCondition : public TestFixture {
49114911
ASSERT_EQUALS("[test.cpp:3:10]: (style) Condition 'b()' is always false [knownConditionTrueFalse]\n"
49124912
"[test.cpp:4:9]: (style) Condition '!b()' is always true [knownConditionTrueFalse]\n",
49134913
errout_str());
4914+
4915+
check("int g();\n" // a value modified inside a nested branch must be lowered to possible
4916+
"void f(int outer, int inner) {\n"
4917+
" int bits = 0;\n"
4918+
" if (outer) {\n"
4919+
" if (inner == 1)\n"
4920+
" bits = g();\n"
4921+
" }\n"
4922+
" if (bits > 0) {}\n"
4923+
"}\n");
4924+
ASSERT_EQUALS("", errout_str());
4925+
4926+
check("int g();\n" // the modifying branch has an escaping sibling - still must be lowered
4927+
"void f(int t, int u) {\n"
4928+
" int v = 0;\n"
4929+
" if (t) {\n"
4930+
" if (u == 2)\n"
4931+
" v = g();\n"
4932+
" else\n"
4933+
" return;\n"
4934+
" }\n"
4935+
" if (v > 0) {}\n"
4936+
"}\n");
4937+
ASSERT_EQUALS("", errout_str());
49144938
}
49154939

49164940
void alwaysTrueSymbolic()

0 commit comments

Comments
 (0)