Skip to content

Commit e772b35

Browse files
aadanendanmar
andauthored
Fix #15005: FP uninitdata with compound assignment or increment (#8822)
isVariableUsage() currently returns nullptr for the lhs of assignment operators, because unless it's a pointer that gets dereferenced, it is just getting overwritten and its potentially uninitialized value is not read. I extend this idea to compound assignment and increment/decrement operators. While working on this, I found some issues with CTU analysis. ``` valueFlowUninit("void increment(int& i) { ++i; }\n" // #6475 "int f() {\n" " int n;\n" " increment(n);\n" " return n;\n" "}\n"); ASSERT_EQUALS("[test.cpp:4:15] -> [test.cpp:1:28]: (warning) Uninitialized variable: i [uninitvar]\n", errout_str()); ``` this test only works because of the false positive "usage" from the increment operator. For example, this test breaks if we use regular assignment because of the current code properly handling "=" this code ``` void increment(int& i) { i = i + 1; } int f() { int n; increment(n); return n; } ``` misses the ctu error, and only throws ``` Checking examples/ctu3.cpp ... examples/ctu3.cpp:1:30: warning: Uninitialized variable: i [uninitvar] void increment(int& i) { i = i + 1; } ^ examples/ctu3.cpp:4:15: note: Calling function 'increment', 1st argument 'n' value is <Uninit> increment(n); ^ examples/ctu3.cpp:1:30: note: Uninitialized variable: i void increment(int& i) { i = i + 1; } ^ ``` I think that the problem has to do with ``` // NOLINTNEXTLINE(readability-non-const-parameter) - used as callback so we need to preserve the signature static bool isVariableUsage(const Settings &settings, const Token *vartok, MathLib::bigint *value) { (void)value; return !!CheckUninitVarImpl::isVariableUsage(vartok, settings.library, true, CheckUninitVarImpl::Alloc::ARRAY); } ``` the hardcoded pointer=true and alloc=ARRAY assignments tell isVariableUsage to return nullptr if it i isn't dereferenced, which it isn't. If we are going to hardcode these values to be this conservative then this test should fail because it currently only passes due to the FP bug I want to fix. I currently have a workaround where I use vartok->variable() to determine whether something is actually a pointer/array. Maybe there can be another PR where we rework the ctu/uninitvar connection. Notably since I don't touch the "=" branch of the code the false negative I mentioned above remains unfixed. Let me know if I should apply my vartok workaround to that code in this PR, if it should be another PR, or if its just the wrong idea overall. --------- Co-authored-by: Daniel Marjamäki <daniel.marjamaki@gmail.com>
1 parent 8f0dd64 commit e772b35

2 files changed

Lines changed: 76 additions & 1 deletion

File tree

