[JENKINS-39547] - Corrupt jar cache

src/main/java/hudson/remoting/Checksum.java

@@ -9,8 +9,6 @@
 import java.security.DigestOutputStream;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
 
 /**
  * Represents 128bit checksum of a jar file.
@@ -74,42 +72,10 @@ static Checksum forFile(File file) throws IOException {
      * Returns the checksum for the given URL.
      */
     static Checksum forURL(URL url) throws IOException {
-        if (CHECKSUMS_BY_URL.containsKey(url)) {
-            return CHECKSUMS_BY_URL.get(url);
-        }
-
-        return calculateFor(url);
-    }
-
-    /**
-     * Allow calculating checksums only one at a time.
-     *
-     * <p>This method caches calculated checksums so future calls to
-     * {@link #forURL)} should not need to re-calculate the value.
-     *
-     * <p>Even if many slaves connect at around the same time, the checksums
-     * should only be calculated once. Making this method synchronized ensures
-     * this behavior.
-     *
-     * <p>Previously when a large number of slaves connected at the same time
-     * the master would experience a spike in CPU and probably I/O. By caching
-     * the results and synchronizing the calculation of the results this issue
-     * is addressed.
-     */
-    private synchronized static Checksum calculateFor(URL url) throws IOException {
-        // When callers all request the checksum of a large jar the calls to
-        // forURL will all fall through to this method since the first caller's
-        // calculation may take a while. Hence re-check the cache at the start.
-        if (CHECKSUMS_BY_URL.containsKey(url)) {
-            return CHECKSUMS_BY_URL.get(url);
-        }
-
         try {
             MessageDigest md = MessageDigest.getInstance(JarLoaderImpl.DIGEST_ALGORITHM);
             Util.copy(url.openStream(), new DigestOutputStream(new NullOutputStream(), md));
-            Checksum checksum =  new Checksum(md.digest(), md.getDigestLength() / 8);
-            CHECKSUMS_BY_URL.putIfAbsent(url, checksum);
-            return checksum;
+            return new Checksum(md.digest(), md.getDigestLength() / 8);
         } catch (NoSuchAlgorithmException e) {
             throw new AssertionError(e);
         }
@@ -128,8 +94,4 @@ public void write(byte[] b) {
         public void write(byte[] b, int off, int len) {
         }
     }
-
-    @edu.umd.cs.findbugs.annotations.SuppressWarnings("DMI_COLLECTION_OF_URLS")
-    private static final ConcurrentMap<URL,Checksum> CHECKSUMS_BY_URL =
-        new ConcurrentHashMap<URL,Checksum>();
 }

src/main/java/hudson/remoting/FileSystemJarCache.java

@@ -1,12 +1,15 @@
 package hudson.remoting;
 
 import javax.annotation.Nonnull;
