Skip to content

Java: Add data flow for record getters. #4123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aibaars merged 2 commits into from
Sep 1, 2020
Merged

Java: Add data flow for record getters. #4123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d82fee1
Java: Add data flow for record getters.
aschackmull Aug 24, 2020
8269287
Java: Add some test cases.
aschackmull Sep 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions java/ql/src/semmle/code/java/dataflow/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,18 @@ predicate readStep(Node node1, Content f, Node node2) {
fr.getField() = f.(FieldContent).getField() and
fr = node2.asExpr()
)
or
exists(Record r, Method getter, Field recf, MethodAccess get |
getter.getDeclaringType() = r and
recf.getDeclaringType() = r and
getter.getNumberOfParameters() = 0 and
getter.getName() = recf.getName() and
not exists(getter.getBody()) and
Copy link
Contributor

@aibaars aibaars Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose abstract records are not allowed, so checking for the absence of a body is fine, however, wouldn't it be better to use getter.fromSource() instead

Copy link
Contributor Author

@aschackmull aschackmull Aug 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Records are implicitly final and therefore can't be abstract. It looks like checking fromSource() would work as well, but I didn't expect it based on the qldoc of fromSource() - it states:

   * Holds if this element pertains to a source file.
   *
   * Elements pertaining to source files may include generated elements
   * not visible in source code, such as implicit default constructors.

So I would actually expect fromSource() to hold for a generated getter when the corresponding record is fromSource(). I wonder why that isn't the case.

Copy link
Contributor

@aibaars aibaars Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds like it's best to keep using the empty body check.

recf = f.(FieldContent).getField() and
get.getMethod() = getter and
node1.asExpr() = get.getQualifier() and
node2.asExpr() = get
)
}

/**
Expand Down
65 changes: 65 additions & 0 deletions java/ql/test/library-tests/dataflow/records/A.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
public class A {
record Pair(Object x, Object y) { }
Copy link
Contributor

@aibaars aibaars Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add some test with fields that have explicit "get" methods? I think one field with a sensible get method that just returns the field (has flow) and one silly get method that returns a constant (no flow)

Copy link
Contributor Author

@aschackmull aschackmull Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


static Object source() { return null; }

void sink(Object o) { }

void foo() {
Pair p1 = new Pair(source(), null);
Pair p2 = new Pair(new Object(), source());
bar(p1, p2);
}

void bar(Pair p1, Pair p2) {
sink(p1.x);
sink(p1.y);
sink(p2.x);
sink(p2.y);
Object p1x = p1.x();
Object p1y = p1.y();
Object p2x = p2.x();
Object p2y = p2.y();
sink(p1x);
sink(p1y);
sink(p2x);
sink(p2y);
}

record RecWithGetter(Object f) {
public Object f() {
return this.f;
}
}

record RecWithWeirdGetter1(Object f) {
public Object f() {
return new Object();
}
}

record RecWithWeirdGetter2(Object f) {
public Object f() {
return source();
}
}

void testExplicitGetter1() {
RecWithGetter r1 = new RecWithGetter(source());
RecWithWeirdGetter1 r2 = new RecWithWeirdGetter1(source());
RecWithWeirdGetter2 r3 = new RecWithWeirdGetter2(source());
testExplicitGetter2(r1, r2, r3);
}

void testExplicitGetter2(RecWithGetter r1, RecWithWeirdGetter1 r2, RecWithWeirdGetter2 r3) {
sink(r1.f);
sink(r2.f);
sink(r3.f);
Object r1f = r1.f();
Object r2f = r2.f();
Object r3f = r3.f();
sink(r1f);
sink(r2f);
sink(r3f);
}
}
1 change: 1 addition & 0 deletions java/ql/test/library-tests/dataflow/records/options
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
//semmle-extractor-options: --javac-args --enable-preview -source 14 -target 14
9 changes: 9 additions & 0 deletions java/ql/test/library-tests/dataflow/records/test.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
| A.java:9:24:9:31 | source(...) | A.java:15:10:15:13 | p1.x |
| A.java:9:24:9:31 | source(...) | A.java:23:10:23:12 | p1x |
| A.java:10:38:10:45 | source(...) | A.java:18:10:18:13 | p2.y |
| A.java:10:38:10:45 | source(...) | A.java:26:10:26:12 | p2y |
| A.java:43:14:43:21 | source(...) | A.java:63:10:63:12 | r3f |
| A.java:48:42:48:49 | source(...) | A.java:55:10:55:13 | r1.f |
| A.java:48:42:48:49 | source(...) | A.java:61:10:61:12 | r1f |
| A.java:49:54:49:61 | source(...) | A.java:56:10:56:13 | r2.f |
| A.java:50:54:50:61 | source(...) | A.java:57:10:57:13 | r3.f |
15 changes: 15 additions & 0 deletions java/ql/test/library-tests/dataflow/records/test.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import java
import semmle.code.java.dataflow.DataFlow
import DataFlow

class Conf extends Configuration {
Conf() { this = "qqconf" }

override predicate isSource(Node n) { n.asExpr().(MethodAccess).getMethod().hasName("source") }

override predicate isSink(Node n) { n.asExpr().(Argument).getCall().getCallee().hasName("sink") }
}

from Conf conf, Node src, Node sink
where conf.hasFlow(src, sink)
select src, sink