-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Ruby: add overlay annotations to AST/CFG/SSA layers #19989
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
4a725ba
to
3c4a528
Compare
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
I think this is technically a breaking change and therefore needs a change note: if a class in a custom query overrides a predicate from a class that's now local, it's a compiler error. Reviewers should check that no supported extension point is becoming local. |
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, some suggestions for predicates that can be made local.
ruby/ql/lib/codeql/ruby/ast/Call.qll
Outdated
@@ -38,6 +41,7 @@ class Call extends Expr instanceof CallImpl { | |||
* foo :bar "baz", qux: 123 | |||
* ``` | |||
*/ | |||
overlay[global] |
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 think this predicate can reasonably be made local by replacing
p.getKey().getConstantValue().isSymbol(keyword)
below with
keyword = p.getKey().(SymbolLiteral).(StringlikeLiteralImpl).getStringValue()
(requires an private import internal.Literal
import).
@@ -425,6 +429,7 @@ class StringConcatenation extends Expr, TStringConcatenation { | |||
* "foo" "bar#{ n }" | |||
* ``` | |||
*/ | |||
overlay[global] |
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 think this can be made local:
final string getConcatenatedValueText() {
forall(StringLiteral c | c = this.getString(_) |
exists(c.(StringlikeLiteralImpl).getStringValue())
) and
result =
concat(string valueText, int i |
valueText = this.getString(i).(StringlikeLiteralImpl).getStringValue()
|
valueText order by i
)
}
(requires private import internal.Literal
).
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.
The pragma still needs to be removed.
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.
Unfortunately, the test failure shows that this change isn't equivalent. It no longer gives a result for "foo" "bar#{ 1 * 1 }" 'baz'
, since StringlikeLiteralImpl.getStringValue()
doesn't give a result for the string with the interpolation.
The qldoc for getConcatenatedValueText()
is actually wrong then, for the current behaviour.
I think I would prefer backing this change out — along with the other, similar changes — and keeping it global. At least in this PR. What do you think?
@@ -40,18 +43,22 @@ class MethodBase extends Callable, BodyStmt, Scope, TMethodBase { | |||
* Holds if this method is public. | |||
* Methods are public by default. | |||
*/ | |||
overlay[global] |
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.
It feels like all of these should be local, but it is probably not important.
@@ -203,6 +206,7 @@ class HashPattern extends CasePattern, THashPattern { | |||
} | |||
|
|||
/** Gets the value for a given key name. */ | |||
overlay[global] |
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.
Can be made local:
key = this.getKey(i).(StringlikeLiteralImpl).getStringValue()
@@ -310,6 +317,7 @@ module ExprNodes { | |||
final ExprCfgNode getAnArgument() { result = this.getArgument(_) } | |||
|
|||
/** Gets the keyword argument whose key is `keyword` of this call. */ | |||
overlay[global] |
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.
Again, this can be made local.
@@ -469,19 +474,24 @@ class ParameterExt extends TParameterExt { | |||
} | |||
} | |||
|
|||
overlay[global] |
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.
What is it that makes this necessary?
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.
There must have been some intermediate state where I needed it to fix compilation errors, but I can't see any reason why it's needed now. I'll remove it.
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.
😌
3c4a528
to
21dc396
Compare
@@ -375,10 +378,12 @@ private module Cached { | |||
Impl::uncertainWriteDefinitionInput(def, result) | |||
} | |||
|
|||
overlay[global] |
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.
Is this still needed?
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 can remove it here, but the BarrierGuards
module below needs to be global
(at least to make the compiler happy).
21dc396
to
1eda55d
Compare
1eda55d
to
e1f2433
Compare
As mentioned in the comment above, I backed out some of the requested changes for getting string-like values, so they are global again. I've also added a change-note. |
Almost everything in those libraries is marked as
overlay[local]
, with the exception that everything depending on resolution of constants and call targets isoverlay[global]
.Overlay compilation is currently disabled for Ruby, so the annotations won't currently have any effect on compilation or evaluation.
I've run Ruby DCA experiments on these changes, combined with an additional commit enabling overlay compilation, showing that they give 100% alert accuracy on
nightly.qls
and a curated source suite of ~200 sources.The QL-for-QL changes are just to ensure that the compiler is happy; I haven't tested them with actual overlay analysis.