- Notifications
You must be signed in to change notification settings - Fork1.7k
Java: Add AnnotatedExitNodes to the CFG.#19885
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Pull Request Overview
This PR enhances the Java CFG by distinguishing normal and exceptional exit nodes and updates the PathSanitizer query to use the new normal-exit node.
- Introduce
AnnotatedExitNode(withNormalExitNodeandExceptionalExitNode) in the CFG. - Adjust the
succfunction to emit annotated exit nodes before the existingExitNode. - Refactor
PathSanitizer.qllto useControlFlow::NormalExitNodein its validation logic.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
java/ql/lib/semmle/code/java/security/PathSanitizer.qll | Swapped out rawExitNode references forControlFlow::NormalExitNode. |
java/ql/lib/semmle/code/java/ControlFlowGraph.qll | ImportedBoolean, addedAnnotatedExitNode types, and updatedsucc. |
Comments suppressed due to low confidence (2)
java/ql/lib/semmle/code/java/ControlFlowGraph.qll:207
- The
toString()implementation usesnormal = true(assignment) instead of checking the field value, which mutates the node and may invert its state. Consider using a conditional based onnormal(e.g.normal and result = "Normal Exit" or not normal and result = "Exceptional Exit") without assignments.
normal = true and result = "Normal Exit"java/ql/lib/semmle/code/java/ControlFlowGraph.qll:83
- [nitpick] Importing
codeql.util.Booleanmay shadow the primitivebooleantype used in the node definitions. Verify that this import is necessary or remove it to avoid confusion between QL and Java boolean types.
private import codeql.util.BooleanUh oh!
There was an error while loading.Please reload this page.
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.
Did you consider switching post-dominance to use the normal exit node? That is what we do in C#, Ruby, and Rust.
Yes, a bit. I agree that's probably more generally useful, but there might be cases where both variants are relevant. OTOH I don't think we have that many uses of post-dominance. First things first, though, I want this change in order to be able to more easily identify guard-wrappers that throw. |
8467ad7 to42d1e96Compare42d1e96 to448cc82Compare1485d70 intogithub:mainUh oh!
There was an error while loading.Please reload this page.
This adds distinct CFG nodes for normal termination of a callable vs. an exception being raised. This also aligns things with other languages using the shared control flow library.