Skip to content

Commit 0b9e823

Browse files
pfultz2danmar
pfultz2
authored and
danmar
committed
Fix issue 9305: False positive uninitvar - struct initialized via function (danmar#2123)
1 parent 2942be5 commit 0b9e823

8 files changed

Lines changed: 94 additions & 47 deletions

File tree

‎lib/astutils.cpp‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -910,14 +910,14 @@ bool isReturnScope(const Token * const endToken, const Settings * settings, bool
910910
return false;
911911
}
912912

913-
bool isVariableChangedByFunctionCall(const Token *tok, nonneg int varid, const Settings *settings, bool *inconclusive)
913+
bool isVariableChangedByFunctionCall(const Token *tok, int indirect, nonneg int varid, const Settings *settings, bool *inconclusive)
914914
{
915915
if (!tok)
916916
return false;
917917
if (tok->varId() == varid)
918-
return isVariableChangedByFunctionCall(tok, settings, inconclusive);
919-
return isVariableChangedByFunctionCall(tok->astOperand1(), varid, settings, inconclusive) ||
920-
isVariableChangedByFunctionCall(tok->astOperand2(), varid, settings, inconclusive);
918+
return isVariableChangedByFunctionCall(tok, indirect, settings, inconclusive);
919+
return isVariableChangedByFunctionCall(tok->astOperand1(), indirect, varid, settings, inconclusive) ||
920+
isVariableChangedByFunctionCall(tok->astOperand2(), indirect, varid, settings, inconclusive);
921921
}
922922

923923
static bool isScopeBracket(const Token *tok)
@@ -933,7 +933,7 @@ static bool isScopeBracket(const Token *tok)
933933
return false;
934934
}
935935

936-
bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings, bool *inconclusive)
936+
bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Settings *settings, bool *inconclusive)
937937
{
938938
if (!tok)
939939
return false;
@@ -1038,7 +1038,7 @@ bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings,
10381038

10391039
const Variable *arg = tok->function()->getArgumentVar(argnr);
10401040

1041-
if (addressOf) {
1041+
if (addressOf || (indirect > 0 && arg && arg->isPointer())) {
10421042
if (!(arg && arg->isConst()))
10431043
return true;
10441044
// If const is applied to the pointer, then the value can still be modified
@@ -1049,7 +1049,7 @@ bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings,
10491049
return arg && !arg->isConst() && arg->isReference();
10501050
}
10511051

1052-
bool isVariableChanged(const Token *tok, const Settings *settings, bool cpp, int depth)
1052+
bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, bool cpp, int depth)
10531053
{
10541054
if (!tok)
10551055
return false;
@@ -1104,7 +1104,7 @@ bool isVariableChanged(const Token *tok, const Settings *settings, bool cpp, int
11041104
while (Token::Match(ptok->astParent(), ".|::|["))
11051105
ptok = ptok->astParent();
11061106
bool inconclusive = false;
1107-
bool isChanged = isVariableChangedByFunctionCall(ptok, settings, &inconclusive);
1107+
bool isChanged = isVariableChangedByFunctionCall(ptok, indirect, settings, &inconclusive);
11081108
isChanged |= inconclusive;
11091109
if (isChanged)
11101110
return true;
@@ -1132,10 +1132,10 @@ bool isVariableChanged(const Token *tok, const Settings *settings, bool cpp, int
11321132

11331133
bool isVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth)
11341134
{
1135-
return findVariableChanged(start, end, varid, globalvar, settings, cpp, depth) != nullptr;
1135+
return findVariableChanged(start, end, 0, varid, globalvar, settings, cpp, depth) != nullptr;
11361136
}
11371137

1138-
Token* findVariableChanged(Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth)
1138+
Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth)
11391139
{
11401140
if (!precedes(start, end))
11411141
return nullptr;
@@ -1148,15 +1148,15 @@ Token* findVariableChanged(Token *start, const Token *end, const nonneg int vari
11481148
return tok;
11491149
continue;
11501150
}
1151-
if (isVariableChanged(tok, settings, cpp, depth))
1151+
if (isVariableChanged(tok, indirect, settings, cpp, depth))
11521152
return tok;
11531153
}
11541154
return nullptr;
11551155
}
11561156

1157-
const Token* findVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth)
1157+
const Token* findVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth)
11581158
{
1159-
return findVariableChanged(const_cast<Token*>(start), end, varid, globalvar, settings, cpp, depth);
1159+
return findVariableChanged(const_cast<Token*>(start), end, indirect, varid, globalvar, settings, cpp, depth);
11601160
}
11611161

