-
Notifications
You must be signed in to change notification settings - Fork 0
fix: missed where opporunity false positive #2
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
base: qa/agent-github-codeql/pr-02-22484/base
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,26 @@ private int numStmts(ForeachStmt fes) { | |
| else result = 1 | ||
| } | ||
|
|
||
| private predicate terminatesCallable(Stmt s) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH The name terminatesCallable is misleading: it returns true for BreakStmt, which does not terminate the callable, only the enclosing loop. Impact: The name terminatesCallable is misleading: it returns true for BreakStmt, which does not terminate the callable, only the enclosing loop. A future maintainer reading missedWhereOpportunity will assume the predicate means method/iterator termination and will not realize break is included, leading to incorrect modifications. Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH The recursive definition of terminatesCallable is hard to follow because it mixes structural recursion (BlockStmt, IfStmt) with leaf cases (ReturnStmt, YieldBreakStmt, ThrowStmt, B Impact: The recursive definition of terminatesCallable is hard to follow because it mixes structural recursion (BlockStmt, IfStmt) with leaf cases (ReturnStmt, YieldBreakStmt, ThrowStmt, BreakStmt) without documenting the intended control-flow semantics. There is no comment explaining why BreakStmt is included or why loops are excluded, making the logic opaque to a newcomer. Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH The predicate does not handle ContinueStmt, GotoStmt, or labeled statements. Impact: The predicate does not handle ContinueStmt, GotoStmt, or labeled statements. A goto to a label outside the loop or a continue in a nested loop can terminate or alter control flow in ways the predicate misclassifies, leading to incorrect alert suppression or missed alerts. Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH The change modifies a query that flags missed Where opportunities, but the new predicate's control-flow analysis is unsound for exception handling. Impact: The change modifies a query that flags missed Where opportunities, but the new predicate's control-flow analysis is unsound for exception handling. A try/catch/finally block where the try contains a return but the finally throws or returns is not modeled, so the query may suppress alerts for loops whose filtered branch does not actually terminate the callable, or flag loops that do. This could hide real code-quality… Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
| exists(Stmt stripped | stripped = s.stripSingletonBlocks() | | ||
| stripped instanceof ReturnStmt | ||
| or | ||
| stripped instanceof YieldBreakStmt | ||
| or | ||
| stripped instanceof ThrowStmt | ||
| or | ||
| stripped instanceof BreakStmt | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · CRITICAL The predicate treats BreakStmt as terminating the callable, but a break only exits the nearest loop/switch, not the method or iterator. Impact: The predicate treats BreakStmt as terminating the callable, but a break only exits the nearest loop/switch, not the method or iterator. In a foreach nested inside another loop, a break in the filtered branch exits only the inner foreach and continues the outer loop, so the loop is still filtering work and should be flagged. This will suppress true positives. Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright. |
||
| or | ||
| stripped = any(BlockStmt b | terminatesCallable(b.getLastStmt())) | ||
| or | ||
| stripped = | ||
| any(IfStmt nested | | ||
| terminatesCallable(nested.getThen()) and | ||
| terminatesCallable(nested.getElse()) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| /** Holds if the type's qualified name is "System.Linq.Enumerable" */ | ||
| predicate isEnumerableType(ValueOrRefType t) { | ||
| t.hasFullyQualifiedName("System.Linq", "Enumerable") | ||
|
|
@@ -152,7 +172,8 @@ predicate missedWhereOpportunity(ForeachStmtGenericEnumerable fes, IfStmt is) { | |
| is.getThen() instanceof ContinueStmt | ||
| or | ||
| not exists(is.getElse()) and | ||
| numStmts(fes) = 1 | ||
| numStmts(fes) = 1 and | ||
| not terminatesCallable(is.getThen()) | ||
| ) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| class MissedWhereOpportunityGood | ||
| { | ||
| public int? FindFirstEven(System.Collections.Generic.IEnumerable<int> values) | ||
| { | ||
| foreach (int value in values) | ||
| { | ||
| if (value % 2 == 0) | ||
| return value; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * The `cs/linq/missed-where` query no longer flags `foreach` loops where the matching branch terminates the method, iterator, or loop instead of continuing with filtered loop work. |
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.
Shipwright · CRITICAL
terminatesCallable is recursive and has no base case for loops or switch statements.
Impact: terminatesCallable is recursive and has no base case for loops or switch statements. A foreach/while/for/do loop whose body contains a return will not be recognized as terminating, causing false positives. More critically, the recursion through BlockStmt.getLastStmt() and IfStmt branches can cycle or fail to terminate on malformed/cyclic ASTs, and the predicate lacks a recursion bound, risking evaluator non-terminat…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.