‎lib/checkuninitvar.cpp‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -834,7 +834,7 @@ bool CheckUninitVarImpl::checkScopeForVariable(const Token *tok, const Variable&
834834
}
835835
}
836836
}
837-
if (Token::simpleMatch(parent->astParent(), "=") && astIsLHS(parent)) {
837+
if (Token::Match(parent->astParent(), "%assign%") && astIsLHS(parent)) {
838838
const Token *eq = parent->astParent();
839839
if (const Token *errorToken = checkExpr(eq->astOperand2(), var, *alloc, number_of_if==0)) {
840840
if (!suppressErrors)
@@ -1295,6 +1295,19 @@ const Token* CheckUninitVarImpl::isVariableUsage(const Token *vartok, const Libr
12951295
}
12961296
if (alloc != NO_ALLOC && astIsRhs(valueExpr))
12971297
return nullptr;
1298+
} else if (tok->astParent() && (tok->astParent()->isAssignmentOp() || tok->astParent()->isIncDecOp())) {
1299+
// NO_ALLOC -> no matter what we read the uninitialized memory.
1300+
// pointer/array -> safe, as long as we don't dereference
1301+
bool isPtr = pointer;
1302+
bool isArr = alloc == ARRAY;
1303+
// "pointer" and "alloc" get set for non-ptr non-array var
1304+
if (vartok && vartok->variable()) {
1305+
isPtr = vartok->variable()->isPointer();
1306+
isArr = vartok->variable()->isArray();
1307+
}
1308+
if ((alloc != NO_ALLOC) && ((isPtr || isArr) && !derefValue)) {
1309+
return nullptr;
1310+
}
12981311
}
12991312
}
13001313

‎test/testuninitvar.cpp‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2202,6 +2202,58 @@ class TestUninitVar : public TestFixture {
22022202
" return i;\n"
22032203
"}\n");
22042204
ASSERT_EQUALS("", errout_str());
2205+
2206+
// #15005
2207+
checkUninitVar("void f() {\n"
2208+
" char *p = new char;\n"
2209+
" p += 1;\n"
2210+
" delete (p - 1);\n"
2211+
"}\n");
2212+
ASSERT_EQUALS("", errout_str());
2213+
2214+
checkUninitVar("void f() {\n"
2215+
" char *p = new char;\n"
2216+
" *p += 1;\n"
2217+
" delete p;\n"
2218+
"}\n");
2219+
ASSERT_EQUALS("[test.cpp:3:6]: (error) Memory is allocated but not initialized: p [uninitdata]\n", errout_str());
2220+
2221+
checkUninitVar("void f() {\n"
2222+
" char *p = new char;\n"
2223+
" (*p)++;\n"
2224+
" delete p;\n"
2225+
"}\n");
2226+
ASSERT_EQUALS("[test.cpp:3:7]: (error) Memory is allocated but not initialized: p [uninitdata]\n", errout_str());
2227+
2228+
checkUninitVar("void f() {\n"
2229+
" char *p = new char;\n"
2230+
" p[0]++;\n"
2231+
" delete p;\n"
2232+
"}\n");
2233+
ASSERT_EQUALS("[test.cpp:3:5]: (error) Memory is allocated but not initialized: p [uninitdata]\n", errout_str());
2234+
2235+
checkUninitVar("void f() {\n"
2236+
" char *buf = (char *)malloc(1);\n"
2237+
" if (!buf)\n"
2238+
" return NULL;\n"
2239+
" buf += buf[0];\n"
2240+
" free(buf);\n"
2241+
"}\n");
2242+
ASSERT_EQUALS("[test.cpp:5:15]: (error) Memory is allocated but not initialized: buf[0] [uninitdata]\n", errout_str());
2243+
2244+
checkUninitVar("void g() {\n"
2245+
" int* p = new int;\n"
2246+
" p++;\n"
2247+
" delete (p - 1);\n"
2248+
"}\n");
2249+
ASSERT_EQUALS("", errout_str());
2250+
2251+
checkUninitVar("void g() {\n"
2252+
" int* p = new int;\n"
2253+
" ++p;\n"
2254+
" delete (p - 1);\n"
2255+
"}\n");
2256+
ASSERT_EQUALS("", errout_str());
22052257
}
22062258

22072259
// class / struct..
@@ -8177,6 +8229,16 @@ class TestUninitVar : public TestFixture {
81778229
"}\n");
81788230
ASSERT_EQUALS("[test.cpp:6:4] -> [test.cpp:2:10]: (error) Using argument p that points at uninitialized variable x [ctuuninitvar]\n", errout_str());
81798231

8232+
ctu("void f(int *p) {\n"
8233+
" a += *p;\n"
8234+
"}\n"
8235+
"int main() {\n"
8236+
" int x;\n"
8237+
" f(&x);\n"
8238+
"}\n");
8239+
ASSERT_EQUALS("[test.cpp:6:4] -> [test.cpp:2:11]: (error) Using argument p that points at uninitialized variable x [ctuuninitvar]\n", errout_str());
8240+
8241+
81808242
ctu("void use(int *p) { a = *p + 3; }\n"
81818243
"void call(int x, int *p) { x++; use(p); }\n"
81828244
"int main() {\n"

0 commit comments

Comments
 (0)