C++: Constructor and assignment models #3733
Conversation
| input.isParameter(idx) and | ||
| output.isReturnValue() and | ||
| not this.(CopyConstructor).hasDataFlow(input, output) and // don't duplicate where we have data flow | ||
| not this.(MoveConstructor).hasDataFlow(input, output) // don't duplicate where we have data flow |
There was a problem hiding this comment.
I find it controversial to model that there is taint from any constructor argument to the constructed object. It's almost like modelling that there is taint from function arguments to function return value for any function.
There was a problem hiding this comment.
To elaborate, we've decided in the team that there should be object-to-field taint flow. This model would introduce field-to-object taint flow, which by transitivity gets us field-to-unrelated-field taint flow.
There was a problem hiding this comment.
Yeah, I see your point that it's similar to any function call taking inputs and producing an object. I've deleted the case for the general constructor but added one for 'conversion constructors' (that are important and much more likely to behave as we expect).
There was a problem hiding this comment.
This seems less dangerous, and I can see that users might reasonably expect C x = a; to propagate taint even if a is not of type C. It's less clear to me whether they'll expect C x(a); to propagate taint from a to x in all cases.
We're not currently using ConversionConstructor in production, and I think it needs a bit of dusting off. Its charpred excludes copy constructors but not move constructors. That's been worked around by excluding move constructors in its getCanonicalQLClass override, but surely that exclusion should be moved to its charpred?
I still find this rule controversial, and I find it likely we'll have to drop it when adding object-to-field flow (https://github.com/github/codeql-c-analysis-team/issues/103). In any case, please run CPP-Differences.
There was a problem hiding this comment.
Its charpred excludes copy constructors but not move constructors.
Yes, I noticed that, I'll fix it in a separate PR.
In any case, please run CPP-Differences.
Diff is running here: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1213/
There was a problem hiding this comment.
... and possibly openjdk as well.
New diff job excluding wireshark and openjdk: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1226/
New diff job of just wireshark: TODO
New diff job of just openjdk: TODO
There was a problem hiding this comment.
Be sure that your branch includes #3754. Otherwise openjdk will continue to fail.
There was a problem hiding this comment.
OK, new attempt with latest everything: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1230/
There was a problem hiding this comment.
Success! - https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1239/
And there are no changes, so that's good.
There was a problem hiding this comment.
I'm surprised there are no changes. But at least that makes me more confident about merging this PR even though it might have less of an impact than I hoped.
…on't want the general case for Constructors any more.
General models for class copy constructors, move constructors, copy assignment and move assignment. Removed the model of the
std::stringconstructor as it's now redundant.