-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
Fix #12612 FP uninitvar with pointer alias in subfunction #7249
base: main
Are you sure you want to change the base?
Conversation
return values.count(tok->varId()) > 0 || | ||
std::any_of(values.begin(), values.end(), [&](const std::pair<nonneg int, ValueFlow::Value>& p) { | ||
return p.second.isUninitValue() && p.second.tokvalue->varId() == tok->varId(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this extra check needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which check exactly? We now match in case there is an alias that has an uninit value pointing to the variable in question. But I'm not sure if it is correct to only check for uninit values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should happen in the analyzeToken function, not here. We are already checking for aliases there as well.
Finally the tokvalue is not always an alias either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
analyzeToken()
has this:
Action la = analyzeLifetime(lifeTok);
if (la.matches()) { ...
So matches()
needs to return true for the propagation of the uninit value to stop.
I looked at how that works for local code and tried to make the same stuff happen with a subfunction/valueFlowInjectParameter()
@@ -586,7 +586,7 @@ struct ValueFlowAnalyzer : Analyzer { | |||
} else if (ref->isUnaryOp("*") && !match(ref->astOperand1())) { | |||
const Token* lifeTok = nullptr; | |||
for (const ValueFlow::Value& v:ref->astOperand1()->values()) { | |||
if (!v.isLocalLifetimeValue()) | |||
if (!v.isLocalLifetimeValue() && !v.isSubFunctionLifetimeValue()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of skipping subfunction lifetimes, you could just check that the path is the same if the propagated value's path(ie the value returned from getValue(lifetok)
) is not zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subfunction lifetimes were skipped before this change, and we didn't proceed to analyzeLifetime()
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I read it backwards. Its enabling it. We still need to check the correct path to avoid FPs.
No description provided.