Skip to content

Java: Add AnnotatedExitNodes to the CFG. - #19885

Merged
aschackmull merged 4 commits into
github:mainfrom
aschackmull:java/annotated-exit-cfg
Jul 17, 2025
Merged

Java: Add AnnotatedExitNodes to the CFG.#19885
aschackmull merged 4 commits into
github:mainfrom
aschackmull:java/annotated-exit-cfg

Conversation

@aschackmull

Copy link
Copy Markdown
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.

CopilotAI review requested due to automatic review settings June 26, 2025 09:48
@aschackmull
aschackmull requested a review from a team as a code ownerJune 26, 2025 09:48
@aschackmullaschackmull added the no-change-note-required This PR does not need a change note label Jun 26, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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.

FileDescription
java/ql/lib/semmle/code/java/security/PathSanitizer.qllSwapped out raw ExitNode references for ControlFlow::NormalExitNode.
java/ql/lib/semmle/code/java/ControlFlowGraph.qllImported 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

Comment threadjava/ql/lib/semmle/code/java/ControlFlowGraph.qll
hvitved
hvitved previously approved these changes Jun 26, 2025

@hvitvedhvitved left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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.

@aschackmull
aschackmullforce-pushed the java/annotated-exit-cfg branch from 42d1e96 to 448cc82CompareJuly 17, 2025 09:21
@aschackmull
aschackmull merged commit 1485d70 into github:mainJul 17, 2025
@aschackmull
aschackmull deleted the java/annotated-exit-cfg branch July 17, 2025 13:02
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JavaKotlinno-change-note-requiredThis PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@aschackmull@hvitved@owen-mc