Improve thread safety issue #7338

jeremylong · Jan 30, 2025 · 4fccbec · 4fccbec
1 parent bebb431
commit 4fccbec
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 17 deletions.
diff --git a/core/src/main/java/org/owasp/dependencycheck/analyzer/NvdCveAnalyzer.java b/core/src/main/java/org/owasp/dependencycheck/analyzer/NvdCveAnalyzer.java
@@ -18,7 +18,9 @@
 package org.owasp.dependencycheck.analyzer;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import javax.annotation.concurrent.ThreadSafe;
 
 import org.owasp.dependencycheck.Engine;
@@ -150,17 +152,17 @@ private synchronized List<Vulnerability> filterEcosystem(String ecosystem, List<
         final List<Vulnerability> remove = new ArrayList<>();
         vulnerabilities.forEach((v) -> {
             boolean found = false;
-            final List<VulnerableSoftware> removeSoftare = new ArrayList<>();
+            final Set<VulnerableSoftware> removeSoftware = new HashSet<>();
             for (VulnerableSoftware s : v.getVulnerableSoftware()) {
                 if (ecosystemMatchesTargetSoftware(ecosystem, s.getTargetSw())) {
                     found = true;
                 } else {
-                    removeSoftare.add(s);
+                    removeSoftware.add(s);
                 }
             }
             if (found) {
-                if (!removeSoftare.isEmpty()) {
-                    removeSoftare.forEach(v.getVulnerableSoftware()::remove);
+                if (!removeSoftware.isEmpty()) {
+                    v.removeVulnerableSoftware(removeSoftware);
                 }
             } else {
                 remove.add(v);

diff --git a/core/src/main/java/org/owasp/dependencycheck/analyzer/OssIndexAnalyzer.java b/core/src/main/java/org/owasp/dependencycheck/analyzer/OssIndexAnalyzer.java
@@ -271,7 +271,7 @@ void enrich(final Dependency dependency) {
                                             .orElse(null);
                                     if (existing != null) {
                                         //TODO - can we enhance anything other than the references?
-                                        existing.getReferences().addAll(v.getReferences());
+                                        existing.addReferences(v.getReferences());
                                     } else {
                                         dependency.addVulnerability(v);
                                     }

diff --git a/core/src/main/java/org/owasp/dependencycheck/dependency/Vulnerability.java b/core/src/main/java/org/owasp/dependencycheck/dependency/Vulnerability.java
@@ -23,9 +23,9 @@
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import javax.annotation.concurrent.NotThreadSafe;
 
 import org.apache.commons.lang3.builder.CompareToBuilder;
@@ -92,11 +92,17 @@ public enum Source {
     /**
      * References for this vulnerability.
      */
-    private final Set<Reference> references = Collections.synchronizedSet(new HashSet<>());
+    private final Set<Reference> references = ConcurrentHashMap.newKeySet();
     /**
      * A set of vulnerable software.
      */
-    private final Set<VulnerableSoftware> vulnerableSoftware = new HashSet<>();
+    private final Set<VulnerableSoftware> vulnerableSoftware = ConcurrentHashMap.newKeySet();
+    /**
+     * Immutable views for getters.
+     */
+    private final Set<Reference> referencesView = Collections.unmodifiableSet(references);
+    private final Set<VulnerableSoftware> vulnerableSoftwareView = Collections.unmodifiableSet(vulnerableSoftware);
+
     /**
      * The CWE(s) for the vulnerability.
      */
@@ -196,7 +202,7 @@ public void setDescription(String description) {
      * @return the value of references
      */
     public Set<Reference> getReferences() {
-        return references;
+        return referencesView;
     }
 
     /**
@@ -271,7 +277,7 @@ public org.owasp.dependencycheck.data.knownexploited.json.Vulnerability getKnown
      * @return the value of vulnerableSoftware
      */
     public Set<VulnerableSoftware> getVulnerableSoftware() {
-        return vulnerableSoftware;
+        return vulnerableSoftwareView;
     }
 
     /**
@@ -283,13 +289,20 @@ public Set<VulnerableSoftware> getVulnerableSoftware() {
      */
     @SuppressWarnings("unchecked")
     public List<VulnerableSoftware> getVulnerableSoftware(boolean sorted) {
-        synchronized (vulnerableSoftware) {
-            final List<VulnerableSoftware> sortedVulnerableSoftware = new ArrayList<>(this.vulnerableSoftware);
-            if (sorted) {
-                Collections.sort(sortedVulnerableSoftware);
-            }
-            return sortedVulnerableSoftware;
+        final List<VulnerableSoftware> sortedVulnerableSoftware = new ArrayList<>(this.vulnerableSoftware);
+        if (sorted) {
+            Collections.sort(sortedVulnerableSoftware);
         }
+        return sortedVulnerableSoftware;
+    }
+
+    /**
+     * Removes the specified vulnerableSoftware from the collection.
+     *
+     * @param vulnerableSoftware a collection of vulnerable software to be removed
+     */
+    public void removeVulnerableSoftware(Set<VulnerableSoftware> vulnerableSoftware) {
+        this.vulnerableSoftware.removeAll(vulnerableSoftware);
     }
 
     /**

diff --git a/core/src/main/java/org/owasp/dependencycheck/processing/BundlerAuditProcessor.java b/core/src/main/java/org/owasp/dependencycheck/processing/BundlerAuditProcessor.java
@@ -201,7 +201,7 @@ private void addReferenceToVulnerability(String parentName, Vulnerability vulner
             ref.setName(vulnerability.getName());
             ref.setSource("bundle-audit");
             ref.setUrl(url);
-            vulnerability.getReferences().add(ref);
+            vulnerability.addReference(ref);
         }
         LOGGER.debug("bundle-audit ({}): {}", parentName, nextLine);
     }