11621162
bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp, int depth)

‎lib/astutils.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ bool isReturnScope(const Token *endToken, const Settings * settings = nullptr, b
129129
* @param settings program settings
130130
* @param inconclusive pointer to output variable which indicates that the answer of the question is inconclusive
131131
*/
132-
bool isVariableChangedByFunctionCall(const Token *tok, nonneg int varid, const Settings *settings, bool *inconclusive);
132+
bool isVariableChangedByFunctionCall(const Token *tok, int indirect, nonneg int varid, const Settings *settings, bool *inconclusive);
133133

134134
/** Is variable changed by function call?
135135
* In case the answer of the question is inconclusive, e.g. because the function declaration is not known
@@ -139,17 +139,17 @@ bool isVariableChangedByFunctionCall(const Token *tok, nonneg int varid, const S
139139
* @param settings program settings
140140
* @param inconclusive pointer to output variable which indicates that the answer of the question is inconclusive
141141
*/
142-
bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings, bool *inconclusive);
142+
bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Settings *settings, bool *inconclusive);
143143

144144
/** Is variable changed in block of code? */
145145
bool isVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);
146146

147-
bool isVariableChanged(const Token *tok, const Settings *settings, bool cpp, int depth = 20);
147+
bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, bool cpp, int depth = 20);
148148

149149
bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp, int depth = 20);
150150

151-
const Token* findVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);
152-
Token* findVariableChanged(Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);
151+
const Token* findVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);
152+
Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);
153153

154154
bool isAliased(const Variable *var);
155155

