C++: Annotate field flow tests #3463
Conversation
MathiasVP
changed the title
|
This test would probably be a good candidate to use |
That sounds awesome. @jbj did mention something like this, but I didn't manage to find it. I'll take a look at it and update this PR. |
There was a problem hiding this comment.
I like how the annotations are now self-maintaining, but I miss how the NOT DETECTED and FALSE POSITIVE annotations stood out visually. The f- and f+ annotations are practically invisible and look very similar. That's of course not an issue specific to this PR, so there's no need to address it here.
| sink(b.f.a()); // flow [FALSE POSITIVE through `b2.f.setB` and `b3.f.setB` in AST, NOT DETECTED by IR] | ||
| sink(b.f.b()); // flow [FALSE POSITIVE through `b1.f.setA` (AST) and `b3.f.setA` in AST, NOT DETECTED by IR] | ||
| sink(b.f.a()); //$ast=flow 55:13 $ast=flow 57:13 $f-:ir=flow | ||
| sink(b.f.b()); //$ast=flow 56:13 $ast=flow 58:13 $f-:ir=flow |
There was a problem hiding this comment.
This was previously annotated as a false positive, but I can see that the comment was written before we increased the access-path approximation by one. So perhaps it's no longer a false positive?
There was a problem hiding this comment.
On master there are 4 sources in total for these two sinks (and no false positives). If I add another field lookup (i.e., sink(b.g.f.a())) then we get the additional false positives the comment above mentions.
I agree. I think we should have an alternative syntax for specifying these cases other than |
…e dataflow library
|
I've opened https://github.com/github/codeql-c-analysis-team/issues/75 to come up with a better syntax. This is the first PR that actually uses the f+/- feature, so I'm glad to get feedback on it. |
This PR adds more descriptive annotations to the field flow test directory.