Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aschackmull
Copy link
Contributor

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.

@Copilot Copilot AI review requested due to automatic review settings June 26, 2025 09:48
@aschackmull aschackmull requested a review from a team as a code owner June 26, 2025 09:48
@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Jun 26, 2025
@github-actions github-actions bot added the Java label Jun 26, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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 (with NormalExitNode and ExceptionalExitNode) in the CFG.
  • Adjust the succ function to emit annotated exit nodes before the existing ExitNode.
  • Refactor PathSanitizer.qll to use ControlFlow::NormalExitNode in 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 raw ExitNode references for ControlFlow::NormalExitNode.
java/ql/lib/semmle/code/java/ControlFlowGraph.qll Imported Boolean, added AnnotatedExitNode types, and updated succ.
Comments suppressed due to low confidence (2)

java/ql/lib/semmle/code/java/ControlFlowGraph.qll:207

  • The toString() implementation uses normal = true (assignment) instead of checking the field value, which mutates the node and may invert its state. Consider using a conditional based on normal (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.Boolean may shadow the primitive boolean type 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.Boolean
Copy link
Contributor

@hvitved hvitved left a 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.

@aschackmull
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java no-change-note-required This PR does not need a change note
2 participants