Skip to content

Commit 86c63db

Browse files
committed
Dataflow: Fix bug causing spurious flow for FeatureHasSinkCallContext.
1 parent f598a0b commit 86c63db

File tree

2 files changed

+39
-19
lines changed

2 files changed

+39
-19
lines changed
Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,2 @@
11
testFailures
2-
| A.java:78:10:78:12 | src | Unexpected result: SinkCc="call" |
32
failures

shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll

Lines changed: 39 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1560,7 +1560,13 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
15601560
argT = TypOption::some(t) and
15611561
argAp = apSome(ap)
15621562
) else (
1563-
summaryCtx = TParamNodeNone() and argT instanceof TypOption::None and argAp = apNone()
1563+
summaryCtx = TParamNodeNone() and
1564+
argT instanceof TypOption::None and
1565+
argAp = apNone() and
1566+
// When the call contexts of source and sink needs to match then there's
1567+
// never any reason to enter a callable except to find a summary. See also
1568+
// the comment in `PathNodeMid::isAtSink`.
1569+
not Config::getAFeature() instanceof FeatureEqualSourceSinkCallContext
15641570
)
15651571
)
15661572
or
@@ -2598,8 +2604,23 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
25982604

25992605
override predicate isSink() {
26002606
sinkNode(node, state) and
2601-
(if hasSinkCallCtx() then instanceofCcNoCall(cc) else any()) and
2602-
ap instanceof ApNil
2607+
ap instanceof ApNil and
2608+
// For `FeatureHasSinkCallContext` the condition `cc instanceof CallContextNoCall`
2609+
// is exactly what we need to check.
2610+
// For `FeatureEqualSourceSinkCallContext` the initial call
2611+
// context was set to `CallContextSomeCall` and jumps are
2612+
// disallowed, so `cc instanceof CallContextNoCall` never holds.
2613+
// On the other hand, in this case there's never any need to
2614+
// enter a call except to identify a summary, so the condition in
2615+
// conjunction with setting the summary context enforces this,
2616+
// which means that the summary context being empty holds if and
2617+
// only if we are in the call context of the source.
2618+
if Config::getAFeature() instanceof FeatureEqualSourceSinkCallContext
2619+
then summaryCtx = TParamNodeNone()
2620+
else
2621+
if Config::getAFeature() instanceof FeatureHasSinkCallContext
2622+
then instanceofCcNoCall(cc)
2623+
else any()
26032624
}
26042625
}
26052626

@@ -4315,21 +4336,21 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
43154336
sinkNode(node, state) and
43164337
sinkModel(node, model) and
43174338
ap instanceof AccessPathNil and
4318-
if hasSinkCallCtx()
4319-
then
4320-
// For `FeatureHasSinkCallContext` the condition `cc instanceof CallContextNoCall`
4321-
// is exactly what we need to check. This also implies
4322-
// `sc instanceof SummaryCtxNone`.
4323-
// For `FeatureEqualSourceSinkCallContext` the initial call context was
4324-
// set to `CallContextSomeCall` and jumps are disallowed, so
4325-
// `cc instanceof CallContextNoCall` never holds. On the other hand,
4326-
// in this case there's never any need to enter a call except to identify
4327-
// a summary, so the condition in `pathIntoCallable` enforces this, which
4328-
// means that `sc instanceof SummaryCtxNone` holds if and only if we are
4329-
// in the call context of the source.
4330-
sc instanceof SummaryCtxNone or
4331-
cc instanceof CallContextNoCall
4332-
else any()
4339+
// For `FeatureHasSinkCallContext` the condition `cc instanceof CallContextNoCall`
4340+
// is exactly what we need to check.
4341+
// For `FeatureEqualSourceSinkCallContext` the initial call context was
4342+
// set to `CallContextSomeCall` and jumps are disallowed, so
4343+
// `cc instanceof CallContextNoCall` never holds. On the other hand,
4344+
// in this case there's never any need to enter a call except to identify
4345+
// a summary, so the condition in `pathIntoCallable` enforces this, which
4346+
// means that `sc instanceof SummaryCtxNone` holds if and only if we are
4347+
// in the call context of the source.
4348+
if Config::getAFeature() instanceof FeatureEqualSourceSinkCallContext
4349+
then sc instanceof SummaryCtxNone
4350+
else
4351+
if Config::getAFeature() instanceof FeatureHasSinkCallContext
4352+
then cc instanceof CallContextNoCall
4353+
else any()
43334354
}
43344355

43354356
PathNodeSink projectToSink(string model) {

0 commit comments

Comments
 (0)