Java: model remaining subpackages of Apache Commons Collections #6684

owen-mc · 2021-09-13T15:34:49Z

Currently the tests are missing. I will add them after the models have been reviewed, to avoid having to update the tests for any models that are changed.

smowton

Note on protected constructors / static methods; these are probably of very marginal relevance for dataflow purposes, so I think it would be fine to either skip testing them and just make a best effort, or if need be drop the models.

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

smowton · 2021-09-14T15:17:40Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+          ".bidimap;AbstractSortedBidiMapDecorator;true;AbstractSortedBidiMapDecorator;;;MapValue of Argument[0];MapValue of Argument[-1];value",
+          ".bidimap;DualHashBidiMap;true;DualHashBidiMap;;;MapKey of Argument[0];MapKey of Argument[-1];value",
+          ".bidimap;DualHashBidiMap;true;DualHashBidiMap;;;MapValue of Argument[0];MapValue of Argument[-1];value",
+          ".bidimap;DualHashBidiMap;true;DualHashBidiMap;;;MapKey of Argument[1];MapValue of Argument[-1];value",


These have createBidiMap methods too, though they're protected so don't matter much (but we are covering protected constructors, so might want to pick them up for consistency)

atorralba

Reviewed the .collection and .iterators packages.

About wrapper classes, I was wondering if we could model their "proxying" aspect and @joefarebrother mentioned that maybe that could be fixed using synthetic fields, which theoretically would enable backtracking side effects to the wrapped collection, e.g.

List a = new ArrayList();
WrappedList b = wrap(a);
b.add(source());
sink(a.get(0));

But it adds a significant amount of effort, since all inherited methods from the wrapped class hierarchy would need to be remodeled, so I don't know if it's worth it.

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

atorralba · 2021-09-15T09:29:43Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+          ".iterators;PeekingIterator;true;peekingIterator;;;Element of Argument[0];Element of ReturnValue;value",
+          ".iterators;PeekingIterator;true;peek;;;Element of Argument[-1];ReturnValue;value",
+          ".iterators;PeekingIterator;true;element;;;Element of Argument[-1];ReturnValue;value",
+          ".iterators;PermutationIterator;true;PermutationIterator;;;Element of Argument[0];Element of Element of Argument[-1];value",


This is smart :)

bmuskalla · 2021-09-15T09:58:14Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+          ".list;AbstractLinkedList;true;removeLast;;;Element of Argument[-1];ReturnValue;value",
+          ".list;AbstractLinkedList;true;updateNode;;;Argument[1];Element of Argument[-1];value",
+          ".list;AbstractLinkedList;true;updateNode;;;Argument[1];Element of Argument[0];value",
+          ".list;AbstractLinkedList;true;createNode;;;Argument[0];Element of ReturnValue;value",


Interesting practice. Even though there currently is nothing affect the qualifier, the javadoc explicitly states this intention (Subclasses can override this to record the change). I think that's fair to assume this flow for all subclasses. Good to know for the model generator

🐟 I have chosen to model AbstractLinkedList in a slightly less precise way than you might expect. I have conflated the contents of any particular node with the contents of all the nodes. The problem this solves is that from any node you can get to any other node using next and previous (and I expect these methods to be commonly used).

In the end I removed most of these models, because they are for private methods which I don't expect to be used as the public interface for any subclasses.

bmuskalla · 2021-09-15T09:59:19Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+          ".list;AbstractLinkedList;true;updateNode;;;Argument[1];Element of Argument[0];value",
+          ".list;AbstractLinkedList;true;createNode;;;Argument[0];Element of ReturnValue;value",
+          ".list;AbstractLinkedList;true;addNodeBefore;;;Argument[1];Element of Argument[-1];value",
+          ".list;AbstractLinkedList;true;addNodeBefore;;;Argument[1];Element of Argument[0];value",


I don't think this affects Argument[0] as it only defines the insertion point

bmuskalla · 2021-09-15T09:59:33Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+          ".list;AbstractLinkedList;true;addNodeBefore;;;Argument[1];Element of Argument[-1];value",
+          ".list;AbstractLinkedList;true;addNodeBefore;;;Argument[1];Element of Argument[0];value",
+          ".list;AbstractLinkedList;true;addNodeAfter;;;Argument[1];Element of Argument[-1];value",
+          ".list;AbstractLinkedList;true;addNodeAfter;;;Argument[1];Element of Argument[0];value",


