Java: make more queries diff-informed with getASelectedLocation #18340
Conversation
This extension allows queries to be diff-informed even when the elements they select are different from the sources and sinks found by data flow.
This commit also adds a test case that would fail under `codeql test run --check-diff-informed` if not for the override of `getASelectedSourceLocation`. There was no existing such test since all the existing tests used anonymous classes whose location was on the same line as the source.
This and other queries would also benefit from making `RegexFlow` diff-informed. That will come later.
There was a problem hiding this comment.
Copilot reviewed 1 out of 13 changed files in this pull request and generated no comments.
Files not reviewed (12)
- java/ql/lib/semmle/code/java/security/BrokenCryptoAlgorithmQuery.qll: Language not supported
- java/ql/lib/semmle/code/java/security/CommandLineQuery.qll: Language not supported
- java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll: Language not supported
- java/ql/lib/semmle/code/java/security/InsecureTrustManagerQuery.qll: Language not supported
- java/ql/lib/semmle/code/java/security/RandomQuery.qll: Language not supported
- java/ql/lib/semmle/code/java/security/SqlInjectionQuery.qll: Language not supported
- java/ql/lib/semmle/code/java/security/TaintedPermissionsCheckQuery.qll: Language not supported
- java/ql/lib/semmle/code/java/security/UnsafeHostnameVerificationQuery.qll: Language not supported
- java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll: Language not supported
- java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected: Language not supported
- shared/dataflow/codeql/dataflow/DataFlow.qll: Language not supported
- shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll: Language not supported
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
|
Do the DCA results look good to you? I'm not used to interpreting them, but they look like ordinary measurement noise to me. |
|
We agreed offline that I should check the outliers in DCA locally to see if it's noise. I ran these queries on apache/flink with 1 thread and 16 GB RAM, timing the query stage (after Trim Cache and Restart Query Server).
|
|
|
||
| // It's valid to use diff-informed data flow for this configuration because | ||
| // the location of the selected element in the query is contained inside the | ||
| // location of the sink. The query, as a predicate, is used negated in |
There was a problem hiding this comment.
Does containment in another location actually guarantee anything, given that alert-filtering only looks at the start line?
There was a problem hiding this comment.
No, I had misunderstood how this works. I'm working on a follow-up PR to make this configuration exact.
This PR contains the uncontroversial parts of #17846, converting 9 data-flow configurations to be diff-informed either by using the concept of selected source/sink locations or by just marking them as diff-informed because I've checked that all use of the configuration is safe.
I recommend reviewing commit-by-commit.
Ideally, these changes should be tested with
codeql test run --check-diff-informed. Most queries were not in shape for potentially failing that test, and I only converted one:UnsafeHostnameVerification.ql. Other queries either had no test cases with challenging location distributions, or they had noqlref-based tests. In particular, these three old-style inline expectations tests should be converted to qlref and post-processing: