Skip to content

Commit 5b11708

Browse files
hmachmac
authored
Merge pull request #11534 from hmac/array-inclusion-barrier-guard-constant
Ruby: Make array inclusion barrier more sensitive
2 parents a743fbc + 4d228bc commit 5b11708

File tree

3 files changed

+94
-24
lines changed

3 files changed

+94
-24
lines changed

‎ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,19 +13,18 @@ cached
1313
private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch) {
1414
exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c |
1515
c = guard and
16-
exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode |
17-
// Only consider strings without any interpolations
18-
not strLitNode.getExpr().getComponent(_) instanceof StringInterpolationComponent and
16+
exists(ExprCfgNode strNode |
17+
strNode.getConstantValue().isStringlikeValue(_) and
1918
c.getExpr() instanceof EqExpr and
2019
branch = true
2120
or
2221
c.getExpr() instanceof CaseEqExpr and branch = true
2322
or
2423
c.getExpr() instanceof NEExpr and branch = false
2524
|
26-
c.getLeftOperand() = strLitNode and c.getRightOperand() = testedNode
25+
c.getLeftOperand() = strNode and c.getRightOperand() = testedNode
2726
or
28-
c.getLeftOperand() = testedNode and c.getRightOperand() = strLitNode
27+
c.getLeftOperand() = testedNode and c.getRightOperand() = strNode
2928
)
3029
)
3130
or
@@ -116,7 +115,7 @@ private predicate stringConstArrayInclusionCall(
116115
isArrayConstant(t.getContainerNode().asExpr(), arr)
117116
|
118117
forall(ExprCfgNode elem | elem = arr.getAnArgument() |
119-
elem instanceof ExprNodes::StringLiteralCfgNode
118+
elem.getConstantValue().isStringlikeValue(_)
120119
)
121120
)
122121
)
@@ -193,39 +192,45 @@ private predicate stringConstCaseCompare(
193192
guard =
194193
any(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
195194
branchNode = case.getBranch(_) and
196-
// For simplicity, consider patterns that contain only string literals or arrays of string literals
195+
// For simplicity, consider patterns that contain only string literals, string-valued variables, or arrays of the same.
197196
forall(ExprCfgNode pattern | pattern = branchNode.getPattern(_) |
197+
// foo = "foo"
198+
//
199+
// when foo
200+
// when foo, bar
198201
// when "foo"
199202
// when "foo", "bar"
200-
pattern instanceof ExprNodes::StringLiteralCfgNode
203+
pattern.getConstantValue().isStringlikeValue(_)
201204
or
202205
pattern =
203206
any(CfgNodes::ExprNodes::SplatExprCfgNode splat |
204207
// when *["foo", "bar"]
205208
forex(ExprCfgNode elem |
206209
elem = splat.getOperand().(ExprNodes::ArrayLiteralCfgNode).getAnArgument()
207210
|
208-
elem instanceof ExprNodes::StringLiteralCfgNode
211+
elem.getConstantValue().isStringlikeValue(_)
209212
)
210213
or
211214
// when *some_var
212215
// when *SOME_CONST
213216
exists(ExprNodes::ArrayLiteralCfgNode arr |
214217
isArrayConstant(splat.getOperand(), arr) and
215218
forall(ExprCfgNode elem | elem = arr.getAnArgument() |
216-
elem instanceof ExprNodes::StringLiteralCfgNode
219+
elem.getConstantValue().isStringlikeValue(_)
217220
)
218221
)
219222
)
220223
)
221224
)
222225
or
226+
// foo = "foo"
227+
//
228+
// in foo
223229
// in "foo"
224-
exists(
225-
CfgNodes::ExprNodes::InClauseCfgNode branchNode, ExprNodes::StringLiteralCfgNode pattern
226-
|
230+
exists(CfgNodes::ExprNodes::InClauseCfgNode branchNode, ExprCfgNode pattern |
227231
branchNode = case.getBranch(_) and
228232
pattern = branchNode.getPattern() and
233+
pattern.getConstantValue().isStringlikeValue(_) and
229234
guard = pattern
230235
)
231236
)

‎ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected‎

Lines changed: 47 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,21 @@ WARNING: Type BarrierGuard has been deprecated and may be removed in future (bar
22
failures
33
oldStyleBarrierGuards
44
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | barrier-guards.rb:3:4:3:6 | foo | true |
5+
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | barrier-guards.rb:3:11:3:15 | "foo" | true |
56
| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | barrier-guards.rb:9:21:9:23 | foo | true |
67
| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:18:5:18:7 | foo | barrier-guards.rb:15:4:15:6 | foo | false |
8+
| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:18:5:18:7 | foo | barrier-guards.rb:15:11:15:15 | "foo" | false |
79
| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:24:5:24:7 | foo | barrier-guards.rb:21:8:21:10 | foo | true |
10+
| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:24:5:24:7 | foo | barrier-guards.rb:21:15:21:19 | "foo" | true |
811
| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:28:5:28:7 | foo | barrier-guards.rb:27:8:27:10 | foo | false |
12+
| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:28:5:28:7 | foo | barrier-guards.rb:27:15:27:19 | "foo" | false |
913
| barrier-guards.rb:37:4:37:20 | call to include? | barrier-guards.rb:38:5:38:7 | foo | barrier-guards.rb:37:17:37:19 | foo | true |
1014
| barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:45:9:45:11 | foo | barrier-guards.rb:43:4:43:6 | foo | true |
15+
| barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:45:9:45:11 | foo | barrier-guards.rb:43:11:43:15 | "foo" | true |
1116
| barrier-guards.rb:70:4:70:21 | call to include? | barrier-guards.rb:71:5:71:7 | foo | barrier-guards.rb:70:18:70:20 | foo | true |
17+
| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | barrier-guards.rb:82:4:82:18 | call to index | false |
1218
| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | barrier-guards.rb:82:15:82:17 | foo | true |
19+
| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | barrier-guards.rb:82:23:82:25 | nil | false |
1320
| barrier-guards.rb:207:4:207:15 | ... == ... | barrier-guards.rb:208:5:208:7 | foo | barrier-guards.rb:207:4:207:6 | foo | true |
1421
| barrier-guards.rb:211:10:211:21 | ... == ... | barrier-guards.rb:212:5:212:7 | foo | barrier-guards.rb:211:10:211:12 | foo | true |
1522
| barrier-guards.rb:215:16:215:27 | ... == ... | barrier-guards.rb:216:5:216:7 | foo | barrier-guards.rb:215:16:215:18 | foo | true |
@@ -18,7 +25,11 @@ oldStyleBarrierGuards
1825
| barrier-guards.rb:219:21:219:32 | ... == ... | barrier-guards.rb:220:5:220:7 | foo | barrier-guards.rb:219:21:219:23 | foo | true |
1926
| barrier-guards.rb:232:6:232:17 | ... == ... | barrier-guards.rb:233:5:233:7 | foo | barrier-guards.rb:232:6:232:8 | foo | true |
2027
| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:237:24:237:26 | foo | barrier-guards.rb:237:6:237:8 | foo | true |
21-
| barrier-guards.rb:268:1:268:12 | ... == ... | barrier-guards.rb:268:17:268:19 | foo | barrier-guards.rb:268:1:268:3 | foo | true |
28+
| barrier-guards.rb:259:4:259:16 | ... == ... | barrier-guards.rb:260:5:260:7 | foo | barrier-guards.rb:259:4:259:6 | foo | true |
29+
| barrier-guards.rb:264:4:264:16 | ... == ... | barrier-guards.rb:265:5:265:7 | foo | barrier-guards.rb:264:4:264:6 | foo | true |
30+
| barrier-guards.rb:272:1:272:12 | ... == ... | barrier-guards.rb:272:17:272:19 | foo | barrier-guards.rb:272:1:272:3 | foo | true |
31+
| barrier-guards.rb:275:4:275:19 | call to include? | barrier-guards.rb:276:5:276:7 | foo | barrier-guards.rb:275:17:275:19 | foo | true |
32+
| barrier-guards.rb:281:4:281:20 | call to include? | barrier-guards.rb:282:5:282:7 | foo | barrier-guards.rb:281:18:281:20 | foo | true |
2233
newStyleBarrierGuards
2334
| barrier-guards.rb:4:5:4:7 | foo |
2435
| barrier-guards.rb:10:5:10:7 | foo |
@@ -49,7 +60,12 @@ newStyleBarrierGuards
4960
| barrier-guards.rb:233:5:233:7 | foo |
5061
| barrier-guards.rb:237:24:237:26 | foo |
5162
| barrier-guards.rb:244:5:244:7 | foo |
52-
| barrier-guards.rb:268:17:268:19 | foo |
63+
| barrier-guards.rb:260:5:260:7 | foo |
64+
| barrier-guards.rb:265:5:265:7 | foo |
65+
| barrier-guards.rb:272:17:272:19 | foo |
66+
| barrier-guards.rb:276:5:276:7 | foo |
67+
| barrier-guards.rb:282:5:282:7 | foo |
68+
| barrier-guards.rb:292:5:292:7 | foo |
5369
controls
5470
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true |
5571
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false |
@@ -313,12 +329,35 @@ controls
313329
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:260:5:260:7 | foo | match |
314330
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:264:1:266:3 | if ... | match |
315331
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:265:5:265:7 | foo | match |
316-
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:268:1:268:19 | ... && ... | match |
317-
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:268:17:268:19 | foo | match |
318-
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:269:1:269:19 | ... && ... | match |
319-
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:269:8:269:10 | foo | match |
332+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:268:1:270:3 | if ... | match |
333+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:269:5:269:7 | foo | match |
334+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:272:1:272:19 | ... && ... | match |
335+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:272:17:272:19 | foo | match |
336+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:273:1:273:19 | ... && ... | match |
337+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:273:8:273:10 | foo | match |
338+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:275:1:277:3 | if ... | match |
339+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:276:5:276:7 | foo | match |
340+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:281:1:283:3 | if ... | match |
341+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:282:5:282:7 | foo | match |
342+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:286:1:288:3 | if ... | match |
343+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:287:5:287:7 | foo | match |
344+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:290:1:295:3 | case ... | match |
345+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:291:1:292:19 | [match] when ... | match |
346+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:291:1:292:19 | [no-match] when ... | match |
347+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:292:5:292:7 | foo | match |
348+
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:294:5:294:7 | foo | match |
320349
| barrier-guards.rb:254:4:254:28 | ... == ... | barrier-guards.rb:255:5:255:7 | foo | true |
321350
| barrier-guards.rb:259:4:259:16 | ... == ... | barrier-guards.rb:260:5:260:7 | foo | true |
322351
| barrier-guards.rb:264:4:264:16 | ... == ... | barrier-guards.rb:265:5:265:7 | foo | true |
323-
| barrier-guards.rb:268:1:268:12 | ... == ... | barrier-guards.rb:268:17:268:19 | foo | true |
324-
| barrier-guards.rb:269:1:269:3 | foo | barrier-guards.rb:269:8:269:10 | foo | true |
352+
| barrier-guards.rb:268:4:268:30 | ... == ... | barrier-guards.rb:269:5:269:7 | foo | true |
353+
| barrier-guards.rb:272:1:272:12 | ... == ... | barrier-guards.rb:272:17:272:19 | foo | true |
354+
| barrier-guards.rb:273:1:273:3 | foo | barrier-guards.rb:273:8:273:10 | foo | true |
355+
| barrier-guards.rb:275:4:275:19 | call to include? | barrier-guards.rb:276:5:276:7 | foo | true |
356+
| barrier-guards.rb:281:4:281:20 | call to include? | barrier-guards.rb:282:5:282:7 | foo | true |
357+
| barrier-guards.rb:286:4:286:20 | call to include? | barrier-guards.rb:287:5:287:7 | foo | true |
358+
| barrier-guards.rb:291:1:292:19 | [match] when ... | barrier-guards.rb:292:5:292:7 | foo | match |
359+
| barrier-guards.rb:291:1:292:19 | [no-match] when ... | barrier-guards.rb:294:5:294:7 | foo | no-match |
360+
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:291:1:292:19 | [match] when ... | match |
361+
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:291:1:292:19 | [no-match] when ... | no-match |
362+
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:292:5:292:7 | foo | match |
363+
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:294:5:294:7 | foo | no-match |

‎ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -257,13 +257,39 @@
257257

258258
F = "foo"
259259
if foo == "#{F}"
260-
foo # $ MISSING: guarded
260+
foo # $ guarded
261261
end
262262

263263
f = "foo"
264264
if foo == "#{f}"
265-
foo # $ MISSING: guarded
265+
foo # $ guarded
266+
end
267+
268+
if foo == "#{f}#{unknown_var}"
269+
foo
266270
end
267271

268272
foo == "foo" && foo # $ guarded
269-
foo && foo == "foo"
273+
foo && foo == "foo"
274+
275+
if [f].include? foo
276+
foo # $ guarded
277+
end
278+
279+
g = "g"
280+
foos = [f, g]
281+
if foos.include? foo
282+
foo # $ guarded
283+
end
284+
285+
foos = [f, g, some_method_call]
286+
if foos.include? foo
287+
foo
288+
end
289+
290+
case foo
291+
when g
292+
foo # $ guarded
293+
else
294+
foo
295+
end

0 commit comments

Comments
 (0)