Skip to content

Commit ad813ef

Browse files
erik-krogherik-krogh
committed
add flowsTo to the use of isAdditionalLoadStep
1 parent ffbd0f6 commit ad813ef

3 files changed

Lines changed: 32 additions & 27 deletions

File tree

‎javascript/ql/src/semmle/javascript/dataflow/Configuration.qll‎

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -788,12 +788,14 @@ private predicate parameterPropRead(
788788
Function f, DataFlow::Node invk, DataFlow::Node arg, string prop, DataFlow::Node read,
789789
DataFlow::Configuration cfg
790790
) {
791-
exists(DataFlow::Node parm |
791+
exists(DataFlow::SourceNode parm |
792792
callInputStep(f, invk, arg, parm, cfg) and
793793
(
794-
read = parm.(DataFlow::SourceNode).getAPropertyRead(prop)
794+
read = parm.getAPropertyRead(prop)
795795
or
796-
isAdditionalLoadStep(parm, read, prop, cfg)
796+
exists(DataFlow::Node use | parm.flowsTo(use) |
797+
isAdditionalLoadStep(use, read, prop, cfg)
798+
)
797799
)
798800
)
799801
}
@@ -881,7 +883,7 @@ private predicate reachableFromStoreBase(
881883
(
882884
flowStep(mid, cfg, nd, newSummary)
883885
or
884-
existsCopyProperty(mid, nd, prop, cfg) and
886+
isAdditionalCopyPropertyStep(mid, nd, prop, cfg) and
885887
newSummary = PathSummary::level()
886888
) and
887889
summary = oldSummary.appendValuePreserving(newSummary)
@@ -906,29 +908,6 @@ private predicate flowThroughProperty(
906908
)
907909
}
908910

909-
/**
910-
* Holds if the property `prop` is copied from `fromNode` to `toNode`.
911-
*/
912-
bindingset[prop, cfg]
913-
private predicate existsCopyProperty(DataFlow::Node fromNode, DataFlow::Node toNode, string prop, DataFlow::Configuration cfg) {
914-
fromNode = toNode
915-
or
916-
existsCopyPropertyRecursive(fromNode, toNode, prop, cfg)
917-
}
918-
919-
/**
920-
* Holds if the property `prop` is copied from `fromNode` to `toNode` using at least 1 step.
921-
*
922-
* The recursion of this predicate has been unfolded once compared to a naive implementation in order to avoid having no constraint on `prop`.
923-
* Therefore a caller of this predicate should also test whether the `toNode` and `fromNode` are equal.
924-
*/
925-
private predicate existsCopyPropertyRecursive(DataFlow::Node fromNode, DataFlow::Node toNode, string prop, DataFlow::Configuration cfg) {
926-
exists(DataFlow::Node mid |
927-
isAdditionalCopyPropertyStep(fromNode, mid, prop, cfg) and
928-
existsCopyProperty(mid, toNode, prop, cfg)
929-
)
930-
}
931-
932911
/**
933912
* Holds if `arg` and `cb` are passed as arguments to a function which in turn
934913
* invokes `cb`, passing `arg` as its `i`th argument.

‎javascript/ql/test/library-tests/Promises/flow.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,4 +84,19 @@
8484
p.catch(e => sink(e)); // NOT OK!
8585
}
8686
leaksRejectedPromise(new Promise((resolve, reject) => reject(source)));
87+
88+
function leaksRejectedAgain(p) {
89+
("foo", p).then(() => {}).catch(e => sink(e)); // NOT OK!
90+
}
91+
leaksRejectedAgain(new Promise((resolve, reject) => reject(source)).then(() => {}));
92+
93+
async function returnsRejected(p) {
94+
try {
95+
await p;
96+
} catch(e) {
97+
return e;
98+
}
99+
}
100+
var foo = returnsRejected(new Promise((resolve, reject) => reject(source)));
101+
sink(foo); // NOT OK!
87102
})();

‎javascript/ql/test/library-tests/Promises/tests.expected‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ test_PromiseDefinition_getExecutor
3636
| flow.js:65:9:65:56 | new Pro ... ource)) | flow.js:65:21:65:55 | (resolv ... source) |
3737
| flow.js:74:10:74:57 | new Pro ... ource)) | flow.js:74:22:74:56 | (resolv ... source) |
3838
| flow.js:86:23:86:70 | new Pro ... ource)) | flow.js:86:35:86:69 | (resolv ... source) |
39+
| flow.js:91:21:91:68 | new Pro ... ource)) | flow.js:91:33:91:67 | (resolv ... source) |
40+
| flow.js:100:28:100:75 | new Pro ... ource)) | flow.js:100:40:100:74 | (resolv ... source) |
3941
| interflow.js:11:12:15:6 | new Pro ... \\n }) | interflow.js:11:24:15:5 | functio ... ;\\n } |
4042
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:3:29:5:3 | functio ... e);\\n } |
4143
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:10:30:17:3 | (res, r ... e);\\n } |
@@ -58,6 +60,8 @@ test_PromiseDefinition
5860
| flow.js:65:9:65:56 | new Pro ... ource)) |
5961
| flow.js:74:10:74:57 | new Pro ... ource)) |
6062
| flow.js:86:23:86:70 | new Pro ... ource)) |
63+
| flow.js:91:21:91:68 | new Pro ... ource)) |
64+
| flow.js:100:28:100:75 | new Pro ... ource)) |
6165
| interflow.js:11:12:15:6 | new Pro ... \\n }) |
6266
| promises.js:3:17:5:4 | new Pro ... );\\n }) |
6367
| promises.js:10:18:17:4 | new Pro ... );\\n }) |
@@ -71,6 +75,7 @@ test_PromiseDefinition_getAResolveHandler
7175
| flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:56:19:56:26 | () => {} |
7276
| flow.js:60:12:60:59 | new Pro ... ource)) | flow.js:61:21:61:28 | () => {} |
7377
| flow.js:74:10:74:57 | new Pro ... ource)) | flow.js:74:64:74:71 | () => {} |
78+
| flow.js:91:21:91:68 | new Pro ... ource)) | flow.js:91:75:91:82 | () => {} |
7479
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:6:16:8:3 | functio ... al;\\n } |
7580
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:18:17:20:3 | (v) => ... v;\\n } |
7681
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:26:20:28:3 | (v) => ... v;\\n } |
@@ -90,6 +95,8 @@ test_PromiseDefinition_getRejectParameter
9095
| flow.js:65:9:65:56 | new Pro ... ource)) | flow.js:65:31:65:36 | reject |
9196
| flow.js:74:10:74:57 | new Pro ... ource)) | flow.js:74:32:74:37 | reject |
9297
| flow.js:86:23:86:70 | new Pro ... ource)) | flow.js:86:45:86:50 | reject |
98+
| flow.js:91:21:91:68 | new Pro ... ource)) | flow.js:91:43:91:48 | reject |
99+
| flow.js:100:28:100:75 | new Pro ... ource)) | flow.js:100:50:100:55 | reject |
93100
| interflow.js:11:12:15:6 | new Pro ... \\n }) | interflow.js:11:43:11:48 | reject |
94101
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:3:48:3:53 | reject |
95102
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:10:36:10:38 | rej |
@@ -109,6 +116,8 @@ test_PromiseDefinition_getResolveParameter
109116
| flow.js:65:9:65:56 | new Pro ... ource)) | flow.js:65:22:65:28 | resolve |
110117
| flow.js:74:10:74:57 | new Pro ... ource)) | flow.js:74:23:74:29 | resolve |
111118
| flow.js:86:23:86:70 | new Pro ... ource)) | flow.js:86:36:86:42 | resolve |
119+
| flow.js:91:21:91:68 | new Pro ... ource)) | flow.js:91:34:91:40 | resolve |
120+
| flow.js:100:28:100:75 | new Pro ... ource)) | flow.js:100:41:100:47 | resolve |
112121
| interflow.js:11:12:15:6 | new Pro ... \\n }) | interflow.js:11:34:11:40 | resolve |
113122
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:3:39:3:45 | resolve |
114123
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:10:31:10:33 | res |
@@ -139,4 +148,6 @@ flow
139148
| flow.js:2:15:2:22 | "source" | flow.js:76:50:76:50 | e |
140149
| flow.js:2:15:2:22 | "source" | flow.js:79:20:79:20 | x |
141150
| flow.js:2:15:2:22 | "source" | flow.js:84:21:84:21 | e |
151+
| flow.js:2:15:2:22 | "source" | flow.js:89:45:89:45 | e |
152+
| flow.js:2:15:2:22 | "source" | flow.js:101:7:101:9 | foo |
142153
| interflow.js:3:18:3:25 | "source" | interflow.js:18:10:18:14 | error |

0 commit comments

Comments
 (0)