Skip to content

Commit 06e898f

Browse files
erik-krogherik-krogh
committed
only use .getALocalSource in copyPropertyStep
1 parent 9998059 commit 06e898f

4 files changed

Lines changed: 52 additions & 25 deletions

File tree

‎javascript/ql/src/semmle/javascript/Promises.qll‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ private module ExceptionalPromiseFlow {
142142
this = promise
143143
}
144144

145-
override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
145+
override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
146146
prop = rejectField() and
147147
(
148148
pred = promise.getRejectParameter().getACall().getArgument(0) or
@@ -185,14 +185,14 @@ private module ExceptionalPromiseFlow {
185185
succ = getCallback(1).getParameter(0)
186186
}
187187

188-
override predicate copyProperty(DataFlow::Node pred, DataFlow::Node succ, string prop) {
188+
override predicate copyProperty(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
189189
not exists(this.getArgument(1)) and
190190
prop = rejectField() and
191191
pred = getReceiver() and
192192
succ = this
193193
}
194194

195-
override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
195+
override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
196196
prop = rejectField() and
197197
pred = getCallback([0..1]).getExceptionalReturn() and
198198
succ = this
@@ -213,7 +213,7 @@ private module ExceptionalPromiseFlow {
213213
succ = getCallback(0).getParameter(0)
214214
}
215215

216-
override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
216+
override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
217217
prop = rejectField() and
218218
pred = getCallback([0..1]).getExceptionalReturn() and
219219
succ = this
@@ -228,7 +228,7 @@ private module ExceptionalPromiseFlow {
228228
this.getMethodName() = "finally"
229229
}
230230

231-
override predicate copyProperty(DataFlow::Node pred, DataFlow::Node succ, string prop) {
231+
override predicate copyProperty(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
232232
prop = rejectField() and
233233
pred = getReceiver() and
234234
succ = this

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

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -225,9 +225,11 @@ abstract class Configuration extends string {
225225
}
226226

227227
/**
228-
* Holds if the `pred` should be stored in the object `succ` under the property `prop`.
228+
* Holds if the `pred` should be stored in the object `succ` under the property `prop`.
229+
*
230+
* `succ` is a DataFlow::SourceNode, as this is assumed by the `isAdditionalCopyPropertyStep` predicate.
229231
*/
230-
predicate isAdditionalStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
232+
predicate isAdditionalStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
231233

232234
/**
233235
* Holds if the property `prop` of the object `pred` should be loaded into `succ`.
@@ -237,7 +239,7 @@ abstract class Configuration extends string {
237239
/**
238240
* Holds if the property `prop` should be copied from the object `pred` to the object `succ`.
239241
*/
240-
predicate isAdditionalCopyPropertyStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
242+
predicate isAdditionalCopyPropertyStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
241243
}
242244

243245
/**
@@ -468,9 +470,11 @@ abstract class AdditionalFlowStep extends DataFlow::Node {
468470

469471
/**
470472
* Holds if the `pred` should be stored in the object `succ` under the property `prop`.
473+
*
474+
* `succ` is a DataFlow::SourceNode, as this is assumed by the `copyProperty` predicate.
471475
*/
472476
cached
473-
predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
477+
predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
474478

475479
/**
476480
* Holds if the property `prop` of the object `pred` should be loaded into `succ`.
@@ -482,7 +486,7 @@ abstract class AdditionalFlowStep extends DataFlow::Node {
482486
* Holds if the property `prop` should be copied from the object `pred` to the object `succ`.
483487
*/
484488
cached
485-
predicate copyProperty(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
489+
predicate copyProperty(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
486490
}
487491

488492
/**
@@ -808,26 +812,25 @@ private predicate reachesReturn(
808812
}
809813

810814
private predicate isAdditionalLoadStep(DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) {
811-
exists(DataFlow::Node obj | pred = obj.getALocalSource() or pred = obj |
812-
any(AdditionalFlowStep s).load(obj, succ, prop)
813-
or
814-
cfg.isAdditionalLoadStep(obj, succ, prop)
815-
)
815+
any(AdditionalFlowStep s).load(pred, succ, prop)
816+
or
817+
cfg.isAdditionalLoadStep(pred, succ, prop)
816818
}
817819

818-
private predicate isAdditionalStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) {
819-
exists(DataFlow::Node obj | pred = obj.getALocalSource() or pred = obj |
820-
any(AdditionalFlowStep s).store(obj, succ, prop)
821-
or
822-
cfg.isAdditionalStoreStep(obj, succ, prop)
823-
)
820+
private predicate isAdditionalStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop, DataFlow::Configuration cfg) {
821+
any(AdditionalFlowStep s).store(pred, succ, prop)
822+
or
823+
cfg.isAdditionalStoreStep(pred, succ, prop)
824824
}
825825

826-
private predicate isAdditionalCopyPropertyStep(DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) {
827-
exists(DataFlow::Node obj | pred = obj.getALocalSource() or pred = obj |
828-
any(AdditionalFlowStep s).copyProperty(obj, succ, prop)
826+
private predicate isAdditionalCopyPropertyStep(DataFlow::SourceNode pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) {
827+
exists(DataFlow::Node predNode, DataFlow::SourceNode succNode |
828+
pred = predNode.getALocalSource() and
829+
succ.getALocalSource() = succNode
830+
|
831+
any(AdditionalFlowStep s).copyProperty(predNode, succNode, prop)
829832
or
830-
cfg.isAdditionalCopyPropertyStep(obj, succ, prop)
833+
cfg.isAdditionalCopyPropertyStep(predNode, succNode, prop)
831834
)
832835
}
833836

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,4 +56,17 @@
5656
var p9 = p8.then(() => {});
5757
var p10 = p9.finally(() => {});
5858
p10.catch((x) => sink(x)); // NOT OK!
59+
60+
var p11 = new Promise((resolve, reject) => reject(source));
61+
var p12 = p11.then(() => {});
62+
p12.catch(x => sink(x)); // NOT OK!
63+
64+
async function throws() {
65+
await new Promise((resolve, reject) => reject(source));
66+
}
67+
try {
68+
throws();
69+
} catch(e) {
70+
sink(e); // NOT OK!
71+
}
5972
})();

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ test_PromiseDefinition_getExecutor
3131
| flow.js:42:2:42:49 | new Pro ... ource)) | flow.js:42:14:42:48 | (resolv ... source) |
3232
| flow.js:48:2:48:36 | new Pro ... urce }) | flow.js:48:14:48:35 | () => { ... ource } |
3333
| flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:55:23:55:57 | (resolv ... source) |
34+
| flow.js:60:12:60:59 | new Pro ... ource)) | flow.js:60:24:60:58 | (resolv ... source) |
35+
| flow.js:65:9:65:56 | new Pro ... ource)) | flow.js:65:21:65:55 | (resolv ... source) |
3436
| interflow.js:11:12:15:6 | new Pro ... \\n }) | interflow.js:11:24:15:5 | functio ... ;\\n } |
3537
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:3:29:5:3 | functio ... e);\\n } |
3638
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:10:30:17:3 | (res, r ... e);\\n } |
@@ -49,6 +51,8 @@ test_PromiseDefinition
4951
| flow.js:42:2:42:49 | new Pro ... ource)) |
5052
| flow.js:48:2:48:36 | new Pro ... urce }) |
5153
| flow.js:55:11:55:58 | new Pro ... ource)) |
54+
| flow.js:60:12:60:59 | new Pro ... ource)) |
55+
| flow.js:65:9:65:56 | new Pro ... ource)) |
5256
| interflow.js:11:12:15:6 | new Pro ... \\n }) |
5357
| promises.js:3:17:5:4 | new Pro ... );\\n }) |
5458
| promises.js:10:18:17:4 | new Pro ... );\\n }) |
@@ -60,6 +64,7 @@ test_PromiseDefinition_getAResolveHandler
6064
| flow.js:40:2:40:49 | new Pro ... ource)) | flow.js:40:56:40:64 | () => { } |
6165
| flow.js:42:2:42:49 | new Pro ... ource)) | flow.js:42:56:42:64 | () => { } |
6266
| flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:56:19:56:26 | () => {} |
67+
| flow.js:60:12:60:59 | new Pro ... ource)) | flow.js:61:21:61:28 | () => {} |
6368
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:6:16:8:3 | functio ... al;\\n } |
6469
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:18:17:20:3 | (v) => ... v;\\n } |
6570
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:26:20:28:3 | (v) => ... v;\\n } |
@@ -75,6 +80,8 @@ test_PromiseDefinition_getRejectParameter
7580
| flow.js:40:2:40:49 | new Pro ... ource)) | flow.js:40:24:40:29 | reject |
7681
| flow.js:42:2:42:49 | new Pro ... ource)) | flow.js:42:24:42:29 | reject |
7782
| flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:55:33:55:38 | reject |
83+
| flow.js:60:12:60:59 | new Pro ... ource)) | flow.js:60:34:60:39 | reject |
84+
| flow.js:65:9:65:56 | new Pro ... ource)) | flow.js:65:31:65:36 | reject |
7885
| interflow.js:11:12:15:6 | new Pro ... \\n }) | interflow.js:11:43:11:48 | reject |
7986
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:3:48:3:53 | reject |
8087
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:10:36:10:38 | rej |
@@ -90,6 +97,8 @@ test_PromiseDefinition_getResolveParameter
9097
| flow.js:40:2:40:49 | new Pro ... ource)) | flow.js:40:15:40:21 | resolve |
9198
| flow.js:42:2:42:49 | new Pro ... ource)) | flow.js:42:15:42:21 | resolve |
9299
| flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:55:24:55:30 | resolve |
100+
| flow.js:60:12:60:59 | new Pro ... ource)) | flow.js:60:25:60:31 | resolve |
101+
| flow.js:65:9:65:56 | new Pro ... ource)) | flow.js:65:22:65:28 | resolve |
93102
| interflow.js:11:12:15:6 | new Pro ... \\n }) | interflow.js:11:34:11:40 | resolve |
94103
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:3:39:3:45 | resolve |
95104
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:10:31:10:33 | res |
@@ -115,4 +124,6 @@ flow
115124
| flow.js:2:15:2:22 | "source" | flow.js:48:54:48:54 | x |
116125
| flow.js:2:15:2:22 | "source" | flow.js:53:39:53:39 | v |
117126
| flow.js:2:15:2:22 | "source" | flow.js:58:24:58:24 | x |
127+
| flow.js:2:15:2:22 | "source" | flow.js:62:22:62:22 | x |
128+
| flow.js:2:15:2:22 | "source" | flow.js:70:8:70:8 | e |
118129
| interflow.js:3:18:3:25 | "source" | interflow.js:18:10:18:14 | error |

0 commit comments

Comments
 (0)