-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
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 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 |
0f20906
to
8d747e9
Compare
Dca looks good. A bunch of FPs for |
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.
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 { |
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.
I'm not sure we should use the overridable
terminology here; how about simply Callable
(and Call
below)?
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.
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( |
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.
Could we rename this predicate to wrapperGuard
?
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.
done.
645e4c8
to
2909def
Compare
This improves the shared library implementation of guard wrappers in several ways. Commit-by-commit review is strongly encouraged.
CustomGuard
/WrapperGuard
module.BarrierGuard
to support wrapped sanitizers.