Possible to get ConcurrentModificationException with JarAnalyzer during dependency removal

[chadlwilson]

Precondition

• 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 Lists which must find matching elements by evaluating object equality.

DependencyCheck/core/src/main/java/org/owasp/dependencycheck/analyzer/JarAnalyzer.java

Line 317 in cf0680b

engine.removeDependency(dependency);

DependencyCheck/core/src/main/java/org/owasp/dependencycheck/Engine.java

Line 281 in a318a7e

dependencies.remove(dependency);

DependencyCheck/core/src/main/java/org/owasp/dependencycheck/Engine.java

Line 108 in a318a7e

private final List<Dependency> dependencies = Collections.synchronizedList(new ArrayList<>());

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

DependencyCheck/core/src/main/java/org/owasp/dependencycheck/dependency/EvidenceCollection.java

Line 410 in 7947234

.append(this.vendors, other.vendors)

...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.
• TreeSets maybe could be replaced with ConcurrentSkipListSets, 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.

[Umesh042005]

Hey @chadlwilson , been reading through this — the iterator-based removal with identity check (==) instead of .equals() seems like a clean fix for the CME. Is that the direction you'd prefer, or would you lean towards ConcurrentSkipListSet approach?

[Umesh042005]

Hey @chadlwilson , I've been reading through this carefully. The core issue is that List.remove(Object) internally calls .equals() which chains into EvidenceCollection.equals() and then into the TreeSet vendors comparison — and if another thread is modifying that vendor evidence at the same time, we get the CME.

Would the right fix be:

1. Use iterator-based removal with identity check (==) in Engine.removeDependency() to bypass the .equals() chain entirely
2. Synchronize equals() and hashCode() in EvidenceCollection

Happy to open a PR if this approach looks right to you!