+import javax.annotation.concurrent.GuardedBy;
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.net.URL;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Map;
 import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -27,6 +30,12 @@ public class FileSystemJarCache extends JarCacheSupport {
      */
     private final Set<Checksum> notified = Collections.synchronizedSet(new HashSet<Checksum>());
 
+    /**
+     * Cache of computer checksums for cached jars.
+     */
+    @GuardedBy("itself")
+    private final Map<String, Checksum> checksumsByPath = new HashMap<>();
+
     /**
      * @param touch
      *      True to touch the cached jar file that's used. This enables external LRU based cache
@@ -60,13 +69,24 @@ protected URL lookInCache(Channel channel, long sum1, long sum2) throws IOExcept
 
     @Override
     protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException, InterruptedException {
+        Checksum expected = new Checksum(sum1, sum2);
         File target = map(sum1, sum2);
 
         if (target.exists()) {
-            // Assume its already been fetched correctly before. ie. We are not going to validate
-            // the checksum.
-            LOGGER.fine(String.format("Jar file already exists: %16X%16X", sum1, sum2));
-            return target.toURI().toURL();
+            Checksum actual = fileChecksum(target);
+            if (expected.equals(actual)) {
+                LOGGER.fine(String.format("Jar file already exists: %s", expected));
+                return target.toURI().toURL();
+            }
+
+            LOGGER.warning(String.format(
+                    "Cached file checksum mismatch: %s%nExpected: %s%n Actual: %s",
+                    target.getAbsolutePath(), expected, actual
+            ));
+            target.delete();
+            synchronized (checksumsByPath) {
+                checksumsByPath.remove(target.getCanonicalPath());
+            }
         }
 
         try {
@@ -81,7 +101,6 @@ protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException
                 }
 
                 // Verify the checksum of the download.
-                Checksum expected = new Checksum(sum1, sum2);
                 Checksum actual = Checksum.forFile(tmp);
                 if (!expected.equals(actual)) {
                     throw new IOException(String.format(
@@ -93,21 +112,19 @@ protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException
                     if (!target.exists()) {
                         throw new IOException("Unable to create " + target + " from " + tmp);
                     }
-
                     // Even if we fail to rename, we are OK as long as the target actually exists at
                     // this point. This can happen if two FileSystemJarCache instances share the
                     // same cache dir.
                     //
                     // Verify the checksum to be sure the target is correct.
-                    actual = Checksum.forFile(target);
+                    actual = fileChecksum(target);
                     if (!expected.equals(actual)) {
                         throw new IOException(String.format(
                                 "Incorrect checksum of previous jar: %s%nExpected: %s%nActual: %s",
                                 target.getAbsolutePath(), expected, actual));
                     }
                 }
 
-
                 return target.toURI().toURL();
             } finally {
                 tmp.delete();
@@ -117,6 +134,25 @@ protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException
         }
     }
 
+    /**
+     * Get file checksum calculating it or retrieving from cache.
+     */
+    private Checksum fileChecksum(File file) throws IOException {
+        String location = file.getCanonicalPath();
+
+        // When callers all request the checksum of a large jar, the first thread
+        // will calculate the checksum and the other treads will be blocked here
+        // until calculated to be picked up from cache right away.
+        synchronized (checksumsByPath) {
+            Checksum checksum = checksumsByPath.get(location);
+            if (checksum != null) return checksum;
+
+            checksum = Checksum.forFile(file);
+            checksumsByPath.put(location, checksum);
+            return checksum;
+        }
+    }
+
     /*package for testing*/ File createTempJar(@Nonnull File target) throws IOException {
         File parent = target.getParentFile();
         Util.mkdirs(parent);

src/main/java/hudson/remoting/JarLoaderImpl.java

@@ -18,10 +18,10 @@
  */
 @edu.umd.cs.findbugs.annotations.SuppressWarnings("SE_BAD_FIELD")
 class JarLoaderImpl implements JarLoader, Serializable {
-    private final ConcurrentMap<Checksum,URL> knownJars = new ConcurrentHashMap<Checksum,URL>();
+    private final ConcurrentMap<Checksum,URL> knownJars = new ConcurrentHashMap<>();
 
     @edu.umd.cs.findbugs.annotations.SuppressWarnings("DMI_COLLECTION_OF_URLS") // TODO: fix this
-    private final ConcurrentMap<URL,Checksum> checksums = new ConcurrentHashMap<URL,Checksum>();
+    private final ConcurrentMap<URL,Checksum> checksums = new ConcurrentHashMap<>();
 
     private final Set<Checksum> presentOnRemote = Collections.synchronizedSet(new HashSet<Checksum>());
 

src/test/java/hudson/remoting/ChecksumTest.java

@@ -45,17 +45,6 @@ public void testForFileAndURL() throws Exception {
         assertNotEquals(Checksum.forFile(tmpFile1), Checksum.forFile(tmpFile2));
     }
 
-    @Test
-    public void testCaching() throws Exception {
-        File tmpFile = createTmpFile("file.txt", FILE_CONTENTS1);
-        HashCode hash = Files.hash(tmpFile, Hashing.sha256());
-        assertEquals(createdExpectedChecksum(hash), Checksum.forFile(tmpFile));
-
-        tmpFile.delete();
-        assertFalse(tmpFile.exists());
-        assertEquals(createdExpectedChecksum(hash), Checksum.forFile(tmpFile));
-    }
-
     private File createTmpFile(String name, String contents) throws Exception {
         File tmpFile = tmp.newFile(name);
         Files.append(contents, tmpFile, Charsets.UTF_8);

src/test/java/hudson/remoting/FileSystemJarCacheTest.java

@@ -9,15 +9,16 @@
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
+import org.jvnet.hudson.test.Bug;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
 
 import java.io.File;
+import java.io.FileWriter;
 import java.io.IOException;
 import java.net.URL;
-import java.nio.charset.Charset;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -61,26 +62,23 @@ public void testRetrieveAlreadyExists() throws Exception {
         File expectedFile = fileSystemJarCache.map(expectedChecksum.sum1, expectedChecksum.sum2);
         expectedFile.getParentFile().mkdirs();
         assertTrue(expectedFile.createNewFile());
+        writeToFile(expectedFile, CONTENTS);
 
         URL url = fileSystemJarCache.retrieve(
                 mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
         assertEquals(expectedFile.toURI().toURL(), url);
+
+        // Changing the content after successfully cached is not an expected use-case.
+        // Here used to verity checksums are cached.
+        writeToFile(expectedFile, "Something else");
+        url = fileSystemJarCache.retrieve(
+                mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
+        assertEquals(expectedFile.toURI().toURL(), url);
     }
 
     @Test
     public void testSuccessfulRetrieve() throws Exception {
-        when(mockChannel.getProperty(JarLoader.THEIRS)).thenReturn(mockJarLoader);
-        doAnswer(new Answer<Void>() {
-            @Override
-            public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
-                RemoteOutputStream o = (RemoteOutputStream) invocationOnMock.getArguments()[2];
-                o.write(CONTENTS.getBytes(Charsets.UTF_8));
-                return null;
-            }
-        }).when(mockJarLoader).writeJarTo(
-                eq(expectedChecksum.sum1),
-                eq(expectedChecksum.sum2),
-                any(RemoteOutputStream.class));
+        mockCorrectLoad();
 
         URL url = fileSystemJarCache.retrieve(
                 mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
@@ -109,25 +107,38 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
                 mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
     }
 
+    @Test
+    @Bug(39547)
+    public void retrieveInvalidChecksum() throws Exception {
+        when(mockChannel.getProperty(JarLoader.THEIRS)).thenReturn(mockJarLoader);
+
+        File expected = fileSystemJarCache.map(expectedChecksum.sum1, expectedChecksum.sum2);
+        writeToFile(expected, "This is no going to match the checksum");
+
+        mockCorrectLoad();
+
+        URL url = fileSystemJarCache.retrieve(mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
+        assertEquals(expectedChecksum, Checksum.forURL(url));
+    }
+
+    private void writeToFile(File expected, String content) throws IOException {
+        expected.getParentFile().mkdirs();
+        FileWriter fileWriter = new FileWriter(expected);
+        try {
+            fileWriter.write(content);
+        } finally {
+            fileWriter.close();
+        }
+    }
+
     @Test
     public void testRenameFailsAndNoTarget() throws Exception {
         File expectedFile = fileSystemJarCache.map(expectedChecksum.sum1, expectedChecksum.sum2);
         File spy = spy(tmp.newFile());
         FileSystemJarCache jarCache = spy(fileSystemJarCache);
         doReturn(spy).when(jarCache).createTempJar(any(File.class));
 
-        when(mockChannel.getProperty(JarLoader.THEIRS)).thenReturn(mockJarLoader);
-        doAnswer(new Answer<Void>() {
-            @Override
-            public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
-                RemoteOutputStream o = (RemoteOutputStream) invocationOnMock.getArguments()[2];
-                o.write(CONTENTS.getBytes(Charsets.UTF_8));
-                return null;
-            }
-        }).when(mockJarLoader).writeJarTo(
-                eq(expectedChecksum.sum1),
-                eq(expectedChecksum.sum2),
-                any(RemoteOutputStream.class));
+        mockCorrectLoad();
 
         when(spy.renameTo(expectedFile)).thenReturn(false);
         assertFalse(expectedFile.exists());
@@ -145,18 +156,7 @@ public void testRenameFailsAndBadPreviousTarget() throws Exception {
         FileSystemJarCache jarCache = spy(fileSystemJarCache);
         doReturn(fileSpy).when(jarCache).createTempJar(any(File.class));
 
-        when(mockChannel.getProperty(JarLoader.THEIRS)).thenReturn(mockJarLoader);
-        doAnswer(new Answer<Void>() {
-            @Override
-            public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
-                RemoteOutputStream o = (RemoteOutputStream) invocationOnMock.getArguments()[2];
-                o.write(CONTENTS.getBytes(Charsets.UTF_8));
-                return null;
-            }
-        }).when(mockJarLoader).writeJarTo(
-                eq(expectedChecksum.sum1),
-                eq(expectedChecksum.sum2),
-                any(RemoteOutputStream.class));
+        mockCorrectLoad();
         doAnswer(new Answer<Boolean>() {
             @Override
             public Boolean answer(InvocationOnMock invocationOnMock) throws Throwable {
@@ -173,4 +173,19 @@ public Boolean answer(InvocationOnMock invocationOnMock) throws Throwable {
 
         jarCache.retrieve(mockChannel, expectedChecksum.sum1, expectedChecksum.sum2);
     }
+
+    private void mockCorrectLoad() throws IOException, InterruptedException {
+        when(mockChannel.getProperty(JarLoader.THEIRS)).thenReturn(mockJarLoader);
+        doAnswer(new Answer<Void>() {
+            @Override
+            public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
+                RemoteOutputStream o = (RemoteOutputStream) invocationOnMock.getArguments()[2];
+                o.write(CONTENTS.getBytes(Charsets.UTF_8));
+                return null;
+            }
+        }).when(mockJarLoader).writeJarTo(
+                eq(expectedChecksum.sum1),
+                eq(expectedChecksum.sum2),
+                any(RemoteOutputStream.class));
+    }
 }