Same as above

bmuskalla · 2021-09-15T10:06:48Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+          ".list;AbstractLinkedList;true;addNodeAfter;;;Argument[1];Element of Argument[-1];value",
+          ".list;AbstractLinkedList;true;addNodeAfter;;;Argument[1];Element of Argument[0];value",
+          ".list;AbstractLinkedList;true;addNode;;;Element of Argument[0];Element of Argument[-1];value",
+          ".list;AbstractLinkedList;true;addNode;;;Element of Argument[0];Element of Argument[1];value",


Not sure how this affects Argument[1]. From the code, it does touch the linked nodes next/previous fields but that's no value flow

bmuskalla · 2021-09-15T10:09:28Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+          ".list;AbstractLinkedList;true;addNode;;;Element of Argument[0];Element of Argument[-1];value",
+          ".list;AbstractLinkedList;true;addNode;;;Element of Argument[0];Element of Argument[1];value",
+          ".list;AbstractLinkedList;true;getNode;;;Element of Argument[-1];Element of ReturnValue;value",
+          ".list;AbstractLinkedList;true;createSubListIterator;;;Element of Argument[-1];Element of ReturnValue;value",


This doesn't seem to carry anything from the qualifier, only from the subList (arg-0)

I have deliberately done this to avoid FNs when we have not been able to track value flow into arg[0]. It does risk creating FPs.

In the end I removed most of these models, because they are for private methods which I don't expect to be used as the public interface for any subclasses.

bmuskalla · 2021-09-15T10:30:53Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+          ".map;CompositeMap;true;CompositeMap;(Map,Map);;MapValue of Argument[0];MapValue of Argument[-1];value",
+          ".map;CompositeMap;true;CompositeMap;(Map,Map);;MapKey of Argument[1];MapKey of Argument[-1];value",
+          ".map;CompositeMap;true;CompositeMap;(Map,Map);;MapValue of Argument[1];MapValue of Argument[-1];value",
+          ".map;CompositeMap;true;CompositeMap;(Map,Map,MapMutator);;MapKey of Argument[0];MapKey of Argument[-1];value",


I wonder if the variations including the MapMutator should be handled as taint

My impression was that the MapMutator was intended to be used to decide which of the array of maps to do the mutation operation on, which we don't care about in our model.

bmuskalla · 2021-09-15T10:37:34Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+          ".map;MultiValueMap;true;multiValueMap;;;Element of MapValue of Argument[0];Element of MapValue of ReturnValue;value",
+          ".map;MultiValueMap;true;getCollection;;;Element of MapValue of Argument[-1];Element of ReturnValue;value",
+          ".map;MultiValueMap;true;putAll;;;Argument[0];MapKey of Argument[-1];value",
+          ".map;MultiValueMap;true;putAll;;;Element of Argument[1];Element of MapValue of Argument[-1];value",


Suggested change

".map;MultiValueMap;true;putAll;;;Element of Argument[1];Element of MapValue of Argument[-1];value",

".map;MultiValueMap;true;putAll;;;Element of Argument[0];Element of MapValue of Argument[-1];value",

I'm modelling the putAll(K key, Collection<V> values) method here, not putAll(Map<? extends K,?> map) (which is inherited from Map). I'll explicitly give the signature to make this clearer.

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

bmuskalla · 2021-09-15T10:41:50Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+          ".properties;AbstractPropertiesFactory;true;load;(Reader);;Argument[0];ReturnValue;taint",
+          ".properties;AbstractPropertiesFactory;true;load;(String);;Argument[0];ReturnValue;taint",
+          ".properties;AbstractPropertiesFactory;true;load;(URI);;Argument[0];ReturnValue;taint",
+          ".properties;AbstractPropertiesFactory;true;load;(URL);;Argument[0];ReturnValue;taint"


Do we need to model SortedProperties. I couldn't find (yet) where we model Properties to account for that

What would you model in SortedProperties? I don't think we model Properties for data flow at the moment. We probably should. We do model Dictionary, which is a superclass of it.

bmuskalla · 2021-09-15T10:44:16Z

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

