Skip to content

Commit a76ab39

Browse files
erik-krogherik-krogh
committed
no longer need for .getALocalSource() in custom load/store
1 parent e08fc08 commit a76ab39

4 files changed

Lines changed: 52 additions & 32 deletions

File tree

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ private module ExceptionalPromiseFlow {
167167
override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
168168
prop = rejectField() and
169169
succ = await.getExceptionTarget() and
170-
pred = operand.getALocalSource()
170+
pred = operand
171171
}
172172
}
173173

@@ -181,14 +181,14 @@ private module ExceptionalPromiseFlow {
181181

182182
override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
183183
prop = rejectField() and
184-
pred = getReceiver().getALocalSource() and
184+
pred = getReceiver() and
185185
succ = getCallback(1).getParameter(0)
186186
}
187187

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

@@ -209,7 +209,7 @@ private module ExceptionalPromiseFlow {
209209

210210
override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
211211
prop = rejectField() and
212-
pred = getReceiver().getALocalSource() and
212+
pred = getReceiver() and
213213
succ = getCallback(0).getParameter(0)
214214
}
215215

@@ -230,7 +230,7 @@ private module ExceptionalPromiseFlow {
230230

231231
override predicate copyProperty(DataFlow::Node pred, DataFlow::Node succ, string prop) {
232232
prop = rejectField() and
233-
pred = getReceiver().getALocalSource() and
233+
pred = getReceiver() and
234234
succ = this
235235
}
236236
}

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

