Skip to content

Fix #11353 FP uninitvar for struct member set via pointer #5314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
orbitcowboy merged 2 commits into from
Aug 11, 2023
Merged

Fix #11353 FP uninitvar for struct member set via pointer #5314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b452d70
Fix #11353 FP uninitvar for struct member set via pointer
chrchr-github Aug 11, 2023
948f5f2
Fix test
chrchr-github Aug 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3295,6 +3295,12 @@ struct MemberExpressionAnalyzer : SubExpressionAnalyzer {
: SubExpressionAnalyzer(e, std::move(val), t, s), varname(std::move(varname))
{}

bool match(const Token* tok) const override
{
return SubExpressionAnalyzer::match(tok) ||
(Token::simpleMatch(tok->astParent(), ".") && SubExpressionAnalyzer::match(tok->astParent()));
Copy link
Copy Markdown
Contributor

@pfultz2 pfultz2 Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we matching the token when the parent matches? That seems wrong.

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise x (in s.x, lifeTok) and s (expr) don't match.

Copy link
Copy Markdown
Contributor

@pfultz2 pfultz2 Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, we probably need to create a different match function for this purpose as there are other uses like this that will suffer the same issue.

}

bool submatch(const Token* tok, bool exact) const override
{
if (!Token::Match(tok, ". %var%"))
Expand Down Expand Up @@ -7965,6 +7971,7 @@ static void valueFlowUninit(TokenList& tokenlist, const Settings* settings)
Token* start = findStartToken(var, tok->next(), &settings->library);

std::map<Token*, ValueFlow::Value> partialReads;
Analyzer::Result result;
if (const Scope* scope = var->typeScope()) {
if (Token::findsimplematch(scope->bodyStart, "union", scope->bodyEnd))
continue;
Expand All @@ -7979,7 +7986,7 @@ static void valueFlowUninit(TokenList& tokenlist, const Settings* settings)
continue;
}
MemberExpressionAnalyzer analyzer(memVar.nameToken()->str(), tok, uninitValue, tokenlist, settings);
valueFlowGenericForward(start, tok->scope()->bodyEnd, analyzer, *settings);
result = valueFlowGenericForward(start, tok->scope()->bodyEnd, analyzer, *settings);

for (auto&& p : *analyzer.partialReads) {
Token* tok2 = p.first;
Expand Down Expand Up @@ -8009,7 +8016,8 @@ static void valueFlowUninit(TokenList& tokenlist, const Settings* settings)
if (partial)
continue;

valueFlowForward(start, tok->scope()->bodyEnd, var->nameToken(), uninitValue, tokenlist, settings);
if (result.terminate != Analyzer::Terminate::Modified)
Copy link
Copy Markdown
Contributor

@pfultz2 pfultz2 Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems strange. Why are we skipping forwarding only when the last variable in the list terminates as modified?

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens for every variable, right?
Why are there even two forwards? Anyway, the second call was setting an uninit value (for return s) when the MemberExpressionAnalyzer had terminated after an assignment, so I tried to prevent that.

Copy link
Copy Markdown
Contributor

@pfultz2 pfultz2 Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens for every variable, right?
Why are there even two forwards?

There is a forward for every variable in the struct.

Anyway, the second call was setting an uninit value (for return s) when the MemberExpressionAnalyzer had terminated after an assignment, so I tried to prevent that.

But this starts at the same spot, so why does the last valueFlowForward not catch the assignment?

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was, because it doesn't do the special MemberExpressionAnalyzer stuff?

Copy link
Copy Markdown
Contributor

@pfultz2 pfultz2 Aug 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea thats it. And the special matching stuff is too just match the lifetimes since we point to class variables. Ideally, we should update the lifetime to point to the correct token, but this would require changing a lot of assumptions through out.

Copy link
Copy Markdown
Contributor

@pfultz2 pfultz2 Aug 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this in #5320.

valueFlowForward(start, tok->scope()->bodyEnd, var->nameToken(), uninitValue, tokenlist, settings);
}
}

Expand Down
23 changes: 16 additions & 7 deletions test/testuninitvar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6139,7 +6139,7 @@ class TestUninitVar : public TestFixture {
" memcpy(wcsin, x, sizeof(wcsstruct));\n" // <- warning
" x->wcsprm = NULL;\n" // <- no warning
"}");
ASSERT_EQUALS("[test.cpp:7]: (error) Uninitialized variable: x\n", errout.str());
ASSERT_EQUALS("[test.cpp:7]: (error) Uninitialized variable: x.wcsprm\n", errout.str());

valueFlowUninit("struct wcsstruct {\n"
" int *wcsprm;\n"
Expand Down Expand Up @@ -6222,7 +6222,7 @@ class TestUninitVar : public TestFixture {
" int * x = &s1.x;\n"
" struct S s2 = {*x, 0};\n"
"}");
ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:9]: (error) Uninitialized variable: *x\n", errout.str());
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: *x\n", errout.str());

valueFlowUninit("struct S {\n"
" int x;\n"
Expand All @@ -6235,7 +6235,7 @@ class TestUninitVar : public TestFixture {
" int * x = &s1.x;\n"
" s2.x = *x;\n"
"}");
ASSERT_EQUALS("[test.cpp:9] -> [test.cpp:10]: (error) Uninitialized variable: *x\n", errout.str());
ASSERT_EQUALS("[test.cpp:10]: (error) Uninitialized variable: *x\n", errout.str());

valueFlowUninit("void f(bool * x) {\n"
" *x = false;\n"
Expand Down Expand Up @@ -6855,7 +6855,7 @@ class TestUninitVar : public TestFixture {
" int a = ab.a;\n"
" int b = ab.b;\n"
"}");
TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: ab.b\n", "", errout.str());
ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: ab.b\n", errout.str());

// STL class member
valueFlowUninit("struct A {\n"
Expand Down Expand Up @@ -6967,7 +6967,7 @@ class TestUninitVar : public TestFixture {
" memcpy(in, s, sizeof(S));\n"
" s->p = NULL;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:4]: (error) Uninitialized variable: s\n",
ASSERT_EQUALS("[test.cpp:4]: (error) Uninitialized variable: s.p\n",
errout.str());

valueFlowUninit("struct S {\n" // #11321
Expand Down Expand Up @@ -7228,7 +7228,7 @@ class TestUninitVar : public TestFixture {
" A::B b;\n"
" x.push_back(b);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: b\n", errout.str());
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: b.i\n", errout.str());

valueFlowUninit("struct A {\n"
" struct B {\n"
Expand All @@ -7252,7 +7252,7 @@ class TestUninitVar : public TestFixture {
" A a;\n"
" x.push_back(a);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: a\n", errout.str());
ASSERT_EQUALS("[test.cpp:9]: (error) Uninitialized variable: a.j\n", errout.str());

valueFlowUninit("struct S { struct T { int* p; } t[2]; };\n" // #11018
"void f() {\n"
Expand All @@ -7272,6 +7272,15 @@ class TestUninitVar : public TestFixture {
" int y = x < (1, s.i);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: s.i\n", errout.str());

valueFlowUninit("struct S { int x; };\n" // #11353
"struct S f() {\n"
" struct S s;\n"
" int* p = &s.x;\n"
" *p = 0;\n"
" return s;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}

void ctu_(const char* file, int line, const char code[]) {
Expand Down