+/**
+ * Value-propagating models for the package `org.apache.commons.collections4.multiset`.
+ */
+private class ApacheMultiSetModel extends SummaryModelCsv {


I'm somehow missing MultiSet with methods like uniqueSet

That is in the base package, rather than this subpackage. It is modelled in the class ApacheCollectionsModel.

java/ql/lib/semmle/code/java/frameworks/apache/Collections.qll

Model value flow for Element, MapKey and MapValue. This assumes that clone() is a shallow copy.

Also update test.ql to use the new InlineFlowTest.

# Conflicts: # java/ql/test/library-tests/frameworks/apache-collections/TestNew.java

owen-mc · 2021-09-27T15:27:15Z

I've now added the tests (and stubs). I fixed some errors in models which became obvious on the way (including in old models for this package).

I had to force-push because I rebased on main to get some improvements to helper utilities, but I haven't changed the original four commits at all, so they shouldn't need to be re-reviewed.

java/ql/test/library-tests/frameworks/apache-collections/test.ql

owen-mc requested a review from a team as a code owner September 13, 2021 15:34

github-actions bot added the Java label Sep 13, 2021

owen-mc force-pushed the java/model/apache-collections-subpackages branch from c1a9f91 to 612966c Compare September 13, 2021 15:37

owen-mc marked this pull request as draft September 13, 2021 21:11

smowton reviewed Sep 14, 2021

View reviewed changes

atorralba reviewed Sep 15, 2021

View reviewed changes

bmuskalla reviewed Sep 15, 2021

View reviewed changes

smowton reviewed Sep 15, 2021

View reviewed changes

owen-mc force-pushed the java/model/apache-collections-subpackages branch from 78a3b1b to cd07422 Compare September 15, 2021 15:40

owen-mc added 19 commits September 27, 2021 16:24

Model java.lang.Object.clone() better for access paths.

3d1d491

Model value flow for Element, MapKey and MapValue. This assumes that clone() is a shallow copy.

Model the remaining subpackages in Apache Commons Collections

fd0fb94

Address problems highlighted by generating tests

e1750ad

Address review comments

3b678bf

Do not modelled protected static inner classes

cb0f82c

Do not model some protected methods

9b12980

Miscellaneous model fixes

f69787a

Add automatically generated tests

a20acfe

Also update test.ql to use the new InlineFlowTest.

Fix errors in generated tests that stop compilation

53ee465

Make concrete subclasses of abstract classes

15161d8

Fix up automatically generated tests

0a92b04

Add tests for non-public abstract classes

c51fb00

Remove redundant casts

768203b

Minor improvement to existing tests

e1101e5

Fix existing models for MapUtils

342c148

Merge new and old tests

cf03bd8

# Conflicts: # java/ql/test/library-tests/frameworks/apache-collections/TestNew.java

Fix up old tests for new helper functions

e6df816

Generate stubs

29db42c

Fix stub

bdd78d2

owen-mc force-pushed the java/model/apache-collections-subpackages branch from cd07422 to bdd78d2 Compare September 27, 2021 15:24

owen-mc marked this pull request as ready for review September 27, 2021 16:19

Add a change note

787f36f

github-actions bot added the documentation label Sep 28, 2021

atorralba reviewed Sep 28, 2021

View reviewed changes

java/ql/test/library-tests/frameworks/apache-collections/test.ql Outdated Show resolved Hide resolved

Remove models that are no longer required

39a12a8

smowton approved these changes Sep 28, 2021

View reviewed changes

smowton merged commit 413ac4e into github:main Sep 28, 2021

owen-mc deleted the java/model/apache-collections-subpackages branch September 28, 2021 13:33

	".map;MultiValueMap;true;putAll;;;Element of Argument[1];Element of MapValue of Argument[-1];value",
	".map;MultiValueMap;true;putAll;;;Element of Argument[0];Element of MapValue of Argument[-1];value",

Java: model remaining subpackages of Apache Commons Collections #6684

Java: model remaining subpackages of Apache Commons Collections #6684

Uh oh!

Conversation

owen-mc commented Sep 13, 2021

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atorralba left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owen-mc Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owen-mc Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owen-mc Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owen-mc commented Sep 27, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

atorralba left a comment •

edited

Loading

owen-mc Sep 15, 2021 •

edited

Loading

owen-mc Sep 15, 2021 •

edited

Loading

owen-mc Sep 15, 2021 •

edited

Loading