fix: ensure open handles will not leak on errors (jeremylong#6326)

knalli · Dec 19, 2023 · 006684b · 006684b
1 parent c2bc4dc
commit 006684b
Show file tree

Hide file tree

Showing 4 changed files with 115 additions and 38 deletions.
diff --git a/...rc/main/java/org/owasp/dependencycheck/data/update/nvd/api/CveApiJson20CveItemSource.java b/...rc/main/java/org/owasp/dependencycheck/data/update/nvd/api/CveApiJson20CveItemSource.java
@@ -22,30 +22,23 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
 import io.github.jeremylong.openvulnerability.client.nvd.DefCveItem;
+import org.apache.commons.io.IOUtils;
 
-import java.io.BufferedInputStream;
-import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
-import java.nio.file.Files;
-import java.util.zip.GZIPInputStream;
 
 public class CveApiJson20CveItemSource implements CveItemSource<DefCveItem> {
 
-    private final File jsonFile;
     private final ObjectMapper mapper;
     private final InputStream inputStream;
     private final JsonParser jsonParser;
     private DefCveItem currentItem;
     private DefCveItem nextItem;
 
-    public CveApiJson20CveItemSource(File jsonFile) throws IOException {
-        this.jsonFile = jsonFile;
+    public CveApiJson20CveItemSource(InputStream inputStream) throws IOException {
         mapper = new ObjectMapper();
         mapper.registerModule(new JavaTimeModule());
-        inputStream = jsonFile.getName().endsWith(".gz") ?
-                new BufferedInputStream(new GZIPInputStream(Files.newInputStream(jsonFile.toPath()))) :
-                new BufferedInputStream(Files.newInputStream(jsonFile.toPath()));
+        this.inputStream = inputStream;
         jsonParser = mapper.getFactory().createParser(inputStream);
 
         JsonToken token = null;
@@ -62,9 +55,7 @@ public CveApiJson20CveItemSource(File jsonFile) throws IOException {
 
     @Override
     public void close() throws Exception {
-        jsonParser.close();
-        inputStream.close();
-        Files.delete(jsonFile.toPath());
+        IOUtils.closeQuietly(jsonParser, inputStream);
     }
 
     @Override

diff --git a/core/src/main/java/org/owasp/dependencycheck/data/update/nvd/api/JsonArrayCveItemSource.java b/core/src/main/java/org/owasp/dependencycheck/data/update/nvd/api/JsonArrayCveItemSource.java
@@ -22,30 +22,23 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
 import io.github.jeremylong.openvulnerability.client.nvd.DefCveItem;
+import org.apache.commons.io.IOUtils;
 
-import java.io.BufferedInputStream;
-import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
-import java.nio.file.Files;
-import java.util.zip.GZIPInputStream;
 
 public class JsonArrayCveItemSource implements CveItemSource<DefCveItem> {
 
-    private final File jsonFile;
     private final ObjectMapper mapper;
     private final InputStream inputStream;
     private final JsonParser jsonParser;
     private DefCveItem currentItem;
     private DefCveItem nextItem;
 
-    public JsonArrayCveItemSource(File jsonFile) throws IOException {
-        this.jsonFile = jsonFile;
+    public JsonArrayCveItemSource(InputStream inputStream) throws IOException {
         mapper = new ObjectMapper();
         mapper.registerModule(new JavaTimeModule());
-        inputStream = jsonFile.getName().endsWith(".gz") ?
-                new BufferedInputStream(new GZIPInputStream(Files.newInputStream(jsonFile.toPath()))) :
-                new BufferedInputStream(Files.newInputStream(jsonFile.toPath()));
+        this.inputStream = inputStream;
         jsonParser = mapper.getFactory().createParser(inputStream);
 
         if (jsonParser.nextToken() == JsonToken.START_ARRAY) {
@@ -55,9 +48,7 @@ public JsonArrayCveItemSource(File jsonFile) throws IOException {
 
     @Override
     public void close() throws Exception {
-        jsonParser.close();
-        inputStream.close();
-        Files.delete(jsonFile.toPath());
+        IOUtils.closeQuietly(jsonParser, inputStream);
     }
 
     @Override

diff --git a/core/src/main/java/org/owasp/dependencycheck/data/update/nvd/api/NvdApiProcessor.java b/core/src/main/java/org/owasp/dependencycheck/data/update/nvd/api/NvdApiProcessor.java
@@ -18,8 +18,15 @@
 package org.owasp.dependencycheck.data.update.nvd.api;
 
 import io.github.jeremylong.openvulnerability.client.nvd.DefCveItem;
+
+import java.io.BufferedInputStream;
 import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
 import java.util.concurrent.Callable;
+import java.util.zip.GZIPInputStream;
+
 import org.owasp.dependencycheck.data.nvd.ecosystem.CveEcosystemMapper;
 import org.owasp.dependencycheck.data.nvdcve.CveDB;
 import org.slf4j.Logger;
@@ -82,16 +89,7 @@ public NvdApiProcessor(final CveDB cveDB, File jsonFile) {
 
     @Override
     public NvdApiProcessor call() throws Exception {
-        CveItemSource<DefCveItem> itemSource = null;
-
-        if (jsonFile.getName().endsWith(".jsonarray.gz")) {
-            itemSource = new JsonArrayCveItemSource(jsonFile);
-        } else if (jsonFile.getName().endsWith(".gz")) {
-            itemSource = new CveApiJson20CveItemSource(jsonFile);
-        } else {
-            itemSource = new JsonArrayCveItemSource(jsonFile);
-        }
-        try {
+        try (CveItemSource<DefCveItem> itemSource = buildItemSource(jsonFile)) {
             while (itemSource.hasNext()) {
                 DefCveItem entry = itemSource.next();
                 try {
@@ -100,13 +98,27 @@ public NvdApiProcessor call() throws Exception {
                     LOGGER.error("Failed to process " + entry.getCve().getId(), ex);
                 }
             }
-        } finally {
-            itemSource.close();
         }
         endTime = System.currentTimeMillis();
         return this;
     }
 
+    static CveItemSource<DefCveItem> buildItemSource(File file) throws IOException {
+        if (file.getName().endsWith(".jsonarray.gz")) {
+            return new JsonArrayCveItemSource(new BufferedInputStream(new GZIPInputStream(
+                    Files.newInputStream(file.toPath())
+            )));
+        } else if (file.getName().endsWith(".gz")) {
+            return new CveApiJson20CveItemSource(new BufferedInputStream(new GZIPInputStream(
+                    Files.newInputStream(file.toPath())
+            )));
+        } else {
+            return new JsonArrayCveItemSource(new BufferedInputStream(
+                    Files.newInputStream(file.toPath())
+            ));
+        }
+    }
+
     /**
      * Calculates how long the update process took.
      *

diff --git a/core/src/test/java/org/owasp/dependencycheck/data/update/nvd/api/NvdApiProcessorTest.java b/core/src/test/java/org/owasp/dependencycheck/data/update/nvd/api/NvdApiProcessorTest.java
@@ -0,0 +1,83 @@
+/*
+ * Copyright 2014 OWASP.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.owasp.dependencycheck.data.update.nvd.api;
+
+import com.fasterxml.jackson.core.JsonParseException;
+import org.junit.Test;
+import org.owasp.dependencycheck.BaseTest;
+import org.owasp.dependencycheck.data.nvdcve.CveDB;
+import org.owasp.dependencycheck.utils.Settings;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.nio.file.NoSuchFileException;
+
+import static org.junit.Assert.assertThrows;
+
+/**
+ *
+ * @author Jeremy Long
+ */
+public class NvdApiProcessorTest extends BaseTest {
+
+    @Test
+    public void doesNotExistFile() throws Exception {
+        try (CveDB cve = new CveDB(getSettings())) {
+            File file = new File("does_not_exist");
+            NvdApiProcessor processor = new NvdApiProcessor(null, file);
+            assertThrows(NoSuchFileException.class, processor::call);
+        }
+    }
+
+    @Test
+    public void unspecifiedFileName() throws Exception {
+        try (CveDB cve = new CveDB(getSettings())) {
+            File file = File.createTempFile("test", "test");
+            writeFileString(file, "");
+            NvdApiProcessor processor = new NvdApiProcessor(null, file);
+            processor.call();
+        }
+    }
+
+    @Test
+    public void invalidFileContent() throws Exception {
+        try (CveDB cve = new CveDB(getSettings())) {
+            File file = File.createTempFile("test", "test.json");
+            // invalid content (broken array)
+            writeFileString(file, "[}");
+            NvdApiProcessor processor = new NvdApiProcessor(null, file);
+            assertThrows(JsonParseException.class, processor::call);
+        }
+    }
+
+    @Test
+    public void processValidStructure() throws Exception {
+        try (CveDB cve = new CveDB(getSettings())) {
+            File file = File.createTempFile("test", "test.json");
+            writeFileString(file, "[]");
+            NvdApiProcessor processor = new NvdApiProcessor(null, file);
+            processor.call();
+        }
+    }
+
+    static void writeFileString(File file, String content) throws IOException {
+        try (FileWriter writer = new FileWriter(file, false)) {
+            writer.write(content);
+            writer.flush();
+        }
+    }
+}