Bug 423715 - Check MD5Verifier's status before using it

Add MD5Verifier to processing steps only if it had been initialized
properly.

During construction, MD5Verifier catches NoSuchAlgorithmException in a
way that requires caller to check the actual status with
getStatus().isOK(). If not properly constructed, MD5Verifier's
write(int) and close() will throw NPE later on.

This change may be considered as an API breaking: before, using
verifier that has not been properly constructed, client will get
NPE. After this change, the actual problem will be hidden as client
will not see such invalid verifier at all.

On the other hand, this should never happen because every JRE must
support MD5 MessageDigest implementation (which is the only
implementation used by p2 as of now).

Change-Id: Ic24f9f6caaa219233715998d74bfc478a5310247
Signed-off-by: Mykola Nikishov <mn@mn.com.ua>

2 files changed, 18 insertions, 11 deletions

diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/RawMirrorRequest.java b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/RawMirrorRequest.java
index de6508d31..924d6e084 100644
--- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/RawMirrorRequest.java
+++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/RawMirrorRequest.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2009, 2017 IBM Corporation and others.
+ * Copyright (c) 2009, 2018 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -79,10 +79,14 @@ public class RawMirrorRequest extends MirrorRequest {
 
 	// Perform the mirror operation without any processing steps
 	@Override
-	protected IStatus getArtifact(IArtifactDescriptor descriptor, OutputStream destination, IProgressMonitor monitor) {
-		ProcessingStepHandler handler = new ProcessingStepHandler();
-		if (SimpleArtifactRepository.DOWNLOAD_MD5_CHECKSUM_ENABLED && descriptor.getProperty(IArtifactDescriptor.DOWNLOAD_MD5) != null)
-			destination = handler.link(new ProcessingStep[] {new MD5Verifier(descriptor.getProperty(IArtifactDescriptor.DOWNLOAD_MD5))}, destination, monitor);
-		return getSourceRepository().getRawArtifact(descriptor, destination, monitor);
+	protected IStatus getArtifact(IArtifactDescriptor artifactDescriptor, OutputStream destination, IProgressMonitor monitor) {
+		if (SimpleArtifactRepository.DOWNLOAD_MD5_CHECKSUM_ENABLED && artifactDescriptor.getProperty(IArtifactDescriptor.DOWNLOAD_MD5) != null) {
+			MD5Verifier checksumVerifier = new MD5Verifier(artifactDescriptor.getProperty(IArtifactDescriptor.DOWNLOAD_MD5));
+			if (checksumVerifier.getStatus().isOK()) {
+				ProcessingStepHandler handler = new ProcessingStepHandler();
+				destination = handler.link(new ProcessingStep[] {checksumVerifier}, destination, monitor);
+			}
+		}
+		return getSourceRepository().getRawArtifact(artifactDescriptor, destination, monitor);
 	}
 }
diff --git a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java
index 69905d5f9..6ee339c8e 100644
--- a/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java
+++ b/bundles/org.eclipse.equinox.p2.artifact.repository/src/org/eclipse/equinox/internal/p2/artifact/repository/simple/SimpleArtifactRepository.java
@@ -459,7 +459,7 @@ public class SimpleArtifactRepository extends AbstractArtifactRepository impleme
 	private synchronized OutputStream addPostSteps(ProcessingStepHandler handler, IArtifactDescriptor descriptor, OutputStream destination, IProgressMonitor monitor) {
 		ArrayList<ProcessingStep> steps = new ArrayList<>();
 		steps.add(new SignatureVerifier());
-		addChecksumVerifiers(steps, ARTIFACT_MD5_CHECKSUM_ENABLED, descriptor.getProperty(IArtifactDescriptor.ARTIFACT_MD5));
+		addChecksumVerifiers(steps, ARTIFACT_MD5_CHECKSUM_ENABLED, descriptor, IArtifactDescriptor.ARTIFACT_MD5);
 
 		if (steps.isEmpty())
 			return destination;
@@ -472,7 +472,7 @@ public class SimpleArtifactRepository extends AbstractArtifactRepository impleme
 		ArrayList<ProcessingStep> steps = new ArrayList<>();
 		if (IArtifactDescriptor.TYPE_ZIP.equals(descriptor.getProperty(IArtifactDescriptor.DOWNLOAD_CONTENTTYPE)))
 			steps.add(new ZipVerifierStep());
-		addChecksumVerifiers(steps, DOWNLOAD_MD5_CHECKSUM_ENABLED, descriptor.getProperty(IArtifactDescriptor.DOWNLOAD_MD5));
+		addChecksumVerifiers(steps, DOWNLOAD_MD5_CHECKSUM_ENABLED, descriptor, IArtifactDescriptor.DOWNLOAD_MD5);
 
 		// Add steps here if needed
 		if (steps.isEmpty())
@@ -485,9 +485,12 @@ public class SimpleArtifactRepository extends AbstractArtifactRepository impleme
 	/**
 	 * Adds checksum verifier to steps only if isChecksumEnabled and checksum is not null
 	 */
-	private void addChecksumVerifiers(ArrayList<ProcessingStep> steps, boolean isChecksumEnabled, String checksum) {
-		if (isChecksumEnabled && checksum != null)
-			steps.add(new MD5Verifier(checksum));
+	private void addChecksumVerifiers(ArrayList<ProcessingStep> steps, boolean isChecksumEnabled, IArtifactDescriptor descriptor, String property) {
+		if (isChecksumEnabled && descriptor.getProperty(property) != null) {
+			MD5Verifier checksumVerifier = new MD5Verifier(descriptor.getProperty(property));
+			if (checksumVerifier.getStatus().isOK())
+				steps.add(checksumVerifier);
+		}
 	}
 
 	private byte[] bytesFromHexString(String string) {