‎lib/checkother.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2719,7 +2719,7 @@ void CheckOther::checkAccessOfMovedVariable()
27192719
else
27202720
inconclusive = true;
27212721
} else {
2722-
const bool isVariableChanged = isVariableChangedByFunctionCall(tok, mSettings, &inconclusive);
2722+
const bool isVariableChanged = isVariableChangedByFunctionCall(tok, 0, mSettings, &inconclusive);
27232723
accessOfMoved = !isVariableChanged && checkUninitVar.isVariableUsage(tok, false, CheckUninitVar::NO_ALLOC);
27242724
if (inconclusive) {
27252725
accessOfMoved = !isMovedParameterAllowedForInconclusiveFunction(tok);

‎lib/checkuninitvar.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1357,7 +1357,7 @@ void CheckUninitVar::valueFlowUninit()
13571357
const bool isleaf = isLeafDot(tok) || uninitderef;
13581358
if (Token::Match(tok->astParent(), ". %var%") && !isleaf)
13591359
continue;
1360-
if (!Token::Match(tok->astParent(), ". %name% (") && !uninitderef && isVariableChanged(tok, mSettings, mTokenizer->isCPP()))
1360+
if (!Token::Match(tok->astParent(), ". %name% (") && !uninitderef && isVariableChanged(tok, v->indirect, mSettings, mTokenizer->isCPP()))
13611361
continue;
13621362
uninitvarError(tok, tok->str(), v->errorPath);
13631363
const Token * nextTok = tok;

‎lib/valueflow.cpp‎

Lines changed: 53 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -169,11 +169,13 @@ static void bailoutInternal(TokenList *tokenlist, ErrorLogger *errorLogger, cons
169169
#define bailout(tokenlist, errorLogger, tok, what) bailoutInternal(tokenlist, errorLogger, tok, what, __FILE__, __LINE__, "(valueFlow)")
170170
#endif
171171

172-
static void changeKnownToPossible(std::list<ValueFlow::Value> &values)
172+
static void changeKnownToPossible(std::list<ValueFlow::Value> &values, int indirect=-1)
173173
{
174-
std::list<ValueFlow::Value>::iterator it;
175-
for (it = values.begin(); it != values.end(); ++it)
176-
it->changeKnownToPossible();
174+
for (ValueFlow::Value& v: values) {
175+
if (indirect >= 0 && v.indirect != indirect)
176+
continue;
177+
v.changeKnownToPossible();
178+
}
177179
}
178180

179181
/**
@@ -1690,7 +1692,7 @@ static void valueFlowReverse(TokenList *tokenlist,
16901692

16911693
// assigned by subfunction?
16921694
bool inconclusive = false;
1693-
if (isVariableChangedByFunctionCall(tok2, settings, &inconclusive)) {
1695+
if (isVariableChangedByFunctionCall(tok2, std::max(val.indirect, val2.indirect), settings, &inconclusive)) {
16941696
if (settings->debugwarnings)
16951697
bailout(tokenlist, errorLogger, tok2, "possible assignment of " + tok2->str() + " by subfunction");
16961698
break;
@@ -2071,6 +2073,15 @@ static bool isAliasOf(const Variable * var, const Token *tok, nonneg int varid,
20712073
return false;
20722074
}
20732075

2076+
static std::set<int> getIndirections(const std::list<ValueFlow::Value>& values)
2077+
{
2078+
std::set<int> result;
2079+
std::transform(values.begin(), values.end(), std::inserter(result, result.end()), [](const ValueFlow::Value& v) {
2080+
return std::max(0, v.indirect);
2081+
});
2082+
return result;
2083+
}
2084+
20742085
static bool valueFlowForward(Token * const startToken,
20752086
const Token * const endToken,
20762087
const Variable * const var,
@@ -2204,16 +2215,21 @@ static bool valueFlowForward(Token * const startToken,
22042215
// conditional block of code that assigns variable..
22052216
else if (!tok2->varId() && Token::Match(tok2, "%name% (") && Token::simpleMatch(tok2->linkAt(1), ") {")) {
22062217
// is variable changed in condition?
2207-
Token* tokChanged = findVariableChanged(tok2->next(), tok2->next()->link(), varid, var->isGlobal(), settings, tokenlist->isCPP());
2208-
if (tokChanged != nullptr) {
2209-
// Set the value before bailing
2210-
if (tokChanged->varId() == varid) {
2211-
for (const ValueFlow::Value &v : values) {
2212-
if (!v.isNonValue())
2213-
continue;
2214-
setTokenValue(tokChanged, v, settings);
2218+
for(int i:getIndirections(values)) {
2219+
Token* tokChanged = findVariableChanged(tok2->next(), tok2->next()->link(), i, varid, var->isGlobal(), settings, tokenlist->isCPP());
2220+
if (tokChanged != nullptr) {
2221+
// Set the value before bailing
2222+
if (tokChanged->varId() == varid) {
2223+
for (const ValueFlow::Value &v : values) {
2224+
if (!v.isNonValue())
2225+
continue;
2226+
setTokenValue(tokChanged, v, settings);
2227+
}
22152228
}
2229+
values.remove_if([&](const ValueFlow::Value& v) { return v.indirect == i; });
22162230
}
2231+
}
2232+
if (values.empty()) {
22172233
if (settings->debugwarnings)
22182234
bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, assignment in condition");
22192235
return false;
@@ -2534,8 +2550,10 @@ static bool valueFlowForward(Token * const startToken,
25342550
Token *expr = (condValue.intvalue != 0) ? op2->astOperand1() : op2->astOperand2();
25352551
for (const ValueFlow::Value &v : values)
25362552
valueFlowAST(expr, varid, v, settings);
2537-
if (isVariableChangedByFunctionCall(expr, varid, settings, nullptr))
2538-
changeKnownToPossible(values);
2553+
if (isVariableChangedByFunctionCall(expr, 0, varid, settings, nullptr))
2554+
changeKnownToPossible(values, 0);
2555+
if (isVariableChangedByFunctionCall(expr, 1, varid, settings, nullptr))
2556+
changeKnownToPossible(values, 1);
25392557
} else {
25402558
for (const ValueFlow::Value &v : values) {
25412559
const ProgramMemory programMemory(getProgramMemory(tok2, varid, v));
@@ -2730,16 +2748,24 @@ static bool valueFlowForward(Token * const startToken,
27302748
}
27312749

27322750
// assigned by subfunction?
2733-
bool inconclusive = false;
2734-
if (isVariableChangedByFunctionCall(tok2, settings, &inconclusive)) {
2751+
for(int i:getIndirections(values)) {
2752+
bool inconclusive = false;
2753+
if (isVariableChangedByFunctionCall(tok2, i, settings, &inconclusive)) {
2754+
values.remove_if([&](const ValueFlow::Value& v) { return v.indirect <= i; });
2755+
}
2756+
if (inconclusive) {
2757+
for (ValueFlow::Value &v : values) {
2758+
if (v.indirect != i)
2759+
continue;
2760+
v.setInconclusive();
2761+
}
2762+
}
2763+
}
2764+
if (values.empty()) {
27352765
if (settings->debugwarnings)
27362766
bailout(tokenlist, errorLogger, tok2, "possible assignment of " + tok2->str() + " by subfunction");
27372767
return false;
27382768
}
2739-
if (inconclusive) {
2740-
for (ValueFlow::Value &v : values)
2741-
v.setInconclusive();
2742-
}
27432769
if (tok2->strAt(1) == "." && tok2->next()->originalName() != "->") {
27442770
if (settings->inconclusive) {
27452771
for (ValueFlow::Value &v : values)
@@ -2751,10 +2777,12 @@ static bool valueFlowForward(Token * const startToken,
27512777
}
27522778
}
27532779
// Variable changed
2754-
if (isVariableChanged(tok2, settings, tokenlist->isCPP())) {
2755-
values.remove_if(std::mem_fn(&ValueFlow::Value::isUninitValue));
2756-
}
2757-
} else if (isAliasOf(var, tok2, varid, values) && isVariableChanged(tok2, settings, tokenlist->isCPP())) {
2780+
for(int i:getIndirections(values)) {
2781+
// Remove unintialized values if modified
2782+
if (isVariableChanged(tok2, i, settings, tokenlist->isCPP()))
2783+
values.remove_if([&](const ValueFlow::Value& v) { return v.isUninitValue() && v.indirect <= i; });
2784+
}
2785+
} else if (isAliasOf(var, tok2, varid, values) && isVariableChanged(tok2, 0, settings, tokenlist->isCPP())) {
27582786
if (settings->debugwarnings)
27592787
bailout(tokenlist, errorLogger, tok2, "Alias variable was modified.");
27602788
// Bail at the end of the statement if its in an assignment

‎test/testastutils.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ class TestAstUtils : public TestFixture {
159159
std::istringstream istr(code);
160160
tokenizer.tokenize(istr, "test.cpp");
161161
const Token * const argtok = Token::findmatch(tokenizer.tokens(), pattern);
162-
return ::isVariableChangedByFunctionCall(argtok, &settings, inconclusive);
162+
return ::isVariableChangedByFunctionCall(argtok, 0, &settings, inconclusive);
163163
}
164164

165165
void isVariableChangedByFunctionCall() {

‎test/testsimplifytypedef.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1150,7 +1150,10 @@ class TestSimplifyTypedef : public TestFixture {
11501150
"}";
11511151

11521152
ASSERT_EQUALS(expected, tok(code, false));
1153-
ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:28]: (debug) valueflow.cpp:3109:valueFlowFunctionReturn bailout: function return; nontrivial function body\n", errout.str());
1153+
ASSERT_EQUALS_WITHOUT_LINENUMBERS(
1154+
"[test.cpp:28]: (debug) valueflow.cpp:3109:valueFlowFunctionReturn bailout: function return; nontrivial function body\n"
1155+
"[test.cpp:26]: (debug) valueflow.cpp::valueFlowForward bailout: possible assignment of s by subfunction\n"
1156+
, errout.str());
11541157
}
11551158

11561159
void simplifyTypedef36() {

‎test/testuninitvar.cpp‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4411,6 +4411,22 @@ class TestUninitVar : public TestFixture {
44114411
" return;\n"
44124412
"}\n");
44134413
ASSERT_EQUALS("", errout.str());
4414+
4415+
// # 9305
4416+
valueFlowUninit("struct kf {\n"
4417+
" double x;\n"
4418+
"};\n"
4419+
"void set(kf* k) {\n"
4420+
" k->x = 0;\n"
4421+
"}\n"
4422+
"void cal() {\n"
4423+
" KF b;\n"
4424+
" KF* pb = &b;\n"
4425+
" set( pb);\n"
4426+
" if (pb->x)\n"
4427+
" return;\n"
4428+
"}\n");
4429+
ASSERT_EQUALS("", errout.str());
44144430
}
44154431

44164432
void uninitvar_memberfunction() {

0 commit comments

Comments
 (0)