Lines changed: 36 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -585,12 +585,9 @@ private predicate exploratoryFlowStep(
585585
basicStoreStep(pred, succ, _) or
586586
basicLoadStep(pred, succ, _) or
587587

588-
any(AdditionalFlowStep s).store(pred, succ, _) or
589-
cfg.isAdditionalStoreStep(pred, succ, _) or
590-
any(AdditionalFlowStep s).load(pred, succ, _) or
591-
cfg.isAdditionalLoadStep(pred, succ, _) or
592-
any(AdditionalFlowStep s).copyProperty(pred, succ, _) or
593-
cfg.isAdditionalCopyPropertyStep(pred, succ, _) or
588+
isAdditionalStoreStep(pred, succ, _, cfg) or
589+
isAdditionalLoadStep(pred, succ, _, cfg) or
590+
isAdditionalCopyPropertyStep(pred, succ, _ ,cfg) or
594591
// the following two disjuncts taken together over-approximate flow through
595592
// higher-order calls
596593
callback(pred, succ) or
@@ -752,10 +749,7 @@ private predicate storeStep(
752749
basicStoreStep(pred, succ, prop) and
753750
summary = PathSummary::level()
754751
or
755-
any(AdditionalFlowStep s).store(pred, succ, prop) and
756-
summary = PathSummary::level()
757-
or
758-
cfg.isAdditionalStoreStep(pred, succ, prop) and
752+
isAdditionalStoreStep(pred, succ, prop, cfg) and
759753
summary = PathSummary::level()
760754
or
761755
exists(Function f, DataFlow::Node mid |
@@ -766,11 +760,7 @@ private predicate storeStep(
766760
returnedPropWrite(f, _, prop, mid)
767761
or
768762
exists(DataFlow::SourceNode base |
769-
(
770-
any(AdditionalFlowStep step).store(mid, _, prop)
771-
or
772-
cfg.isAdditionalStoreStep(mid, _, prop)
773-
)
763+
isAdditionalStoreStep(mid, _, prop, cfg)
774764
and
775765
base.flowsToExpr(f.getAReturnedExpr())
776766
)
@@ -793,9 +783,7 @@ private predicate parameterPropRead(
793783
(
794784
read = parm.(DataFlow::SourceNode).getAPropertyRead(prop)
795785
or
796-
any(AdditionalFlowStep step).load(parm, read, prop)
797-
or
798-
cfg.isAdditionalLoadStep(parm, read, prop)
786+
isAdditionalLoadStep(parm, read, prop, cfg)
799787
)
800788
)
801789
}
@@ -818,6 +806,30 @@ private predicate reachesReturn(
818806
)
819807
}
820808

809+
private predicate isAdditionalLoadStep(DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) {
810+
exists(DataFlow::Node obj | pred = obj.getALocalSource() or pred = obj |
811+
any(AdditionalFlowStep s).load(obj, succ, prop)
812+
or
813+
cfg.isAdditionalLoadStep(obj, succ, prop)
814+
)
815+
}
816+
817+
private predicate isAdditionalStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) {
818+
exists(DataFlow::Node obj | pred = obj.getALocalSource() or pred = obj |
819+
any(AdditionalFlowStep s).store(obj, succ, prop)
820+
or
821+
cfg.isAdditionalStoreStep(obj, succ, prop)
822+
)
823+
}
824+
825+
private predicate isAdditionalCopyPropertyStep(DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) {
826+
exists(DataFlow::Node obj | pred = obj.getALocalSource() or pred = obj |
827+
any(AdditionalFlowStep s).copyProperty(obj, succ, prop)
828+
or
829+
cfg.isAdditionalCopyPropertyStep(obj, succ, prop)
830+
)
831+
}
832+
821833
/**
822834
* Holds if property `prop` of `pred` may flow into `succ` along a path summarized by
823835
* `summary`.
@@ -829,10 +841,7 @@ private predicate loadStep(
829841
basicLoadStep(pred, succ, prop) and
830842
summary = PathSummary::level()
831843
or
832-
any(AdditionalFlowStep s).load(pred, succ, prop) and
833-
summary = PathSummary::level()
834-
or
835-
cfg.isAdditionalLoadStep(pred, succ, prop) and
844+
isAdditionalLoadStep(pred, succ, prop, cfg) and
836845
summary = PathSummary::level()
837846
or
838847
exists(Function f, DataFlow::Node read |
@@ -874,7 +883,7 @@ private predicate flowThroughProperty(
874883
) {
875884
exists(string prop, DataFlow::Node storeBase, DataFlow::Node loadBase, PathSummary oldSummary, PathSummary newSummary |
876885
reachableFromStoreBase(prop, pred, storeBase, cfg, oldSummary) and
877-
(storeBase = loadBase or existsCopyProperty(storeBase, loadBase, prop)) and
886+
(storeBase = loadBase or existsCopyProperty(storeBase, loadBase, prop, cfg)) and
878887
loadStep(loadBase, succ, prop, cfg, newSummary) and
879888
summary = oldSummary.append(newSummary)
880889
)
@@ -886,11 +895,11 @@ private predicate flowThroughProperty(
886895
* The recursion of this predicate has been unfolded once compared to a naive implementation in order to avoid having no constraint on `prop`.
887896
* Therefore a caller of this predicate should also test whether the `toNode` and `fromNode` are equal.
888897
*/
889-
private predicate existsCopyProperty(DataFlow::Node fromNode, DataFlow::Node toNode, string prop) {
890-
exists(DataFlow::AdditionalFlowStep step, DataFlow::Node mid |
891-
step.copyProperty(fromNode, mid, prop) and
898+
private predicate existsCopyProperty(DataFlow::Node fromNode, DataFlow::Node toNode, string prop, DataFlow::Configuration cfg) {
899+
exists(DataFlow::Node mid |
900+
isAdditionalCopyPropertyStep(fromNode, mid, prop, cfg) and
892901
(
893-
existsCopyProperty(mid, toNode, prop)
902+
existsCopyProperty(mid, toNode, prop, cfg)
894903
or
895904
mid = toNode
896905
)

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,4 +51,9 @@
5151
return Promise.resolve(src);
5252
}
5353
createPromise(source).then(v => sink(v)); // NOT OK!
54+
55+
var p8 = new Promise((resolve, reject) => reject(source));
56+
var p9 = p8.then(() => {});
57+
var p10 = p9.finally(() => {});
58+
p10.catch((x) => sink(x)); // NOT OK!
5459
})();

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ test_PromiseDefinition_getExecutor
3030
| flow.js:40:2:40:49 | new Pro ... ource)) | flow.js:40:14:40:48 | (resolv ... source) |
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 } |
33+
| flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:55:23:55:57 | (resolv ... source) |
3334
| interflow.js:11:12:15:6 | new Pro ... \\n }) | interflow.js:11:24:15:5 | functio ... ;\\n } |
3435
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:3:29:5:3 | functio ... e);\\n } |
3536
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:10:30:17:3 | (res, r ... e);\\n } |
@@ -47,6 +48,7 @@ test_PromiseDefinition
4748
| flow.js:40:2:40:49 | new Pro ... ource)) |
4849
| flow.js:42:2:42:49 | new Pro ... ource)) |
4950
| flow.js:48:2:48:36 | new Pro ... urce }) |
51+
| flow.js:55:11:55:58 | new Pro ... ource)) |
5052
| interflow.js:11:12:15:6 | new Pro ... \\n }) |
5153
| promises.js:3:17:5:4 | new Pro ... );\\n }) |
5254
| promises.js:10:18:17:4 | new Pro ... );\\n }) |
@@ -57,6 +59,7 @@ test_PromiseDefinition_getAResolveHandler
5759
| flow.js:26:2:26:49 | new Pro ... ource)) | flow.js:26:56:26:66 | x => foo(x) |
5860
| flow.js:40:2:40:49 | new Pro ... ource)) | flow.js:40:56:40:64 | () => { } |
5961
| flow.js:42:2:42:49 | new Pro ... ource)) | flow.js:42:56:42:64 | () => { } |
62+
| flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:56:19:56:26 | () => {} |
6063
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:6:16:8:3 | functio ... al;\\n } |
6164
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:18:17:20:3 | (v) => ... v;\\n } |
6265
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:26:20:28:3 | (v) => ... v;\\n } |
@@ -71,6 +74,7 @@ test_PromiseDefinition_getRejectParameter
7174
| flow.js:32:2:32:49 | new Pro ... ource)) | flow.js:32:24:32:29 | reject |
7275
| flow.js:40:2:40:49 | new Pro ... ource)) | flow.js:40:24:40:29 | reject |
7376
| flow.js:42:2:42:49 | new Pro ... ource)) | flow.js:42:24:42:29 | reject |
77+
| flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:55:33:55:38 | reject |
7478
| interflow.js:11:12:15:6 | new Pro ... \\n }) | interflow.js:11:43:11:48 | reject |
7579
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:3:48:3:53 | reject |
7680
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:10:36:10:38 | rej |
@@ -85,6 +89,7 @@ test_PromiseDefinition_getResolveParameter
8589
| flow.js:32:2:32:49 | new Pro ... ource)) | flow.js:32:15:32:21 | resolve |
8690
| flow.js:40:2:40:49 | new Pro ... ource)) | flow.js:40:15:40:21 | resolve |
8791
| flow.js:42:2:42:49 | new Pro ... ource)) | flow.js:42:15:42:21 | resolve |
92+
| flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:55:24:55:30 | resolve |
8893
| interflow.js:11:12:15:6 | new Pro ... \\n }) | interflow.js:11:34:11:40 | resolve |
8994
| promises.js:3:17:5:4 | new Pro ... );\\n }) | promises.js:3:39:3:45 | resolve |
9095
| promises.js:10:18:17:4 | new Pro ... );\\n }) | promises.js:10:31:10:33 | res |
@@ -109,4 +114,5 @@ flow
109114
| flow.js:2:15:2:22 | "source" | flow.js:46:60:46:60 | a |
110115
| flow.js:2:15:2:22 | "source" | flow.js:48:54:48:54 | x |
111116
| flow.js:2:15:2:22 | "source" | flow.js:53:39:53:39 | v |
117+
| flow.js:2:15:2:22 | "source" | flow.js:58:24:58:24 | x |
112118
| interflow.js:3:18:3:25 | "source" | interflow.js:18:10:18:14 | error |

0 commit comments

Comments
 (0)