Exclude bounds-check arithmetic from tainted-arithmetic sinks#21608
Open
MarkLee131 wants to merge 5 commits intogithub:mainfrom
Open
Exclude bounds-check arithmetic from tainted-arithmetic sinks#21608MarkLee131 wants to merge 5 commits intogithub:mainfrom
MarkLee131 wants to merge 5 commits intogithub:mainfrom
Conversation
The java/tainted-arithmetic query now recognizes when an arithmetic expression appears directly as an operand of a comparison (e.g., `if (off + len > array.length)`). Such expressions are bounds checks, not vulnerable computations, and are excluded via the existing overflowIrrelevant predicate. Add test cases for bounds-checking patterns that should not be flagged.
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts the Java java/tainted-arithmetic query to avoid reporting arithmetic that is used purely as part of bounds-check comparisons (addressing #21607), and adds regression coverage + a changelog entry.
Changes:
- Extend the “overflow/underflow irrelevant” logic to treat arithmetic used directly in comparison-based bounds checks as non-sinks.
- Add new “GOOD” bounds-checking patterns to the CWE-190 query tests.
- Add a minor analysis change note describing the reduced false positives.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| java/ql/lib/semmle/code/java/security/ArithmeticCommon.qll | Updates the exclusion logic for overflow/underflow-relevant arithmetic to cover bounds-check comparison operands. |
| java/ql/test/query-tests/security/CWE-190/semmle/tests/ArithmeticTainted.java | Adds new “GOOD” bounds-check examples intended to ensure the query doesn’t flag these patterns. |
| java/ql/lib/change-notes/2026-03-28-tainted-arithmetic-bounds-check.md | Documents the behavioral change for the java/tainted-arithmetic query. |
java/ql/lib/change-notes/2026-03-28-tainted-arithmetic-bounds-check.md
Outdated
Show resolved
Hide resolved
java/ql/test/query-tests/security/CWE-190/semmle/tests/ArithmeticTainted.java
Show resolved
Hide resolved
owen-mc
requested changes
Mar 28, 2026
Contributor
owen-mc
left a comment
There was a problem hiding this comment.
Thank you for this contribution. Special thanks for giving real world examples where this false positive occurs - this really helps us to see that it is worth fixing. This PR just needs a few small changes and it can be merged.
java/ql/test/query-tests/security/CWE-190/semmle/tests/ArithmeticTainted.java
Show resolved
Hide resolved
java/ql/lib/change-notes/2026-03-28-tainted-arithmetic-bounds-check.md
Outdated
Show resolved
Hide resolved
…ticTainted.java Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
…check.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Clarify that arithmeticUsedInBoundsCheck applies to if-condition comparisons, not all comparisons - Update expected test line numbers to reflect added test calls
Contributor
Author
|
Thanks for review. I have finished the changes and tested it locally. My test cmd is: Output: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix #21607 :
The java/tainted-arithmetic query now recognizes when an arithmetic expression appears directly as an operand of a comparison (e.g.,
if (off + len > array.length)). Such expressions are bounds checks, not vulnerable computations, and are excluded via the existing overflowIrrelevant predicate.Add test cases for bounds-checking patterns that should not be flagged.