Skip to content

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

Merged
orbitcowboy merged 2 commits intocppcheck-opensource:mainfrom
chrchr-github:chr_Fix11353
Aug 11, 2023
Merged

Fix #11353 FP uninitvar for struct member set via pointer#5314
orbitcowboy merged 2 commits intocppcheck-opensource:mainfrom
chrchr-github:chr_Fix11353

Conversation

@chrchr-github
Copy link
Copy Markdown
Collaborator

No description provided.

@orbitcowboy orbitcowboy merged commit 720ae01 into cppcheck-opensource:main Aug 11, 2023
@chrchr-github chrchr-github deleted the chr_Fix11353 branch August 11, 2023 17:01
Comment thread lib/valueflow.cpp
bool match(const Token* tok) const override
{
return SubExpressionAnalyzer::match(tok) ||
(Token::simpleMatch(tok->astParent(), ".") && SubExpressionAnalyzer::match(tok->astParent()));
Copy link
Copy Markdown
Contributor

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

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

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.

Comment thread lib/valueflow.cpp
continue;

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

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

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

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

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

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

Choose a reason for hiding this comment

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

Fixed this in #5320.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants