Skip to content

Commit 67962cb

Browse files
committed
Ruby: Fix bad join in access predicate
Joining on variable name alone is a bad thing: ``` [2022-01-25 11:13:20] (228s) Tuple counts for Variable::Cached::access#ff#shared/3@868b54tu after 3m37s: 112554 ~0% {3} r1 = JOIN Variable::VariableReal::getNameImpl_dispred#ff WITH Variable::VariableReal::getDeclaringScopeImpl_dispred#ff ON FIRST 1 OUTPUT Lhs.1, Lhs.0 'arg2', Rhs.1 'arg1' 561015756 ~1% {3} r2 = JOIN r1 WITH Variable::variableName#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1 'arg0', Lhs.2 'arg1', Lhs.1 'arg2' return r2 ``` This change ensures that we join on name and scope simultaneously.
1 parent 78b4d7c commit 67962cb

File tree

1 file changed

+22
-14
lines changed

1 file changed

+22
-14
lines changed

ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -106,20 +106,23 @@ private predicate scopeDefinesParameterVariable(
106106
)
107107
}
108108

109-
private string variableName(Ruby::AstNode i) {
110-
result = i.(Ruby::Identifier).getValue()
111-
or
112-
exists(Ruby::KeywordPattern p | i = p.getKey() and not exists(p.getValue()) |
113-
result = i.(Ruby::String).getChild(0).(Ruby::StringContent).getValue() or
114-
result = i.(Ruby::HashKeySymbol).getValue()
109+
pragma[nomagic]
110+
private string variableNameInScope(Ruby::AstNode i, Scope::Range scope) {
111+
scope = scopeOf(i) and
112+
(
113+
result = i.(Ruby::Identifier).getValue()
114+
or
115+
exists(Ruby::KeywordPattern p | i = p.getKey() and not exists(p.getValue()) |
116+
result = i.(Ruby::String).getChild(0).(Ruby::StringContent).getValue() or
117+
result = i.(Ruby::HashKeySymbol).getValue()
118+
)
115119
)
116120
}
117121

118122
/** Holds if `name` is assigned in `scope` at `i`. */
119123
private predicate scopeAssigns(Scope::Range scope, string name, Ruby::AstNode i) {
120124
(explicitAssignmentNode(i, _) or implicitAssignmentNode(i)) and
121-
name = variableName(i) and
122-
scope = scopeOf(i)
125+
name = variableNameInScope(i, scope)
123126
}
124127

125128
cached
@@ -306,13 +309,18 @@ private module Cached {
306309
i = any(Ruby::WhileModifier x).getBody()
307310
}
308311

312+
pragma[nomagic]
313+
private predicate hasScopeAndName(VariableReal variable, Scope::Range scope, string name) {
314+
variable.getNameImpl() = name and
315+
scope = variable.getDeclaringScopeImpl()
316+
}
317+
309318
cached
310319
predicate access(Ruby::AstNode access, VariableReal variable) {
311-
exists(string name |
312-
variable.getNameImpl() = name and
313-
name = variableName(access)
320+
exists(string name, Scope::Range scope |
321+
pragma[only_bind_into](name) = variableNameInScope(access, scope)
314322
|
315-
variable.getDeclaringScopeImpl() = scopeOf(access) and
323+
hasScopeAndName(variable, scope, name) and
316324
not access.getLocation().strictlyBefore(variable.getLocationImpl()) and
317325
// In case of overlapping parameter names, later parameters should not
318326
// be considered accesses to the first parameter
@@ -321,8 +329,8 @@ private module Cached {
321329
else any()
322330
or
323331
exists(Scope::Range declScope |
324-
variable.getDeclaringScopeImpl() = declScope and
325-
inherits(scopeOf(access), name, declScope)
332+
hasScopeAndName(variable, declScope, pragma[only_bind_into](name)) and
333+
inherits(scope, name, declScope)
326334
)
327335
)
328336
}

0 commit comments

Comments
 (0)