Skip to content

Commit b54bbcd

Browse files
hvitvedhvitved
committed
Ruby: Do not distinguish between symbols and strings in hash keys
1 parent f04a55e commit b54bbcd

File tree

5 files changed

+83
-57
lines changed

5 files changed

+83
-57
lines changed

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -829,7 +829,28 @@ class ContentSet extends TContentSet {
829829
this.isAny() and
830830
exists(result)
831831
or
832-
result = this.getAnElementReadContent()
832+
exists(Content elementContent | elementContent = this.getAnElementReadContent() |
833+
result = elementContent
834+
or
835+
// Do not distinguish symbol keys from string keys. This allows us to
836+
// give more precise summaries for something like `with_indifferent_access`,
837+
// and the amount of false-positive flow arising from this should be very
838+
// limited.
839+
elementContent =
840+
any(Content::KnownElementContent known, ConstantValue cv |
841+
cv = known.getIndex() and
842+
result.(Content::KnownElementContent).getIndex() =
843+
any(ConstantValue cv2 |
844+
cv2.(ConstantValue::ConstantSymbolValue).getStringlikeValue() =
845+
cv.(ConstantValue::ConstantStringValue).getStringlikeValue()
846+
or
847+
cv2.(ConstantValue::ConstantStringValue).getStringlikeValue() =
848+
cv.(ConstantValue::ConstantSymbolValue).getStringlikeValue()
849+
)
850+
|
851+
known
852+
)
853+
)
833854
}
834855
}
835856

‎ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -121,16 +121,6 @@ module ActiveSupport {
121121
* Extensions to the `Hash` class.
122122
*/
123123
module Hash {
124-
private class WithIndifferentAccessSummary extends SimpleSummarizedCallable {
125-
WithIndifferentAccessSummary() { this = "with_indifferent_access" }
126-
127-
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
128-
input = "Argument[self].Element[any]" and
129-
output = "ReturnValue.Element[any]" and
130-
preservesValue = true
131-
}
132-
}
133-
134124
/**
135125
* Flow summary for `reverse_merge`, and its alias `with_defaults`.
136126
*/
@@ -167,8 +157,9 @@ module ActiveSupport {
167157
}
168158

169159
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
170-
input = "Argument[self].Element[any]" and
171-
output = "ReturnValue.Element[?]" and
160+
// keys are considered equal modulo string/symbol in our implementation
161+
input = "Argument[self].WithElement[any]" and
162+
output = "ReturnValue" and
172163
preservesValue = true
173164
}
174165
}

