Add models for Apache Commons Lang's tuple types #5772

smowton · 2021-04-26T14:42:42Z

This also adds support for (fully-qualified) fields.

My approach here is to keep the models to the concrete types (Im)?mutable(Pair|Triple), relying on virtual dispatch to determine what happens to calls against the interfaces Pair and Triple. This can fail when neither type is referred to explicitly and tuples are provided by external code already constructed, however I don't add the synthetic field needed for this case in this PR.

hvitved

Very nice! Did you forget to check in the test .ql and .expected files?

hvitved · 2021-04-27T06:48:32Z

java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll

 private SummaryComponent interpretComponent(string c) {
  specSplit(_, c, _) and
  (
    exists(int pos | parseArg(c, pos) and result = SummaryComponent::argument(pos))
    or
    exists(int pos | parseParam(c, pos) and result = SummaryComponent::parameter(pos))
    or
+    c.matches("Field%") and result = SummaryComponent::field(interpretField(c.splitAt(" ", 1)))


"Field %"?

hvitved · 2021-04-27T06:50:47Z

java/ql/src/semmle/code/java/dataflow/internal/DataFlowPrivate.qll

@@ -137,10 +137,10 @@ class MapValueContent extends Content, TMapValueContent {
 * Thus, `node2` references an object with a field `f` that contains the
 * value of `node1`.
 */
-predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
+predicate storeStep(Node node1, Content f, Node node2) {


Just verifying that this change is expected.

Indeed, because we now have storeSteps pointed at synthetic nodes that don't have PUNs (perhaps they should?)

smowton · 2021-04-27T11:19:25Z

No files are forgotten; the flow.ql and empty .expected (for an inline-expectation test) are already present in the commons-lang3 directory.

Marcono1234

It looks like the public constructors are not modeled, is that on purpose?

Marcono1234 · 2021-05-01T13:31:52Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+        "org.apache.commons.lang3.tuple;ImmutablePair;false;right;;;Argument[0];Field org.apache.commons.lang3.tuple.ImmutablePair.right of ReturnValue;value",
+        "org.apache.commons.lang3.tuple;ImmutablePair;false;left;;;Argument[0];Field org.apache.commons.lang3.tuple.ImmutablePair.left of ReturnValue;value",


Really minor, but is it intended that here "right" is listed before "left"? In all other cases it seems to be the other way around.

Marcono1234 · 2021-05-01T13:40:13Z

java/ql/test/library-tests/frameworks/apache-commons-lang3/TripleTest.java

+
+    void sink(Object o) {}
+
+    void test() throws Exception {


This seems to be missing a test for Triple.of(left, middle, right)
Might be good to add it for completeness?

Marcono1234 · 2021-05-01T13:41:34Z

java/ql/test/library-tests/frameworks/apache-commons-lang3/PairTest.java

+
+    void sink(Object o) {}
+
+    void test() throws Exception {


This seems to be missing a test for Pair.of(left, right)
Might be good to add it for completeness?

smowton · 2021-05-11T11:06:03Z

@Marcono1234 thanks, done

aschackmull · 2021-06-17T10:57:02Z

java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll

+bindingset[name]
+private Field interpretField(string name) {
+  exists(string splitRegex, string package, string className, string fieldName |
+    splitRegex = "^(.*)\\.([^.]+)\\.([^.]+)$" and
+    package = name.regexpCapture(splitRegex, 1) and
+    className = name.regexpCapture(splitRegex, 2) and
+    fieldName = name.regexpCapture(splitRegex, 3)
+  |
+    result = any(Field f | f.hasQualifiedName(package, className, fieldName))
+  )
+}


Instead of having a bindingset-annotated predicate, I'd prefer to match the style of parseArg and parseParam.

Suggested change

bindingset[name]

private Field interpretField(string name) {

exists(string splitRegex, string package, string className, string fieldName |

splitRegex = "^(.*)\\.([^.]+)\\.([^.]+)$" and

package = name.regexpCapture(splitRegex, 1) and

className = name.regexpCapture(splitRegex, 2) and

fieldName = name.regexpCapture(splitRegex, 3)

|

result = any(Field f | f.hasQualifiedName(package, className, fieldName))

)

}

private predicate parseField(string c, Field f) {

specSplit(_, c, _) and

exists(string splitRegex, string package, string className, string fieldName |

fieldRegex = "^Field (.*)\\.([^.]+)\\.([^.]+)$" and

package = c.regexpCapture(fieldRegex, 1) and

className = c.regexpCapture(fieldRegex, 2) and

fieldName = c.regexpCapture(fieldRegex, 3) and

f.hasQualifiedName(package, className, fieldName)

)

}

smowton · 2021-06-17T11:50:22Z

@aschackmull done, and style changed from Field foo of to Field[foo] of

aschackmull · 2021-06-17T11:54:06Z

I've got a feeling that the tests in java/ql/test/library-tests/dataflow/external-models/ might fail...

smowton · 2021-06-17T12:01:50Z

Nope, those tests seem fine. I don't immediately see what trouble you would expect.

aschackmull · 2021-06-17T12:11:10Z

I was thinking of query predicate invalidModelRow.

smowton · 2021-06-17T13:55:51Z

I think that's fine because it defers to invalidSpecComponent which in turn uses not exists(interpretComponent(c)), so there is no longer a need to double-specify valid formats as of 14f9a5c which deleted the line part.regexpMatch("|ReturnValue|ArrayElement|Element|MapKey|MapValue")

aschackmull · 2021-06-18T08:35:15Z

Ah, perfect!

smowton requested a review from a team as a code owner April 26, 2021 14:42

github-actions bot added the Java label Apr 26, 2021

hvitved reviewed Apr 27, 2021

View reviewed changes

smowton mentioned this pull request Apr 28, 2021

Java: Add synthetic fields; model Commons Lang's MutableObject type #5796

Merged

Marcono1234 reviewed May 1, 2021

View reviewed changes

github-actions bot added the documentation label May 11, 2021

aschackmull reviewed Jun 17, 2021

View reviewed changes

smowton added 9 commits June 17, 2021 12:24

Add support for CSV-specified flow to or from fields.

73fa680

Add models for Apache Commons tuples

472a2a6

Improve matching of Field specifiers; add Field recognition in tests

365aab9

Order left and right consistently

eebaab8

Add tests for Pair.of and Triple.of

fbaa382

Model constructors for (Imm|M)utable(Pair|Triple)

2cc1f46

Add change note

5cf0243

Improve style of interpretField

74b2a2c

Field foo of -> Field[foo] of

d28c95d

smowton force-pushed the smowton/feature/apache-tuple-flow branch from d91a3e9 to d28c95d Compare June 17, 2021 11:49

aschackmull approved these changes Jun 18, 2021

View reviewed changes

aschackmull merged commit 7eb6da3 into github:main Jun 18, 2021

		"org.apache.commons.lang3.tuple;ImmutablePair;false;right;;;Argument[0];Field org.apache.commons.lang3.tuple.ImmutablePair.right of ReturnValue;value",
		"org.apache.commons.lang3.tuple;ImmutablePair;false;left;;;Argument[0];Field org.apache.commons.lang3.tuple.ImmutablePair.left of ReturnValue;value",

Add models for Apache Commons Lang's tuple types #5772

Add models for Apache Commons Lang's tuple types #5772

Uh oh!

Conversation

smowton commented Apr 26, 2021

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

hvitved Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

hvitved Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

smowton Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

smowton commented Apr 27, 2021

Uh oh!

Marcono1234 left a comment

Choose a reason for hiding this comment

Uh oh!

Marcono1234 May 1, 2021

Choose a reason for hiding this comment

Uh oh!

Marcono1234 May 1, 2021

Choose a reason for hiding this comment

Uh oh!

Marcono1234 May 1, 2021

Choose a reason for hiding this comment

Uh oh!

smowton commented May 11, 2021

Uh oh!

aschackmull Jun 17, 2021

Choose a reason for hiding this comment

Uh oh!

smowton commented Jun 17, 2021

Uh oh!

aschackmull commented Jun 17, 2021

Uh oh!

smowton commented Jun 17, 2021

Uh oh!

aschackmull commented Jun 17, 2021

Uh oh!

smowton commented Jun 17, 2021

Uh oh!

aschackmull commented Jun 18, 2021

Uh oh!

Uh oh!