Skip to content

Commit d52d3d7

Browse files
hvitvedhvitved
authored
Merge pull request #10644 from hvitved/ruby/prevent-reevaluation
Ruby: Prevent reevaluation of expensive predicates
2 parents 9942dff + dc432c7 commit d52d3d7

File tree

16 files changed

+210
-160
lines changed

16 files changed

+210
-160
lines changed

‎javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll‎

Lines changed: 58 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -544,7 +544,7 @@ private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath) {
544544
}
545545

546546
/** Gets the node identified by the given `(package, type, path)` tuple. */
547-
API::Node getNodeFromPath(string package, string type, AccessPath path) {
547+
private API::Node getNodeFromPath(string package, string type, AccessPath path) {
548548
result = getNodeFromPath(package, type, path, path.getNumToken())
549549
}
550550

@@ -567,23 +567,25 @@ private predicate typeStep(API::Node pred, API::Node succ) {
567567
*
568568
* Unlike `getNodeFromPath`, the `path` may end with one or more call-site filters.
569569
*/
570-
Specific::InvokeNode getInvocationFromPath(string package, string type, AccessPath path, int n) {
570+
private Specific::InvokeNode getInvocationFromPath(
571+
string package, string type, AccessPath path, int n
572+
) {
571573
result = Specific::getAnInvocationOf(getNodeFromPath(package, type, path, n))
572574
or
573575
result = getInvocationFromPath(package, type, path, n - 1) and
574576
invocationMatchesCallSiteFilter(result, path.getToken(n - 1))
575577
}
576578

577579
/** Gets an invocation identified by the given `(package, type, path)` tuple. */
578-
Specific::InvokeNode getInvocationFromPath(string package, string type, AccessPath path) {
580+
private Specific::InvokeNode getInvocationFromPath(string package, string type, AccessPath path) {
579581
result = getInvocationFromPath(package, type, path, path.getNumToken())
580582
}
581583

582584
/**
583585
* Holds if `name` is a valid name for an access path token in the identifying access path.
584586
*/
585587
bindingset[name]
586-
predicate isValidTokenNameInIdentifyingAccessPath(string name) {
588+
private predicate isValidTokenNameInIdentifyingAccessPath(string name) {
587589
name = ["Argument", "Parameter", "ReturnValue", "WithArity", "TypeVar"]
588590
or
589591
Specific::isExtraValidTokenNameInIdentifyingAccessPath(name)
@@ -594,7 +596,7 @@ predicate isValidTokenNameInIdentifyingAccessPath(string name) {
594596
* in an identifying access path.
595597
*/
596598
bindingset[name]
597-
predicate isValidNoArgumentTokenInIdentifyingAccessPath(string name) {
599+
private predicate isValidNoArgumentTokenInIdentifyingAccessPath(string name) {
598600
name = "ReturnValue"
599601
or
600602
Specific::isExtraValidNoArgumentTokenInIdentifyingAccessPath(name)
@@ -605,7 +607,7 @@ predicate isValidNoArgumentTokenInIdentifyingAccessPath(string name) {
605607
* in an identifying access path.
606608
*/
607609
bindingset[name, argument]
608-
predicate isValidTokenArgumentInIdentifyingAccessPath(string name, string argument) {
610+
private predicate isValidTokenArgumentInIdentifyingAccessPath(string name, string argument) {
609611
name = ["Argument", "Parameter"] and
610612
argument.regexpMatch("(N-|-)?\\d+(\\.\\.((N-|-)?\\d+)?)?")
611613
or
@@ -622,51 +624,61 @@ predicate isValidTokenArgumentInIdentifyingAccessPath(string name, string argume
622624
* Module providing access to the imported models in terms of API graph nodes.
623625
*/
624626
module ModelOutput {
625-
/**
626-
* Holds if a CSV source model contributed `source` with the given `kind`.
627-
*/
628-
API::Node getASourceNode(string kind) {
629-
exists(string package, string type, string path |
630-
sourceModel(package, type, path, kind) and
631-
result = getNodeFromPath(package, type, path)
632-
)
633-
}
627+
cached
628+
private module Cached {
629+
/**
630+
* Holds if a CSV source model contributed `source` with the given `kind`.
631+
*/
632+
cached
633+
API::Node getASourceNode(string kind) {
634+
exists(string package, string type, string path |
635+
sourceModel(package, type, path, kind) and
636+
result = getNodeFromPath(package, type, path)
637+
)
638+
}
634639

635-
/**
636-
* Holds if a CSV sink model contributed `sink` with the given `kind`.
637-
*/
638-
API::Node getASinkNode(string kind) {
639-
exists(string package, string type, string path |
640-
sinkModel(package, type, path, kind) and
641-
result = getNodeFromPath(package, type, path)
642-
)
643-
}
640+
/**
641+
* Holds if a CSV sink model contributed `sink` with the given `kind`.
642+
*/
643+
cached
644+
API::Node getASinkNode(string kind) {
645+
exists(string package, string type, string path |
646+
sinkModel(package, type, path, kind) and
647+
result = getNodeFromPath(package, type, path)
648+
)
649+
}
644650

645-
/**
646-
* Holds if a relevant CSV summary exists for these parameters.
647-
*/
648-
predicate relevantSummaryModel(
649-
string package, string type, string path, string input, string output, string kind
650-
) {
651-
isRelevantPackage(package) and
652-
summaryModel(package, type, path, input, output, kind)
653-
}
651+
/**
652+
* Holds if a relevant CSV summary exists for these parameters.
653+
*/
654+
cached
655+
predicate relevantSummaryModel(
656+
string package, string type, string path, string input, string output, string kind
657+
) {
658+
isRelevantPackage(package) and
659+
summaryModel(package, type, path, input, output, kind)
660+
}
654661

655-
/**
656-
* Holds if a `baseNode` is an invocation identified by the `package,type,path` part of a summary row.
657-
*/
658-
predicate resolvedSummaryBase(
659-
string package, string type, string path, Specific::InvokeNode baseNode
660-
) {
661-
summaryModel(package, type, path, _, _, _) and
662-
baseNode = getInvocationFromPath(package, type, path)
662+
/**
663+
* Holds if a `baseNode` is an invocation identified by the `package,type,path` part of a summary row.
664+
*/
665+
cached
666+
predicate resolvedSummaryBase(
667+
string package, string type, string path, Specific::InvokeNode baseNode
668+
) {
669+
summaryModel(package, type, path, _, _, _) and
670+
baseNode = getInvocationFromPath(package, type, path)
671+
}
672+
673+
/**
674+
* Holds if `node` is seen as an instance of `(package,type)` due to a type definition
675+
* contributed by a CSV model.
676+
*/
677+
cached
678+
API::Node getATypeNode(string package, string type) { result = getNodeFromType(package, type) }
663679
}
664680

665-
/**
666-
* Holds if `node` is seen as an instance of `(package,type)` due to a type definition
667-
* contributed by a CSV model.
668-
*/
669-
API::Node getATypeNode(string package, string type) { result = getNodeFromType(package, type) }
681+
import Cached
670682

671683
/**
672684
* Gets an error message relating to an invalid CSV row in a model.

‎python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll‎

Lines changed: 58 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -544,7 +544,7 @@ private API::Node getNodeFromSubPath(API::Node base, AccessPath subPath) {
544544
}
545545

546546
/** Gets the node identified by the given `(package, type, path)` tuple. */
547-
API::Node getNodeFromPath(string package, string type, AccessPath path) {
547+
private API::Node getNodeFromPath(string package, string type, AccessPath path) {
548548
result = getNodeFromPath(package, type, path, path.getNumToken())
549549
}
550550

@@ -567,23 +567,25 @@ private predicate typeStep(API::Node pred, API::Node succ) {
567567
*
568568
* Unlike `getNodeFromPath`, the `path` may end with one or more call-site filters.
569569
*/
570-
Specific::InvokeNode getInvocationFromPath(string package, string type, AccessPath path, int n) {
570+
private Specific::InvokeNode getInvocationFromPath(
571+
string package, string type, AccessPath path, int n
572+
) {
571573
result = Specific::getAnInvocationOf(getNodeFromPath(package, type, path, n))
572574
or
573575
result = getInvocationFromPath(package, type, path, n - 1) and
574576
invocationMatchesCallSiteFilter(result, path.getToken(n - 1))
575577
}
576578

577579
/** Gets an invocation identified by the given `(package, type, path)` tuple. */
578-
Specific::InvokeNode getInvocationFromPath(string package, string type, AccessPath path) {
580+
private Specific::InvokeNode getInvocationFromPath(string package, string type, AccessPath path) {
579581
result = getInvocationFromPath(package, type, path, path.getNumToken())
580582
}
581583

582584
/**
583585
* Holds if `name` is a valid name for an access path token in the identifying access path.
584586
*/
585587
bindingset[name]
586-
predicate isValidTokenNameInIdentifyingAccessPath(string name) {
588+
private predicate isValidTokenNameInIdentifyingAccessPath(string name) {
587589
name = ["Argument", "Parameter", "ReturnValue", "WithArity", "TypeVar"]
588590
or
589591
Specific::isExtraValidTokenNameInIdentifyingAccessPath(name)
@@ -594,7 +596,7 @@ predicate isValidTokenNameInIdentifyingAccessPath(string name) {
594596
* in an identifying access path.
595597
*/
596598
bindingset[name]
597-
predicate isValidNoArgumentTokenInIdentifyingAccessPath(string name) {
599+
private predicate isValidNoArgumentTokenInIdentifyingAccessPath(string name) {
598600
name = "ReturnValue"
599601
or
600602
Specific::isExtraValidNoArgumentTokenInIdentifyingAccessPath(name)
@@ -605,7 +607,7 @@ predicate isValidNoArgumentTokenInIdentifyingAccessPath(string name) {
605607
* in an identifying access path.
606608
*/
607609
bindingset[name, argument]
608-
predicate isValidTokenArgumentInIdentifyingAccessPath(string name, string argument) {
610+
private predicate isValidTokenArgumentInIdentifyingAccessPath(string name, string argument) {
609611
name = ["Argument", "Parameter"] and
610612
argument.regexpMatch("(N-|-)?\\d+(\\.\\.((N-|-)?\\d+)?)?")
611613
or
@@ -622,51 +624,61 @@ predicate isValidTokenArgumentInIdentifyingAccessPath(string name, string argume
622624
* Module providing access to the imported models in terms of API graph nodes.
623625
*/
624626
module ModelOutput {
625-
/**
626-
* Holds if a CSV source model contributed `source` with the given `kind`.
627-
*/
628-
API::Node getASourceNode(string kind) {
629-
exists(string package, string type, string path |
630-
sourceModel(package, type, path, kind) and
631-
result = getNodeFromPath(package, type, path)
632-
)
633-
}
627+
cached
628+
private module Cached {
629+
/**
630+
* Holds if a CSV source model contributed `source` with the given `kind`.
631+
*/
632+
cached
633+
API::Node getASourceNode(string kind) {
634+
exists(string package, string type, string path |
635+
sourceModel(package, type, path, kind) and
636+
result = getNodeFromPath(package, type, path)
637+
)
638+
}
634639

635-
/**
636-
* Holds if a CSV sink model contributed `sink` with the given `kind`.
637-
*/
638-
API::Node getASinkNode(string kind) {
639-
exists(string package, string type, string path |
640-
sinkModel(package, type, path, kind) and
641-
result = getNodeFromPath(package, type, path)
642-
)
643-
}
640+
/**
641+
* Holds if a CSV sink model contributed `sink` with the given `kind`.
642+
*/
643+
cached
644+
API::Node getASinkNode(string kind) {
645+
exists(string package, string type, string path |
646+
sinkModel(package, type, path, kind) and
647+
result = getNodeFromPath(package, type, path)
648+
)
649+
}
644650

645-
/**
646-
* Holds if a relevant CSV summary exists for these parameters.
647-
*/
648-
predicate relevantSummaryModel(
649-
string package, string type, string path, string input, string output, string kind
650-
) {
651-
isRelevantPackage(package) and
652-
summaryModel(package, type, path, input, output, kind)
653-
}
651+
/**
652+
* Holds if a relevant CSV summary exists for these parameters.
653+
*/
654+
cached
655+
predicate relevantSummaryModel(
656+
string package, string type, string path, string input, string output, string kind
657+
) {
658+
isRelevantPackage(package) and
659+
summaryModel(package, type, path, input, output, kind)
660+
}
654661

655-
/**
656-
* Holds if a `baseNode` is an invocation identified by the `package,type,path` part of a summary row.
657-
*/
658-
predicate resolvedSummaryBase(
659-
string package, string type, string path, Specific::InvokeNode baseNode
660-
) {
661-
summaryModel(package, type, path, _, _, _) and
662-
baseNode = getInvocationFromPath(package, type, path)
662+
/**
663+
* Holds if a `baseNode` is an invocation identified by the `package,type,path` part of a summary row.
664+
*/
665+
cached
666+
predicate resolvedSummaryBase(
667+
string package, string type, string path, Specific::InvokeNode baseNode
668+
) {
669+
summaryModel(package, type, path, _, _, _) and
670+
baseNode = getInvocationFromPath(package, type, path)
671+
}
672+
673+
/**
674+
* Holds if `node` is seen as an instance of `(package,type)` due to a type definition
675+
* contributed by a CSV model.
676+
*/
677+
cached
678+
API::Node getATypeNode(string package, string type) { result = getNodeFromType(package, type) }
663679
}
664680

665-
/**
666-
* Holds if `node` is seen as an instance of `(package,type)` due to a type definition
667-
* contributed by a CSV model.
668-
*/
669-
API::Node getATypeNode(string package, string type) { result = getNodeFromType(package, type) }
681+
import Cached
670682

671683
/**
672684
* Gets an error message relating to an invalid CSV row in a model.

‎ruby/ql/lib/codeql/ruby/ApiGraphs.qll‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ private import codeql.ruby.AST
1010
private import codeql.ruby.DataFlow
1111
private import codeql.ruby.typetracking.TypeTracker
1212
private import codeql.ruby.typetracking.TypeTrackerSpecific as TypeTrackerSpecific
13-
private import codeql.ruby.ast.internal.Module
1413
private import codeql.ruby.controlflow.CfgNodes
1514
private import codeql.ruby.dataflow.internal.DataFlowPrivate as DataFlowPrivate
1615
private import codeql.ruby.dataflow.internal.DataFlowDispatch as DataFlowDispatch
@@ -482,7 +481,7 @@ module API {
482481
MkDef(DataFlow::Node nd) { isDef(nd) }
483482

484483
private string resolveTopLevel(ConstantReadAccess read) {
485-
TResolved(result) = resolveConstantReadAccess(read) and
484+
result = read.getModule().getQualifiedName() and
486485
not result.matches("%::%")
487486
}
488487

@@ -706,7 +705,7 @@ module API {
706705
exists(ClassDeclaration c, DataFlow::Node a, DataFlow::Node b |
707706
use(pred, a) and
708707
use(succ, b) and
709-
resolveConstant(b.asExpr().getExpr()) = resolveConstantWriteAccess(c) and
708+
b.asExpr().getExpr().(ConstantReadAccess).getAQualifiedName() = c.getAQualifiedName() and
710709
pragma[only_bind_into](c).getSuperclassExpr() = a.asExpr().getExpr() and
711710
lbl = Label::subclass()
712711
)

‎ruby/ql/lib/codeql/ruby/ast/Constant.qll‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,15 @@ class ConstantReadAccess extends ConstantAccess {
293293
*/
294294
Expr getValue() { result = getConstantReadAccessValue(this) }
295295

296+
/**
297+
* Gets a fully qualified name for this constant read, based on the context in
298+
* which it occurs.
299+
*/
300+
string getAQualifiedName() { result = resolveConstant(this) }
301+
302+
/** Gets the module that this read access resolves to, if any. */
303+
Module getModule() { result = resolveConstantReadAccess(this) }
304+
296305
final override string getAPrimaryQlClass() { result = "ConstantReadAccess" }
297306
}
298307

@@ -354,7 +363,7 @@ class ConstantWriteAccess extends ConstantAccess {
354363
* constants up the namespace chain, the fully qualified name of a nested
355364
* constant can be ambiguous from just statically looking at the AST.
356365
*/
357-
string getAQualifiedName() { result = resolveConstantWriteAccess(this) }
366+
string getAQualifiedName() { result = resolveConstantWrite(this) }
358367

359368
/**
360369
* Gets a qualified name for this constant. Deprecated in favor of

‎ruby/ql/lib/codeql/ruby/ast/Module.qll‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,13 @@ class Module extends TModule {
3434
exists(Namespace n | this = TUnresolved(n) and result = "...::" + n.toString())
3535
}
3636

37+
/**
38+
* Gets the qualified name of this module, if any.
39+
*
40+
* Only modules that can be resolved will have a qualified name.
41+
*/
42+
final string getQualifiedName() { this = TResolved(result) }
43+
3744
/** Gets the location of this module. */
3845
Location getLocation() {
3946
exists(Namespace n | this = TUnresolved(n) and result = n.getLocation())

‎ruby/ql/lib/codeql/ruby/ast/internal/Module.qll‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,9 @@ private module Cached {
135135
)
136136
}
137137

138+
cached
139+
string resolveConstantWrite(ConstantWriteAccess c) { result = resolveConstantWriteAccess(c) }
140+
138141
cached
139142
Method lookupMethod(Module m, string name) { TMethod(result) = lookupMethodOrConst(m, name) }
140143

@@ -472,7 +475,7 @@ private module ResolveImpl {
472475
}
473476
}
474477

475-
import ResolveImpl
478+
private import ResolveImpl
476479

477480
/**
478481
* A variant of AstNode::getEnclosingModule that excludes

0 commit comments

Comments
 (0)