Improve thread safety issue #7338

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

Hide file tree

Showing 4 changed files with 49 additions and 18 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,7 +152,7 @@ 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> removeSoftare = new HashSet<>();
             for (VulnerableSoftware s : v.getVulnerableSoftware()) {
                 if (ecosystemMatchesTargetSoftware(ecosystem, s.getTargetSw())) {
                     found = true;
@@ -160,7 +162,7 @@ private synchronized List<Vulnerability> filterEcosystem(String ecosystem, List<
             }
             if (found) {
                 if (!removeSoftare.isEmpty()) {
-                    removeSoftare.forEach(v.getVulnerableSoftware()::remove);
+                    v.removeVulnerableSoftware(removeSoftare);
                 }
             } 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
@@ -92,7 +92,7 @@ public enum Source {
     /**
      * References for this vulnerability.
      */
-    private final Set<Reference> references = Collections.synchronizedSet(new HashSet<>());
+    private final Set<Reference> references = new HashSet<>();
     /**
      * A set of vulnerable software.
      */
@@ -196,7 +196,9 @@ public void setDescription(String description) {
      * @return the value of references
      */
     public Set<Reference> getReferences() {
-        return references;
+        synchronized (references) {
+            return Collections.unmodifiableSet(new HashSet<>(references));
+        }
     }
 
     /**
@@ -207,7 +209,10 @@ public Set<Reference> getReferences() {
      * @return the list of references
      */
     public List<Reference> getReferences(boolean sorted) {
-        final List<Reference> sortedRefs = new ArrayList<>(this.references);
+        final List<Reference> sortedRefs;
+        synchronized (references) {
+            sortedRefs = new ArrayList<>(references);
+        }
         if (sorted) {
             Collections.sort(sortedRefs);
         }
@@ -220,7 +225,9 @@ public List<Reference> getReferences(boolean sorted) {
      * @param references a collection of references to add
      */
     public void addReferences(Set<Reference> references) {
-        this.references.addAll(references);
+        synchronized (this.references) {
+            this.references.addAll(references);
+        }
     }
 
     /**
@@ -229,7 +236,9 @@ public void addReferences(Set<Reference> references) {
      * @param ref a reference for the vulnerability
      */
     public void addReference(Reference ref) {
-        this.references.add(ref);
+        synchronized (this.references) {
+            this.references.add(ref);
+        }
     }
 
     /**
@@ -244,7 +253,9 @@ public void addReference(String referenceSource, String referenceName, String re
         ref.setSource(referenceSource);
         ref.setName(referenceName);
         ref.setUrl(referenceUrl);
-        this.references.add(ref);
+        synchronized (this.references) {
+            this.references.add(ref);
+        }
     }
 
     /**
@@ -271,7 +282,9 @@ public org.owasp.dependencycheck.data.knownexploited.json.Vulnerability getKnown
      * @return the value of vulnerableSoftware
      */
     public Set<VulnerableSoftware> getVulnerableSoftware() {
-        return vulnerableSoftware;
+        synchronized (vulnerableSoftware) {
+            return Collections.unmodifiableSet(new HashSet<>(vulnerableSoftware));
+        }
     }
 
     /**
@@ -283,12 +296,24 @@ public Set<VulnerableSoftware> getVulnerableSoftware() {
      */
     @SuppressWarnings("unchecked")
     public List<VulnerableSoftware> getVulnerableSoftware(boolean sorted) {
+        final List<VulnerableSoftware> sortedVulnerableSoftware;
         synchronized (vulnerableSoftware) {
-            final List<VulnerableSoftware> sortedVulnerableSoftware = new ArrayList<>(this.vulnerableSoftware);
-            if (sorted) {
-                Collections.sort(sortedVulnerableSoftware);
-            }
-            return sortedVulnerableSoftware;
+            sortedVulnerableSoftware = new ArrayList<>(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) {
+        synchronized (this.vulnerableSoftware) {
+            this.vulnerableSoftware.removeAll(vulnerableSoftware);
         }
     }
 
@@ -298,7 +323,9 @@ public List<VulnerableSoftware> getVulnerableSoftware(boolean sorted) {
      * @param vulnerableSoftware a collection of vulnerable software
      */
     public void addVulnerableSoftware(Set<VulnerableSoftware> vulnerableSoftware) {
-        this.vulnerableSoftware.addAll(vulnerableSoftware);
+        synchronized (this.vulnerableSoftware) {
+            this.vulnerableSoftware.addAll(vulnerableSoftware);
+        }
     }
 
     /**
@@ -307,7 +334,9 @@ public void addVulnerableSoftware(Set<VulnerableSoftware> vulnerableSoftware) {
      * @param software the vulnerable software reference to add
      */
     public void addVulnerableSoftware(VulnerableSoftware software) {
-        vulnerableSoftware.add(software);
+        synchronized (this.vulnerableSoftware) {
+            vulnerableSoftware.add(software);
+        }
     }
 
     /**

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);
     }