‎ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,16 @@ edges
4040
| hash_flow.rb:42:17:42:26 | call to taint | hash_flow.rb:42:5:42:8 | [post] hash [element a] | provenance | |
4141
| hash_flow.rb:43:5:43:8 | [post] hash [element 0] | hash_flow.rb:44:10:44:13 | hash [element 0] | provenance | |
4242
| hash_flow.rb:43:5:43:8 | [post] hash [element :a] | hash_flow.rb:46:10:46:13 | hash [element :a] | provenance | |
43+
| hash_flow.rb:43:5:43:8 | [post] hash [element :a] | hash_flow.rb:48:10:48:13 | hash [element :a] | provenance | |
44+
| hash_flow.rb:43:5:43:8 | [post] hash [element a] | hash_flow.rb:46:10:46:13 | hash [element a] | provenance | |
4345
| hash_flow.rb:43:5:43:8 | [post] hash [element a] | hash_flow.rb:48:10:48:13 | hash [element a] | provenance | |
4446
| hash_flow.rb:43:5:43:8 | hash [element 0] | hash_flow.rb:43:5:43:8 | [post] hash [element 0] | provenance | |
4547
| hash_flow.rb:43:5:43:8 | hash [element :a] | hash_flow.rb:43:5:43:8 | [post] hash [element :a] | provenance | |
4648
| hash_flow.rb:43:5:43:8 | hash [element a] | hash_flow.rb:43:5:43:8 | [post] hash [element a] | provenance | |
4749
| hash_flow.rb:44:10:44:13 | hash [element 0] | hash_flow.rb:44:10:44:16 | ...[...] | provenance | |
4850
| hash_flow.rb:46:10:46:13 | hash [element :a] | hash_flow.rb:46:10:46:17 | ...[...] | provenance | |
51+
| hash_flow.rb:46:10:46:13 | hash [element a] | hash_flow.rb:46:10:46:17 | ...[...] | provenance | |
52+
| hash_flow.rb:48:10:48:13 | hash [element :a] | hash_flow.rb:48:10:48:18 | ...[...] | provenance | |
4953
| hash_flow.rb:48:10:48:13 | hash [element a] | hash_flow.rb:48:10:48:18 | ...[...] | provenance | |
5054
| hash_flow.rb:55:5:55:9 | hash1 [element :a] | hash_flow.rb:56:10:56:14 | hash1 [element :a] | provenance | |
5155
| hash_flow.rb:55:13:55:37 | ...[...] [element :a] | hash_flow.rb:55:5:55:9 | hash1 [element :a] | provenance | |
@@ -583,7 +587,9 @@ edges
583587
| hash_flow.rb:626:11:626:11 | a [element] | hash_flow.rb:626:11:626:16 | ...[...] | provenance | |
584588
| hash_flow.rb:626:11:626:16 | ...[...] | hash_flow.rb:626:10:626:17 | ( ... ) | provenance | |
585589
| hash_flow.rb:632:5:632:8 | hash [element :a] | hash_flow.rb:639:5:639:8 | hash [element :a] | provenance | |
590+
| hash_flow.rb:632:5:632:8 | hash [element :a] | hash_flow.rb:640:11:640:14 | hash [element :a] | provenance | |
586591
| hash_flow.rb:632:5:632:8 | hash [element :c] | hash_flow.rb:639:5:639:8 | hash [element :c] | provenance | |
592+
| hash_flow.rb:632:5:632:8 | hash [element :c] | hash_flow.rb:642:11:642:14 | hash [element :c] | provenance | |
587593
| hash_flow.rb:632:12:636:5 | call to [] [element :a] | hash_flow.rb:632:5:632:8 | hash [element :a] | provenance | |
588594
| hash_flow.rb:632:12:636:5 | call to [] [element :c] | hash_flow.rb:632:5:632:8 | hash [element :c] | provenance | |
589595
| hash_flow.rb:633:15:633:25 | call to taint | hash_flow.rb:632:12:636:5 | call to [] [element :a] | provenance | |
@@ -599,10 +605,12 @@ edges
599605
| hash_flow.rb:639:5:639:8 | hash [element :a] | hash_flow.rb:639:5:639:8 | [post] hash [element] | provenance | |
600606
| hash_flow.rb:639:5:639:8 | hash [element :c] | hash_flow.rb:639:5:639:8 | [post] hash [element] | provenance | |
601607
| hash_flow.rb:639:5:639:8 | hash [element] | hash_flow.rb:639:5:639:8 | [post] hash [element] | provenance | |
608+
| hash_flow.rb:640:11:640:14 | hash [element :a] | hash_flow.rb:640:11:640:19 | ...[...] | provenance | |
602609
| hash_flow.rb:640:11:640:14 | hash [element] | hash_flow.rb:640:11:640:19 | ...[...] | provenance | |
603610
| hash_flow.rb:640:11:640:19 | ...[...] | hash_flow.rb:640:10:640:20 | ( ... ) | provenance | |
604611
| hash_flow.rb:641:11:641:14 | hash [element] | hash_flow.rb:641:11:641:19 | ...[...] | provenance | |
605612
| hash_flow.rb:641:11:641:19 | ...[...] | hash_flow.rb:641:10:641:20 | ( ... ) | provenance | |
613+
| hash_flow.rb:642:11:642:14 | hash [element :c] | hash_flow.rb:642:11:642:19 | ...[...] | provenance | |
606614
| hash_flow.rb:642:11:642:14 | hash [element] | hash_flow.rb:642:11:642:19 | ...[...] | provenance | |
607615
| hash_flow.rb:642:11:642:19 | ...[...] | hash_flow.rb:642:10:642:20 | ( ... ) | provenance | |
608616
| hash_flow.rb:648:5:648:8 | hash [element :a] | hash_flow.rb:653:9:653:12 | hash [element :a] | provenance | |
@@ -1149,7 +1157,9 @@ nodes
11491157
| hash_flow.rb:44:10:44:13 | hash [element 0] | semmle.label | hash [element 0] |
11501158
| hash_flow.rb:44:10:44:16 | ...[...] | semmle.label | ...[...] |
11511159
| hash_flow.rb:46:10:46:13 | hash [element :a] | semmle.label | hash [element :a] |
1160+
| hash_flow.rb:46:10:46:13 | hash [element a] | semmle.label | hash [element a] |
11521161
| hash_flow.rb:46:10:46:17 | ...[...] | semmle.label | ...[...] |
1162+
| hash_flow.rb:48:10:48:13 | hash [element :a] | semmle.label | hash [element :a] |
11531163
| hash_flow.rb:48:10:48:13 | hash [element a] | semmle.label | hash [element a] |
11541164
| hash_flow.rb:48:10:48:18 | ...[...] | semmle.label | ...[...] |
11551165
| hash_flow.rb:55:5:55:9 | hash1 [element :a] | semmle.label | hash1 [element :a] |
@@ -1740,12 +1750,14 @@ nodes
17401750
| hash_flow.rb:639:5:639:8 | hash [element :c] | semmle.label | hash [element :c] |
17411751
| hash_flow.rb:639:5:639:8 | hash [element] | semmle.label | hash [element] |
17421752
| hash_flow.rb:640:10:640:20 | ( ... ) | semmle.label | ( ... ) |
1753+
| hash_flow.rb:640:11:640:14 | hash [element :a] | semmle.label | hash [element :a] |
17431754
| hash_flow.rb:640:11:640:14 | hash [element] | semmle.label | hash [element] |
17441755
| hash_flow.rb:640:11:640:19 | ...[...] | semmle.label | ...[...] |
17451756
| hash_flow.rb:641:10:641:20 | ( ... ) | semmle.label | ( ... ) |
17461757
| hash_flow.rb:641:11:641:14 | hash [element] | semmle.label | hash [element] |
17471758
| hash_flow.rb:641:11:641:19 | ...[...] | semmle.label | ...[...] |
17481759
| hash_flow.rb:642:10:642:20 | ( ... ) | semmle.label | ( ... ) |
1760+
| hash_flow.rb:642:11:642:14 | hash [element :c] | semmle.label | hash [element :c] |
17491761
| hash_flow.rb:642:11:642:14 | hash [element] | semmle.label | hash [element] |
17501762
| hash_flow.rb:642:11:642:19 | ...[...] | semmle.label | ...[...] |
17511763
| hash_flow.rb:648:5:648:8 | hash [element :a] | semmle.label | hash [element :a] |
@@ -2349,6 +2361,8 @@ hashLiteral
23492361
| hash_flow.rb:30:10:30:16 | ...[...] | hash_flow.rb:19:14:19:23 | call to taint | hash_flow.rb:30:10:30:16 | ...[...] | $@ | hash_flow.rb:19:14:19:23 | call to taint | call to taint |
23502362
| hash_flow.rb:44:10:44:16 | ...[...] | hash_flow.rb:38:15:38:24 | call to taint | hash_flow.rb:44:10:44:16 | ...[...] | $@ | hash_flow.rb:38:15:38:24 | call to taint | call to taint |
23512363
| hash_flow.rb:46:10:46:17 | ...[...] | hash_flow.rb:40:16:40:25 | call to taint | hash_flow.rb:46:10:46:17 | ...[...] | $@ | hash_flow.rb:40:16:40:25 | call to taint | call to taint |
2364+
| hash_flow.rb:46:10:46:17 | ...[...] | hash_flow.rb:42:17:42:26 | call to taint | hash_flow.rb:46:10:46:17 | ...[...] | $@ | hash_flow.rb:42:17:42:26 | call to taint | call to taint |
2365+
| hash_flow.rb:48:10:48:18 | ...[...] | hash_flow.rb:40:16:40:25 | call to taint | hash_flow.rb:48:10:48:18 | ...[...] | $@ | hash_flow.rb:40:16:40:25 | call to taint | call to taint |
23522366
| hash_flow.rb:48:10:48:18 | ...[...] | hash_flow.rb:42:17:42:26 | call to taint | hash_flow.rb:48:10:48:18 | ...[...] | $@ | hash_flow.rb:42:17:42:26 | call to taint | call to taint |
23532367
| hash_flow.rb:56:10:56:18 | ...[...] | hash_flow.rb:55:21:55:30 | call to taint | hash_flow.rb:56:10:56:18 | ...[...] | $@ | hash_flow.rb:55:21:55:30 | call to taint | call to taint |
23542368
| hash_flow.rb:61:10:61:18 | ...[...] | hash_flow.rb:59:13:59:22 | call to taint | hash_flow.rb:61:10:61:18 | ...[...] | $@ | hash_flow.rb:59:13:59:22 | call to taint | call to taint |

‎ruby/ql/test/library-tests/dataflow/hash-flow/hash_flow.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,9 @@ def m2()
4343
hash['b'] = 3
4444
sink(hash[0]) # $ hasValueFlow=2.1
4545
sink(hash[1])
46-
sink(hash[:a]) # $ hasValueFlow=2.2
46+
sink(hash[:a]) # $ hasValueFlow=2.2 $ SPURIOUS hasValueFlow=2.3
4747
sink(hash[:b])
48-
sink(hash['a']) # $ hasValueFlow=2.3
48+
sink(hash['a']) # $ hasValueFlow=2.3 $ SPURIOUS hasValueFlow=2.2
4949
sink(hash['b'])
5050
end
5151

0 commit comments

Comments
 (0)