Skip to content

Commit 9a9bbe3

Browse files
aschackmullaschackmull
committed
Dataflow: Support side-effects for callbacks in summaries.
1 parent fc8b439 commit 9a9bbe3

File tree

4 files changed

+144
-32
lines changed

4 files changed

+144
-32
lines changed

‎java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll

Lines changed: 76 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -186,10 +186,15 @@ module Private {
186186
TArgumentSummaryComponent(int i) { parameterPosition(i) } or
187187
TReturnSummaryComponent(ReturnKind rk)
188188

189+
private TSummaryComponent thisParam() { result = TParameterSummaryComponent(instanceParameterPosition()) }
190+
189191
newtype TSummaryComponentStack =
190192
TSingletonSummaryComponentStack(SummaryComponent c) or
191193
TConsSummaryComponentStack(SummaryComponent head, SummaryComponentStack tail) {
192194
tail.(RequiredSummaryComponentStack).required(head)
195+
or
196+
tail.(RequiredSummaryComponentStack).required(TParameterSummaryComponent(_)) and
197+
head = thisParam()
193198
}
194199

195200
pragma[nomagic]
@@ -198,21 +203,63 @@ module Private {
198203
boolean preservesValue
199204
) {
200205
c.propagatesFlow(input, output, preservesValue)
206+
or
207+
// observe side effects of callbacks on input arguments
208+
c.propagatesFlow(output, input, preservesValue) and
209+
preservesValue = true and
210+
isCallbackParameter(input) and
211+
isContentOfArgument(output)
212+
or
213+
// flow from the receiver of a callback into the instance-parameter
214+
exists(SummaryComponentStack s, SummaryComponentStack callbackRef |
215+
c.propagatesFlow(s, _, _) or c.propagatesFlow(_, s, _)
216+
|
217+
callbackRef = s.drop(_) and
218+
(isCallbackParameter(callbackRef) or callbackRef.head() = TReturnSummaryComponent(_)) and
219+
input = callbackRef.tail() and
220+
output = TConsSummaryComponentStack(thisParam(), input) and
221+
preservesValue = true
222+
)
223+
}
224+
225+
private predicate isCallbackParameter(SummaryComponentStack s) {
226+
s.head() = TParameterSummaryComponent(_) and exists(s.tail())
227+
}
228+
229+
private predicate isContentOfArgument(SummaryComponentStack s) {
230+
s.head() = TContentSummaryComponent(_) and isContentOfArgument(s.tail())
231+
or
232+
s = TSingletonSummaryComponentStack(TArgumentSummaryComponent(_))
233+
}
234+
235+
private predicate outputState(SummarizedCallable c, SummaryComponentStack s) {
236+
summary(c, _, s, _)
237+
or
238+
exists(SummaryComponentStack out |
239+
outputState(c, out) and
240+
out.head() = TContentSummaryComponent(_) and
241+
s = out.tail()
242+
)
243+
or
244+
// Add the argument node corresponding to the requested post-update node
245+
inputState(c, s) and isCallbackParameter(s)
246+
}
247+
248+
private predicate inputState(SummarizedCallable c, SummaryComponentStack s) {
249+
summary(c, s, _, _)
250+
or
251+
exists(SummaryComponentStack inp | inputState(c, inp) and s = inp.tail())
252+
or
253+
exists(SummaryComponentStack out |
254+
outputState(c, out) and
255+
out.head() = TParameterSummaryComponent(_) and
256+
s = out.tail()
257+
)
201258
}
202259

203260
private newtype TSummaryNodeState =
204-
TSummaryNodeInputState(SummaryComponentStack s) {
205-
exists(SummaryComponentStack input |
206-
summary(_, input, _, _) and
207-
s = input.drop(_)
208-
)
209-
} or
210-
TSummaryNodeOutputState(SummaryComponentStack s) {
211-
exists(SummaryComponentStack output |
212-
summary(_, _, output, _) and
213-
s = output.drop(_)
214-
)
215-
}
261+
TSummaryNodeInputState(SummaryComponentStack s) { inputState(_, s) } or
262+
TSummaryNodeOutputState(SummaryComponentStack s) { outputState(_, s) }
216263

217264
/**
218265
* A state used to break up (complex) flow summaries into atomic flow steps.
@@ -238,20 +285,14 @@ module Private {
238285
pragma[nomagic]
239286
predicate isInputState(SummarizedCallable c, SummaryComponentStack s) {
240287
this = TSummaryNodeInputState(s) and
241-
exists(SummaryComponentStack input |
242-
summary(c, input, _, _) and
243-
s = input.drop(_)
244-
)
288+
inputState(c, s)
245289
}
246290

247291
/** Holds if this state is a valid output state for `c`. */
248292
pragma[nomagic]
249293
predicate isOutputState(SummarizedCallable c, SummaryComponentStack s) {
250294
this = TSummaryNodeOutputState(s) and
251-
exists(SummaryComponentStack output |
252-
summary(c, _, output, _) and
253-
s = output.drop(_)
254-
)
295+
outputState(c, s)
255296
}
256297

257298
/** Gets a textual representation of this state. */
@@ -331,19 +372,12 @@ module Private {
331372
receiver = summaryNodeInputState(c, s.drop(1))
332373
}
333374

334-
private Node pre(Node post) {
335-
summaryPostUpdateNode(post, result)
336-
or
337-
not summaryPostUpdateNode(post, _) and
338-
result = post
339-
}
340-
341375
private predicate callbackInput(
342376
SummarizedCallable c, SummaryComponentStack s, Node receiver, int i
343377
) {
344378
any(SummaryNodeState state).isOutputState(c, s) and
345379
s.head() = TParameterSummaryComponent(i) and
346-
receiver = pre(summaryNodeOutputState(c, s.drop(1)))
380+
receiver = summaryNodeInputState(c, s.drop(1))
347381
}
348382

349383
/** Holds if a call targeting `receiver` should be synthesized inside `c`. */
@@ -395,7 +429,7 @@ module Private {
395429
or
396430
exists(int i | head = TParameterSummaryComponent(i) |
397431
result =
398-
getCallbackParameterType(getNodeType(summaryNodeOutputState(pragma[only_bind_out](c),
432+
getCallbackParameterType(getNodeType(summaryNodeInputState(pragma[only_bind_out](c),
399433
s.drop(1))), i)
400434
)
401435
)
@@ -421,10 +455,16 @@ module Private {
421455
}
422456

423457
/** Holds if summary node `post` is a post-update node with pre-update node `pre`. */
424-
predicate summaryPostUpdateNode(Node post, ParamNode pre) {
458+
predicate summaryPostUpdateNode(Node post, Node pre) {
425459
exists(SummarizedCallable c, int i |
426460
isParameterPostUpdate(post, c, i) and
427-
pre.isParameterOf(c, i)
461+
pre.(ParamNode).isParameterOf(c, i)
462+
)
463+
or
464+
exists(SummarizedCallable callable, SummaryComponentStack s |
465+
callbackInput(callable, s, _, _) and
466+
pre = summaryNodeOutputState(callable, s) and
467+
post = summaryNodeInputState(callable, s)
428468
)
429469
}
430470

@@ -462,7 +502,11 @@ module Private {
462502
// for `StringBuilder.append(x)` with a specified value flow from qualifier to
463503
// return value and taint flow from argument 0 to the qualifier, then this
464504
// allows us to infer taint flow from argument 0 to the return value.
465-
summaryPostUpdateNode(pred, succ) and preservesValue = true
505+
succ instanceof ParamNode and summaryPostUpdateNode(pred, succ) and preservesValue = true
506+
or
507+
// Similarly we would like to chain together summaries where values get passed
508+
// into callbacks along the way.
509+
pred instanceof ArgNode and summaryPostUpdateNode(succ, pred) and preservesValue = true
466510
}
467511

468512
/**

‎java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImplSpecific.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ private module FlowSummaries {
1616
/** Holds is `i` is a valid parameter position. */
1717
predicate parameterPosition(int i) { i in [-1 .. any(Parameter p).getPosition()] }
1818

19+
/** Gets the parameter position of the instance parameter. */
20+
int instanceParameterPosition() { result = -1 }
21+
1922
/** Gets the synthesized summary data-flow node for the given values. */
2023
Node summaryNode(SummarizedCallable c, SummaryNodeState state) { result = getSummaryNode(c, state) }
2124

@@ -37,6 +40,8 @@ DataFlowType getReturnType(SummarizedCallable c, ReturnKind rk) {
3740
*/
3841
DataFlowType getCallbackParameterType(DataFlowType t, int i) {
3942
result = getErasedRepr(t.(FunctionalInterface).getRunMethod().getParameterType(i))
43+
or
44+
result = getErasedRepr(t.(FunctionalInterface)) and i = -1
4045
}
4146

4247
/**

‎java/ql/test/library-tests/dataflow/callback-dispatch/A.java

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package my.callback.qltest;
22

3+
import java.util.*;
4+
35
public class A {
46
public interface Consumer1 {
57
void eat(Object o);
@@ -28,6 +30,20 @@ static <T> void applyConsumer3(T x, Consumer3<T> con) {
2830
// con.eat(x);
2931
}
3032

33+
static <T> T applyConsumer3_ret_postup(Consumer3<T> con) {
34+
// summary:
35+
// x = new T();
36+
// con.eat(x);
37+
// return x;
38+
return null;
39+
}
40+
41+
static <T> void forEach(T[] xs, Consumer3<T> con) {
42+
// summary:
43+
// x = xs[..];
44+
// con.eat(x);
45+
}
46+
3147
public interface Producer1<T> {
3248
T make();
3349
}
@@ -38,6 +54,14 @@ static <T> T applyProducer1(Producer1<T> prod) {
3854
return null;
3955
}
4056

57+
static <T> T produceConsume(Producer1<T> prod, Consumer3<T> con) {
58+
// summary:
59+
// x = prod.make();
60+
// con.eat(x);
61+
// return x;
62+
return null;
63+
}
64+
4165
public interface Converter1<T1,T2> {
4266
T2 conv(T1 x);
4367
}
@@ -109,5 +133,40 @@ void foo2() {
109133
};
110134
applyConsumer3(new Integer[] { (Integer)source(12) }, pc);
111135
sink(applyProducer1(pc)[0]); // $ flow=11
136+
137+
Integer res = applyProducer1(new Producer1<Integer>() {
138+
private Integer ii = (Integer)source(13);
139+
@Override public Integer make() {
140+
return this.ii;
141+
}
142+
});
143+
sink(res); // $ flow=13
144+
145+
ArrayList<Object> list = new ArrayList<>();
146+
applyConsumer3(list, l -> l.add(source(14)));
147+
sink(list.get(0)); // $ flow=14
148+
149+
Consumer3<ArrayList<Object>> tainter = l -> l.add(source(15));
150+
sink(applyConsumer3_ret_postup(tainter).get(0)); // $ flow=15
151+
152+
forEach(new Object[] {source(16)}, x -> sink(x)); // $ flow=16
153+
154+
// Spurious flow from 17 is reasonable as it would likely
155+
// also occur if the lambda body was inlined in a for loop.
156+
// It occurs from the combination of being able to observe
157+
// the side-effect of the callback on the other argument and
158+
// being able to chain summaries that update/read arguments,
159+
// e.g. fluent apis.
160+
// Spurious flow from 18 is due to not matching call targets
161+
// in a return-from-call-to-enter-call flow sequence.
162+
forEach(new Object[2][], xs -> { sink(xs[0]); xs[0] = source(17); }); // $ SPURIOUS: flow=17 flow=18
163+
164+
Object[][] xss = new Object[][] { { null } };
165+
forEach(xss, x -> {x[0] = source(18);});
166+
sink(xss[0][0]); // $ flow=18
167+
168+
Object res2 = produceConsume(() -> source(19), A::sink); // $ flow=19
169+
sink(res2); // $ flow=19
112170
}
171+
113172
}

‎java/ql/test/library-tests/dataflow/callback-dispatch/test.ql

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,11 @@ class SummaryModelTest extends SummaryModelCsv {
1010
"my.callback.qltest;A;false;applyConsumer1;(Object,Consumer1);;Argument[0];Parameter[0] of Argument[1];value",
1111
"my.callback.qltest;A;false;applyConsumer2;(Object,Consumer2);;Argument[0];Parameter[0] of Argument[1];value",
1212
"my.callback.qltest;A;false;applyConsumer3;(Object,Consumer3);;Argument[0];Parameter[0] of Argument[1];value",
13+
"my.callback.qltest;A;false;applyConsumer3_ret_postup;(Consumer3);;Parameter[0] of Argument[0];ReturnValue;value",
14+
"my.callback.qltest;A;false;forEach;(Object[],Consumer3);;ArrayElement of Argument[0];Parameter[0] of Argument[1];value",
1315
"my.callback.qltest;A;false;applyProducer1;(Producer1);;ReturnValue of Argument[0];ReturnValue;value",
16+
"my.callback.qltest;A;false;produceConsume;(Producer1,Consumer3);;ReturnValue of Argument[0];Parameter[0] of Argument[1];value",
17+
"my.callback.qltest;A;false;produceConsume;(Producer1,Consumer3);;Parameter[0] of Argument[1];ReturnValue;value",
1418
"my.callback.qltest;A;false;applyConverter1;(Object,Converter1);;Argument[0];Parameter[0] of Argument[1];value",
1519
"my.callback.qltest;A;false;applyConverter1;(Object,Converter1);;ReturnValue of Argument[1];ReturnValue;value"
1620
]

0 commit comments

Comments
 (0)