Skip to content

Guards: Improve support for wrapped guards #20121

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

Merged
merged 11 commits into from
Aug 7, 2025

Conversation

aschackmull
Copy link
Contributor

This improves the shared library implementation of guard wrappers in several ways. Commit-by-commit review is strongly encouraged.

  • Improves the Guards instantiation interface by removing the nesting of parameteristion of the CustomGuard/WrapperGuard module.
  • Generalises wrappers to support all sorts of return values - not just booleans.
  • Generalises wrappers to also support exceptional/normal return.
  • Adds a (currently unused) module intended to be connected to BarrierGuard to support wrapped sanitizers.

@Copilot Copilot AI review requested due to automatic review settings July 24, 2025 11:12
@aschackmull aschackmull requested a review from a team as a code owner July 24, 2025 11:12
@github-actions github-actions bot added the Java label Jul 24, 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 improves the shared library implementation of guard wrappers by generalizing the wrapper support and simplifying the interface. The changes extend wrapper functionality to handle all return value types (not just booleans), support both normal and exceptional returns, and prepare for wrapped sanitizer support.

  • Removes the nested parameterization of CustomGuard/WrapperGuard modules to simplify the interface
  • Generalizes wrapper support to handle all return value types and exceptional/normal returns
  • Adds infrastructure for wrapped sanitizers through new ValidationWrapper modules

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
shared/controlflow/codeql/controlflow/Guards.qll Core implementation with generalized wrapper support, new validation wrapper modules, and simplified interface
java/ql/lib/semmle/code/java/controlflow/Guards.qll Java-specific implementation updates to use new wrapper interface and remove old CustomGuard module
java/ql/test/library-tests/guards/Guards.java Test cases for new wrapper functionality including non-boolean returns and exception handling
java/ql/test/library-tests/guards/GuardsInline.expected Expected test output for new wrapper test cases

@aschackmull
Copy link
Contributor Author

Dca looks good. A bunch of FPs for java/dereferenced-value-may-be-null are removed.

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.

LGTM, thanks a lot for splitting the PR into several commits. Some minor comments.

/** A non-overridable method with a boolean return value. */
class BooleanMethod {
/** A non-overridable method. */
class NonOverridableMethod {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should use the overridable terminology here; how about simply Callable (and Call below)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure. Firstly, yes, we should generalise from Method to Callable (since constructors can also act as wrapper guards) - if I do that here, then I should probably rerun dca. Regarding the non-overridable prefix, we'll need to do something, since we can't simply use the entire virtual dispatch call graph as basis for computing wrappers. And I don't think we should reuse the Callable name here if it is supposed to mean non-overridable callable.
We could of course move the responsibility for a heuristic to replace non-overridability into the guards library - e.g. count dispatch targets and restrict to cases with a unique dispatch target, but that's unlikely to be enough as certain methods are intended to be overridden even though their overrides might not exist in the db. It's possible that we could conjure suitable heuristics to rule out those cases - perhaps checking for trivial method bodies like always throwing.
But if we are to make such a change then I think it merits its own PR.

* that the argument has the value `val`.
*/
private BooleanMethod customGuard(ParameterPosition ppos, boolean retval, GuardValue val) {
private NonOverridableMethod customGuard(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we rename this predicate to wrapperGuard?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@aschackmull aschackmull force-pushed the guards/wrapperguard branch from 645e4c8 to 2909def Compare August 7, 2025 12:52
@aschackmull aschackmull merged commit 3674966 into github:main Aug 7, 2025
43 checks passed
@aschackmull aschackmull deleted the guards/wrapperguard branch August 7, 2025 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants