From cd4f09dc86adfdbf6c19becc2af3dbe116cc568d Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Sat, 11 Feb 2017 20:16:24 +0100 Subject: [PATCH 1/6] NvdCveUpdater: Refactor thread pool concept - Make thread pools members of the class to facilitate reuse - Increase default max download thread pool size from 3 to 50 (should be fine for mostly blocking tasks like downloading) --- .../data/update/NvdCveUpdater.java | 57 ++++++++++++------- .../main/resources/dependencycheck.properties | 2 +- .../test/resources/dependencycheck.properties | 2 +- .../test/resources/dependencycheck.properties | 2 +- 4 files changed, 38 insertions(+), 25 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java index bcf19a34b..93cdbcf1e 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java @@ -54,9 +54,21 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { */ private static final Logger LOGGER = LoggerFactory.getLogger(NvdCveUpdater.class); /** - * The max thread pool size to use when downloading files. + * The thread pool size to use for CPU-intense tasks. */ - public static final int MAX_THREAD_POOL_SIZE = Settings.getInt(Settings.KEYS.MAX_DOWNLOAD_THREAD_POOL_SIZE, 3); + private static final int PROCESSING_THREAD_POOL_SIZE = 1; + /** + * The thread pool size to use when downloading files. + */ + private static final int DOWNLOAD_THREAD_POOL_SIZE = Settings.getInt(Settings.KEYS.MAX_DOWNLOAD_THREAD_POOL_SIZE, 50); + /** + * ExecutorService for CPU-intense processing tasks. + */ + private ExecutorService processingExecutorService = null; + /** + * ExecutorService for tasks that involve blocking activities and are not very CPU-intense, e.g. downloading files. + */ + private ExecutorService downloadExecutorService = null; /** * Downloads the latest NVD CVE XML file from the web and imports it into @@ -72,10 +84,11 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { return; } } catch (InvalidSettingException ex) { - LOGGER.trace("inavlid setting UPDATE_NVDCVE_ENABLED", ex); + LOGGER.trace("invalid setting UPDATE_NVDCVE_ENABLED", ex); } try { + initializeExecutorServices(); openDataStores(); boolean autoUpdate = true; try { @@ -101,10 +114,27 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { } throw new UpdateException("Unable to download the NVD CVE data.", ex); } finally { + shutdownExecutorServices(); closeDataStores(); } } + private void initializeExecutorServices() { + processingExecutorService = Executors.newFixedThreadPool(PROCESSING_THREAD_POOL_SIZE); + downloadExecutorService = Executors.newFixedThreadPool(DOWNLOAD_THREAD_POOL_SIZE); + LOGGER.debug("#download threads: {}", DOWNLOAD_THREAD_POOL_SIZE); + LOGGER.debug("#processing threads: {}", PROCESSING_THREAD_POOL_SIZE); + } + + private void shutdownExecutorServices() { + if (processingExecutorService != null) { + processingExecutorService.shutdownNow(); + } + if (downloadExecutorService != null) { + downloadExecutorService.shutdownNow(); + } + } + /** * Checks if the NVD CVE XML files were last checked recently. As an * optimization, we can avoid repetitive checks against the NVD. Setting @@ -178,18 +208,13 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { LOGGER.info("NVD CVE requires several updates; this could take a couple of minutes."); } - final int poolSize = (MAX_THREAD_POOL_SIZE < maxUpdates) ? MAX_THREAD_POOL_SIZE : maxUpdates; - - final ExecutorService downloadExecutors = Executors.newFixedThreadPool(poolSize); - final ExecutorService processExecutor = Executors.newSingleThreadExecutor(); final Set>> downloadFutures = new HashSet>>(maxUpdates); for (NvdCveInfo cve : updateable) { if (cve.getNeedsUpdate()) { - final DownloadTask call = new DownloadTask(cve, processExecutor, getCveDB(), Settings.getInstance()); - downloadFutures.add(downloadExecutors.submit(call)); + final DownloadTask call = new DownloadTask(cve, processingExecutorService, getCveDB(), Settings.getInstance()); + downloadFutures.add(downloadExecutorService.submit(call)); } } - downloadExecutors.shutdown(); //next, move the future future processTasks to just future processTasks final Set> processFutures = new HashSet>(maxUpdates); @@ -198,21 +223,13 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { try { task = future.get(); } catch (InterruptedException ex) { - downloadExecutors.shutdownNow(); - processExecutor.shutdownNow(); - LOGGER.debug("Thread was interrupted during download", ex); throw new UpdateException("The download was interrupted", ex); } catch (ExecutionException ex) { - downloadExecutors.shutdownNow(); - processExecutor.shutdownNow(); - LOGGER.debug("Thread was interrupted during download execution", ex); throw new UpdateException("The execution of the download was interrupted", ex); } if (task == null) { - downloadExecutors.shutdownNow(); - processExecutor.shutdownNow(); LOGGER.debug("Thread was interrupted during download"); throw new UpdateException("The download was interrupted; unable to complete the update"); } else { @@ -227,15 +244,11 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { throw task.getException(); } } catch (InterruptedException ex) { - processExecutor.shutdownNow(); LOGGER.debug("Thread was interrupted during processing", ex); throw new UpdateException(ex); } catch (ExecutionException ex) { - processExecutor.shutdownNow(); LOGGER.debug("Execution Exception during process", ex); throw new UpdateException(ex); - } finally { - processExecutor.shutdown(); } } diff --git a/dependency-check-core/src/main/resources/dependencycheck.properties b/dependency-check-core/src/main/resources/dependencycheck.properties index 21db405f5..d4b6b19e1 100644 --- a/dependency-check-core/src/main/resources/dependencycheck.properties +++ b/dependency-check-core/src/main/resources/dependencycheck.properties @@ -1,7 +1,7 @@ application.name=${pom.name} application.version=${pom.version} autoupdate=true -max.download.threads=3 +max.download.threads=50 # the url to obtain the current engine version from engine.version.url=https://jeremylong.github.io/DependencyCheck/current.txt diff --git a/dependency-check-core/src/test/resources/dependencycheck.properties b/dependency-check-core/src/test/resources/dependencycheck.properties index 138bb587c..277b1e6ad 100644 --- a/dependency-check-core/src/test/resources/dependencycheck.properties +++ b/dependency-check-core/src/test/resources/dependencycheck.properties @@ -1,7 +1,7 @@ application.name=${pom.name} application.version=${pom.version} autoupdate=true -max.download.threads=3 +max.download.threads=50 # the url to obtain the current engine version from engine.version.url=https://jeremylong.github.io/DependencyCheck/current.txt diff --git a/dependency-check-utils/src/test/resources/dependencycheck.properties b/dependency-check-utils/src/test/resources/dependencycheck.properties index f580b54e8..69b649e92 100644 --- a/dependency-check-utils/src/test/resources/dependencycheck.properties +++ b/dependency-check-utils/src/test/resources/dependencycheck.properties @@ -1,7 +1,7 @@ application.name=${pom.name} application.version=${pom.version} autoupdate=true -max.download.threads=3 +max.download.threads=50 # the url to obtain the current engine version from engine.version.url=http://jeremylong.github.io/DependencyCheck/current.txt From a9fc6bf02c3fa040eade89da659468f5d1aec754 Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Thu, 16 Feb 2017 07:15:39 +0100 Subject: [PATCH 2/6] cleanup: remove unused stuff --- .../data/update/nvd/UpdateableNvdCve.java | 19 ------------------- .../data/update/nvd/UpdateableNvdCveTest.java | 18 +----------------- 2 files changed, 1 insertion(+), 36 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/nvd/UpdateableNvdCve.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/nvd/UpdateableNvdCve.java index 6df4e5fa6..08327eeac 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/nvd/UpdateableNvdCve.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/nvd/UpdateableNvdCve.java @@ -25,8 +25,6 @@ import java.util.Map.Entry; import java.util.TreeMap; import org.owasp.dependencycheck.utils.DownloadFailedException; import org.owasp.dependencycheck.utils.Downloader; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** * Contains a collection of updateable NvdCveInfo objects. This is used to determine which files need to be downloaded and @@ -36,10 +34,6 @@ import org.slf4j.LoggerFactory; */ public class UpdateableNvdCve implements Iterable, Iterator { - /** - * A reference to the logger. - */ - private static final Logger LOGGER = LoggerFactory.getLogger(UpdateableNvdCve.class); /** * A collection of sources of data. */ @@ -68,19 +62,6 @@ public class UpdateableNvdCve implements Iterable, Iterator Date: Thu, 16 Feb 2017 07:48:06 +0100 Subject: [PATCH 3/6] Refactoring: Move retrieval of last modified timestamps from UpdateableNvdCve to NvdCveUpdater - UpdateableNvdCve is from its nature more like a simple value object - Facilitates performance optimization for retrieval of last modification timestamps --- .../data/update/NvdCveUpdater.java | 55 ++++++++++++++++--- .../data/update/nvd/UpdateableNvdCve.java | 12 +--- .../data/update/nvd/UpdateableNvdCveTest.java | 36 +++++------- 3 files changed, 66 insertions(+), 37 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java index 93cdbcf1e..c3263bc1e 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java @@ -19,8 +19,11 @@ package org.owasp.dependencycheck.data.update; import java.net.MalformedURLException; import java.util.Calendar; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Set; +import java.net.URL; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -36,6 +39,7 @@ import org.owasp.dependencycheck.data.update.nvd.NvdCveInfo; import org.owasp.dependencycheck.data.update.nvd.ProcessTask; import org.owasp.dependencycheck.data.update.nvd.UpdateableNvdCve; import org.owasp.dependencycheck.utils.DateUtil; +import org.owasp.dependencycheck.utils.Downloader; import org.owasp.dependencycheck.utils.DownloadFailedException; import org.owasp.dependencycheck.utils.InvalidSettingException; import org.owasp.dependencycheck.utils.Settings; @@ -357,20 +361,57 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { private UpdateableNvdCve retrieveCurrentTimestampsFromWeb() throws MalformedURLException, DownloadFailedException, InvalidDataException, InvalidSettingException { - final UpdateableNvdCve updates = new UpdateableNvdCve(); - updates.add(MODIFIED, Settings.getString(Settings.KEYS.CVE_MODIFIED_20_URL), - Settings.getString(Settings.KEYS.CVE_MODIFIED_12_URL), - false); final int start = Settings.getInt(Settings.KEYS.CVE_START_YEAR); final int end = Calendar.getInstance().get(Calendar.YEAR); + + final Map lastModifiedDates = retrieveLastModifiedDates(start, end); + + final UpdateableNvdCve updates = new UpdateableNvdCve(); + final String baseUrl20 = Settings.getString(Settings.KEYS.CVE_SCHEMA_2_0); final String baseUrl12 = Settings.getString(Settings.KEYS.CVE_SCHEMA_1_2); for (int i = start; i <= end; i++) { - updates.add(Integer.toString(i), String.format(baseUrl20, i), - String.format(baseUrl12, i), - true); + final String url = String.format(baseUrl20, i); + updates.add(Integer.toString(i), url, String.format(baseUrl12, i), + lastModifiedDates.get(url), true); } + + final String url = Settings.getString(Settings.KEYS.CVE_MODIFIED_20_URL); + updates.add(MODIFIED, url, Settings.getString(Settings.KEYS.CVE_MODIFIED_12_URL), + lastModifiedDates.get(url), false); + return updates; } + + /** + * Retrieves the timestamps from the NVD CVE meta data file. + * + * @param startYear the first year whose item to check for the timestamp + * @param endYear the last year whose item to check for the timestamp + * @return the timestamps from the currently published nvdcve downloads page + * @throws MalformedURLException thrown if the URL for the NVD CCE Meta data + * is incorrect. + * @throws DownloadFailedException thrown if there is an error downloading + * the nvd cve meta data file + */ + private Map retrieveLastModifiedDates(int startYear, int endYear) + throws MalformedURLException, DownloadFailedException { + + final Set urls = new HashSet(); + final String baseUrl20 = Settings.getString(Settings.KEYS.CVE_SCHEMA_2_0); + for (int i = startYear; i <= endYear; i++) { + final String url = String.format(baseUrl20, i); + urls.add(url); + } + urls.add(Settings.getString(Settings.KEYS.CVE_MODIFIED_20_URL)); + + final Map lastModifiedDates = new HashMap(); + for(String url: urls) { + LOGGER.debug("Checking for updates from: {}", url); + lastModifiedDates.put(url, Downloader.getLastModified(new URL(url))); + } + + return lastModifiedDates; + } } diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/nvd/UpdateableNvdCve.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/nvd/UpdateableNvdCve.java index 08327eeac..4287bba4d 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/nvd/UpdateableNvdCve.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/nvd/UpdateableNvdCve.java @@ -17,14 +17,10 @@ */ package org.owasp.dependencycheck.data.update.nvd; -import java.net.MalformedURLException; -import java.net.URL; import java.util.Iterator; import java.util.Map; import java.util.Map.Entry; import java.util.TreeMap; -import org.owasp.dependencycheck.utils.DownloadFailedException; -import org.owasp.dependencycheck.utils.Downloader; /** * Contains a collection of updateable NvdCveInfo objects. This is used to determine which files need to be downloaded and @@ -68,18 +64,16 @@ public class UpdateableNvdCve implements Iterable, Iterator Date: Thu, 16 Feb 2017 08:50:06 +0100 Subject: [PATCH 4/6] Parallelize retrieval of last modification timestamps --- .../data/update/NvdCveUpdater.java | 44 +++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java index c3263bc1e..46affc263 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java @@ -24,10 +24,12 @@ import java.util.HashSet; import java.util.Map; import java.util.Set; import java.net.URL; +import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import org.owasp.dependencycheck.data.nvdcve.CveDB; import org.owasp.dependencycheck.data.nvdcve.DatabaseException; import org.owasp.dependencycheck.data.nvdcve.DatabaseProperties; @@ -406,12 +408,48 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { } urls.add(Settings.getString(Settings.KEYS.CVE_MODIFIED_20_URL)); + final Map> timestampFutures = new HashMap>(); + for (String url : urls) { + final TimestampRetriever timestampRetriever = new TimestampRetriever(url); + final Future future = downloadExecutorService.submit(timestampRetriever); + timestampFutures.put(url, future); + } + final Map lastModifiedDates = new HashMap(); - for(String url: urls) { - LOGGER.debug("Checking for updates from: {}", url); - lastModifiedDates.put(url, Downloader.getLastModified(new URL(url))); + for (String url : urls) { + final Future timestampFuture = timestampFutures.get(url); + final long timestamp; + try { + timestamp = timestampFuture.get(60, TimeUnit.SECONDS); + } catch (Exception e) { + throw new DownloadFailedException(e); + } + lastModifiedDates.put(url, timestamp); } return lastModifiedDates; } + + /** + * Retrieves the last modified timestamp from a NVD CVE meta data file. + */ + private static class TimestampRetriever implements Callable { + + private String url; + + TimestampRetriever(String url) { + this.url = url; + } + + @Override + public Long call() throws Exception { + LOGGER.debug("Checking for updates from: {}", url); + try { + Settings.initialize(); + return Downloader.getLastModified(new URL(url)); + } finally { + Settings.cleanup(false); + } + } + } } From eca0e7a852f5a2efae8ab41362a775015b52335b Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Thu, 16 Feb 2017 20:53:48 +0100 Subject: [PATCH 5/6] Fix integration test --- .../org/owasp/dependencycheck/data/update/NvdCveUpdater.java | 2 +- .../data/update/NvdCveUpdaterIntegrationTest.java | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java index 46affc263..dd22e2fa8 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java @@ -125,7 +125,7 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { } } - private void initializeExecutorServices() { + void initializeExecutorServices() { processingExecutorService = Executors.newFixedThreadPool(PROCESSING_THREAD_POOL_SIZE); downloadExecutorService = Executors.newFixedThreadPool(DOWNLOAD_THREAD_POOL_SIZE); LOGGER.debug("#download threads: {}", DOWNLOAD_THREAD_POOL_SIZE); diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/update/NvdCveUpdaterIntegrationTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/update/NvdCveUpdaterIntegrationTest.java index d666c9772..526ae1580 100644 --- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/update/NvdCveUpdaterIntegrationTest.java +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/update/NvdCveUpdaterIntegrationTest.java @@ -33,6 +33,7 @@ public class NvdCveUpdaterIntegrationTest extends BaseTest { public NvdCveUpdater getUpdater() throws MalformedURLException, DownloadFailedException, UpdateException { NvdCveUpdater instance = new NvdCveUpdater(); + instance.initializeExecutorServices(); return instance; } From 59401cc9f8e277c668affa3401e5cefb409ec71b Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Thu, 16 Feb 2017 20:55:26 +0100 Subject: [PATCH 6/6] cleanup/code style --- .../data/update/NvdCveUpdater.java | 7 ++++--- .../data/update/NvdCveUpdaterIntegrationTest.java | 15 ++++++--------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java index dd22e2fa8..a942f98a3 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/update/NvdCveUpdater.java @@ -225,7 +225,7 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { //next, move the future future processTasks to just future processTasks final Set> processFutures = new HashSet>(maxUpdates); for (Future> future : downloadFutures) { - Future task = null; + Future task; try { task = future.get(); } catch (InterruptedException ex) { @@ -280,8 +280,9 @@ public class NvdCveUpdater extends BaseUpdater implements CachedWebDataSource { * @throws UpdateException Is thrown if there is an issue with the last * updated properties file */ - protected final UpdateableNvdCve getUpdatesNeeded() throws MalformedURLException, DownloadFailedException, UpdateException { - UpdateableNvdCve updates = null; + final UpdateableNvdCve getUpdatesNeeded() throws MalformedURLException, DownloadFailedException, UpdateException { + LOGGER.info("starting getUpdatesNeeded() ..."); + UpdateableNvdCve updates; try { updates = retrieveCurrentTimestampsFromWeb(); } catch (InvalidDataException ex) { diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/update/NvdCveUpdaterIntegrationTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/update/NvdCveUpdaterIntegrationTest.java index 526ae1580..94b115d90 100644 --- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/update/NvdCveUpdaterIntegrationTest.java +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/update/NvdCveUpdaterIntegrationTest.java @@ -17,13 +17,10 @@ */ package org.owasp.dependencycheck.data.update; -import java.net.MalformedURLException; import static org.junit.Assert.assertNotNull; import org.junit.Test; import org.owasp.dependencycheck.BaseTest; -import org.owasp.dependencycheck.data.update.exception.UpdateException; import org.owasp.dependencycheck.data.update.nvd.UpdateableNvdCve; -import org.owasp.dependencycheck.utils.DownloadFailedException; /** * @@ -31,23 +28,23 @@ import org.owasp.dependencycheck.utils.DownloadFailedException; */ public class NvdCveUpdaterIntegrationTest extends BaseTest { - public NvdCveUpdater getUpdater() throws MalformedURLException, DownloadFailedException, UpdateException { + public NvdCveUpdater getUpdater() { NvdCveUpdater instance = new NvdCveUpdater(); instance.initializeExecutorServices(); return instance; } -// test removed as it is duplicative of the EngineIntegrationTest and the NvdCveUpdaterIntergraionTest -// /** -// * Test of update method, of class StandardUpdate. -// */ + /** + * Test of update method. + */ @Test public void testUpdate() throws Exception { NvdCveUpdater instance = getUpdater(); instance.update(); } + /** - * Test of updatesNeeded method, of class StandardUpdate. + * Test of updatesNeeded method. */ @Test public void testUpdatesNeeded() throws Exception {