dependency-check / dependency-check/DependencyCheck
Possible to get ConcurrentModificationException with JarAnalyzer during dependency removal
- Dominant language
- Java
- Stars
- 7.7k
- Forks
- 1.4k
- Avg merge
- 9d 22h
- Merged PRs (30d)
- 13
Description
**Precondition**
- [x] I checked the issues list for existing open or closed reports of the same problem.
**Describe the bug**
Any of the logic which removes dependencies via analyzers is vulnerable to failing with `ConcurrentModificationException` because the remove needs to be done from `List`s which must find matching elements by evaluating object equality.
https://github.com/dependency-check/DependencyCheck/blob/cf0680bac47c65d1a8f68cb4d5345ee9c3e63086/core/src/main/java/org/owasp/dependencycheck/analyzer/JarAnalyzer.java#L317
https://github.com/dependency-check/DependencyCheck/blob/a318a7e06b40029e8a05fcff46279082feefa9bf/core/src/main/java/org/owasp/dependencycheck/Engine.java#L281
https://github.com/dependency-check/DependencyCheck/blob/a318a7e06b40029e8a05fcff46279082feefa9bf/core/src/main/java/org/owasp/dependencycheck/Engine.java#L108
Lots of analyzers remove dependencies like this, but appear to have different threading models. In any case, there are possibly other analyzers, or threads running the `JarAnalyzer` that are adding evidence at the same time, which could break the equality checks.
In this case, the CME was at
https://github.com/dependency-check/DependencyCheck/blob/794723481b70c59384f8b330238aceedbb7ac1f8/core/src/main/java/org/owasp/dependencycheck/dependency/EvidenceCollection.java#L410
...implying something else was modifying the vendor evidence for 1+ dependency.
The multi-layered synchronization is likely ineffective because `Engine.removeDependency` and `EvidenceCollection.addEvidence` use different object locks, and `EvidenceCollection.equals/hashcode` etc are not synchronized.
**Version of dependency-check used**
12.2.0, via `EngineIT` in this case, but believe possibly real world possible as well - to verify.
**Log file**
```
[WARN] An unexpected error occurred during analysis of '/home/runner/work/DependencyCheck/DependencyCheck/core/target/test-classes/bootable-0.1.0.jar' (Jar Analyzer): null
java.util.ConcurrentModificationException
at java.base/java.util.TreeMap$PrivateEntryIterator.nextEntry(TreeMap.java:1208)
at java.base/java.util.TreeMap$KeyIterator.next(TreeMap.java:1262)
at java.base/java.util.AbstractCollection.containsAll(AbstractCollection.java:324)
at java.base/java.util.AbstractSet.equals(AbstractSet.java:95)
at org.apache.commons.lang3.builder.EqualsBuilder.append(EqualsBuilder.java:739)
at org.owasp.dependencycheck.dependency.EvidenceCollection.equals(EvidenceCollection.java:410)
at org.owasp.dependencycheck.dependency.Dependency.equals(Dependency.java:938)
at java.base/java.util.ArrayList.remove(ArrayList.java:656)
at java.base/java.util.Collections$SynchronizedCollection.remove(Collections.java:2043)
at org.owasp.dependencycheck.Engine.removeDependency(Engine.java:281)
at org.owasp.dependencycheck.analyzer.JarAnalyzer.analyzeDependency(JarAnalyzer.java:317)
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.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at java.base/java.lang.Thread.run(Thread.java:829)
```
https://github.com/chadlwilson/DependencyCheck/actions/runs/24352391403/job/71110132139#step:11:1658
**To Reproduce**
Probably running `EngineIT` enough times with bad luck is sufficient, but needs a closer look.
**Expected behavior**
Should be threadsafe :-)
**Additional context**
Creating this as a reminder for me/someone else to take a look at this at some point.
- synchronizing `hashCode` and `equals` on the `EvidenceCollection` might be enough, albeit making collection usage and list searches especially slow.
- `TreeSet`s maybe could be replaced with `ConcurrentSkipListSet`s, but might not solve the "whole" problem, and better to look at it holistically.
- the `Engine.dependencies` `synchronizedList` seems a bit of an odd data structure choice as it makes these removals expensive, and involving a lot of deep equality calculations on otherwise mutable objects which are likely to have thread safety issues. This whole process could possibly be made both faster and lower chance of concurrency issues _without changing data structure_ by doing the removal manually (don't use `List.remove(Object)` and instead implement manually based on an _object identity check_ alone to find the matching item via an `Iterator` and then removing via the iterator. Probably better to use a `ConcurrentList` with safe iteration for that as well.
Contributor guide
Assessment
This issue has not been assessed yet.