make synchronized

[jeremylong]

Given run 3231291349 there is a ConcurrentModificationException:

java.util.ConcurrentModificationException
    at java.util.ArrayList$Itr.checkForComodification (ArrayList.java:911)
    at java.util.ArrayList$Itr.next (ArrayList.java:861)
    at org.owasp.dependencycheck.analyzer.AbstractSuppressionAnalyzer.analyzeDependency (AbstractSuppressionAnalyzer.java:136)
    at org.owasp.dependencycheck.analyzer.CpeSuppressionAnalyzer.analyzeDependency (CpeSuppressionAnalyzer.java:91)
    at org.owasp.dependencycheck.analyzer.AbstractAnalyzer.analyze (AbstractAnalyzer.java:131)
    at org.owasp.dependencycheck.analyzer.CPEAnalyzer.determineIdentifiers (CPEAnalyzer.java:960)
    at org.owasp.dependencycheck.analyzer.CPEAnalyzer.determineCPE (CPEAnalyzer.java:298)
    at org.owasp.dependencycheck.analyzer.CPEAnalyzer.analyzeDependency (CPEAnalyzer.java:779)
    at org.owasp.dependencycheck.analyzer.AbstractAnalyzer.analyze (AbstractAnalyzer.java:131)
    at org.owasp.dependencycheck.AnalysisTask.call (AnalysisTask.java:88)
    at org.owasp.dependencycheck.AnalysisTask.call (AnalysisTask.java:37)
    at java.util.concurrent.FutureTask.run (FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:624)
    at java.lang.Thread.run (Thread.java:750)

This PR adds synchronization...

[aikebah]

Given run 3231291349 there is a ConcurrentModificationException:

...

This PR adds synchronization...

Making analyzeDependency a synchronized method would not prevent the rules list to be modified when analysis traverses it, so I don't think making this method synchronized would do any good (the method reads the list, but does not modify it).
The modification of the rules (likely for this case: resets (list.clear()) in testcases) should be guarded by synchronisation and the same synchronisation object should be used within analyzeDependency to prevent it running concurrently to list-modifying code.

[jeremylong]

@aikebah what I believe is happening is during execution on multi-module project - while module 1 is cycling over the suppression rules, module 2 triggers a reload on the suppression rules which causes module 1's execution to throw the concurrent modification exception.

We have only been seeing this error in the CI on 690-threadsafety - which is there to identify this type of issue.

[jeremylong]

So either we synchronize on the rules or we create a copy of the rules:

protected void analyzeDependency(Dependency dependency, Engine engine) throws AnalysisException {
        List<SuppressionRule> suppressions = new ArrayList<>();
        suppressions.addAll(rules.list());
        if (suppressions.isEmpty()) {
            return;
        }
        for (SuppressionRule rule : suppressions) {
...

Thoughts?

[aikebah]

calling analyzeDependency happens a lot (once for each dependency), so copying the rules for each dependency would likely cause a tremendous amount of wasted CPU cycles and memory. My gut feel would be that we're best off by changing the rules to a synchronized List and synchronizing the iteration on that same list as per https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#synchronizedList-java.util.List-

[aikebah]

at second thought: why would the same JVM reload it... I think the answer lies in 'different submodule with different suppressionFile configuration', which would mean we want to fully prevent that reload from happening while the entire analysis runs. That would call for a ThreadLocal rules list instead of a singleton, assuming that we process the entire analysis of a certain module within the same thread.

[jeremylong]

The reload is happening even with the same suppression file - 690-threadsafety does not contain any local suppression files. I think ThreadLocal rules are going to be the best option.

[jeremylong]

After thinking through this a bit more - I'm going to remove the singleton on the rules and add a new mechanism to store objects throughout the execution.