[JENKINS-39547] - Corrupt jar cache #130

olivergondza · 2016-11-07T12:52:19Z

Verify checksum after loaded from slave jar cache on agent side.
Extract checksum caching from Checksum class to JarLoaderImpl and FileSystemJarCache.
- It was done twice on master side and made the system more fragile on both ends.
- I wonder if we need checksum cashing on agent side at all as we should rarely call #retrieve on same jar more than once between agent restarts. Getting rid of it would improve this fix as it would detect jar cache manipulation without restart.

We need a way to calculate reliable checksums and as implemented it causes temporary write-then-move files to have checksums cached never to use used.

This moves checksum caching to FileSystemJarCache.

oleg-nenashev

IMHO concurrency management is broken in this PR. I would recommend to just introduce a sync object and to synchronize all methods properly against it. ConcurrentMap is not reliable for get-get operation-

oleg-nenashev · 2016-11-07T14:55:45Z

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

@@ -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 =


So this change completely deletes the checksum cache

Yes, I do not think it was a good idea to implement it in Checksum class in the first place.

oleg-nenashev · 2016-11-09T15:30:25Z

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

+        // 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.
+        String location = file.getCanonicalPath();
+        if (checksumsByPath.containsKey(location)) {


🐛 checksumsByPath is not correctly synchronized here. contains() and get() go as separate calls, and the item may be removed between these two calls

oleg-nenashev · 2016-11-09T15:33:56Z

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

+        synchronized (this) {
+            cs = Checksum.forFile(file);
+        }
+        checksumsByPath.put(location, cs);


Again, in the case of unlikely parallel download of different JARs, we may put wrong checksum here.

Download A

Thread 1. Thread interrupt/scheduling

Thread 1. Calculate checksum

Thread 2. Download B, replace A

Thread 2. Calculate B checksum

Thread 2. Put B checksum to the cache

Thread 2. Thread interrupt/scheduling

Thread 1. Put A checksum to the cache (but the file is B)

oleg-nenashev · 2016-11-09T15:35:15Z

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

@@ -18,10 +18,9 @@
 */
 @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<>();


Modification of the serializable class. Would it cause compatibility issues if we send it over the channel with different remoting versions on sides. I'd guess so. 🐜

Just ignore. 🤦

oleg-nenashev · 2016-11-09T15:35:42Z

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

@@ -110,24 +107,36 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
    }

    @Test
+    public void retrieveInvalidChecksum() throws Exception {


Reference issue?

olivergondza · 2016-11-10T10:47:34Z

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


-    @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<String, Checksum> checksums = new ConcurrentHashMap<>();


@oleg-nenashev, I did not considered this to be remotable as this is master-side abstraction provided the classloading is not by-directional. This does not seem to be generally true, though.

JarLoader is being used as a Channel property, which is accessible on both sides. So it may be really serialized over the channel if the Channel#remoteChannel implementation passes it back as a seruializable result

oleg-nenashev · 2016-11-10T10:57:15Z

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


-    @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<String, Checksum> checksums = new ConcurrentHashMap<>();


JarLoader is being used as a Channel property, which is accessible on both sides. So it may be really serialized over the channel if the Channel#remoteChannel implementation passes it back as a seruializable result

oleg-nenashev · 2016-11-23T23:18:41Z

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

+                return target.toURI().toURL();
+            }
+
+            LOGGER.warning(String.format(


Is it really a warning? I suppose it's a valid behavior when somebody updates jars on the remote side (e.g. plugin update)

It is, when you update a jar, it should have different checksum and therefore be stored on different location so this code path will not be executed. This warning is a signal that what we found cached as ABC (by location) is in fact DEF (by checksum). This can be caused by manipulating the cache dir (what this PR addresses), bug in remoting that cached jar on wrong location or change of checksum algorithm. In all these cases it is undesirable to load the jar blindly.

oleg-nenashev · 2016-11-23T23:23:50Z

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

-            // the checksum.
-            LOGGER.fine(String.format("Jar file already exists: %16X%16X", sum1, sum2));
-            return target.toURI().toURL();
+            Checksum actual = fileChecksum(target);


Improvement follow-up proposal: Perform checksum calculation for JAR cache during the remoting startup (with option to disable it) before establishing the connection. Should help with performance issues during first job execution due to classloading.

oleg-nenashev

Logging level is my last concern. And test failure, but it's unlikely related

oleg-nenashev · 2016-11-23T23:28:58Z

@olivergondza Maybe it makes sense to integrate the patch into stable-2.x once it's ready

olivergondza · 2016-11-24T11:21:36Z

I do not consider it all that critical after we agreed it is not security vulnerability. No hurry as long as I am concerned.

oleg-nenashev · 2016-11-24T16:55:30Z

Retriggering the CI

olivergondza · 2016-11-25T08:23:38Z

Maven invocation on ci.jenkins.io fails badly as maven terminate prematurely there. I am not able to reproduce that locally with:

Apache Maven 3.3.9 (NON-CANONICAL_2015-11-23T13:17:27+03:00_root; 2015-11-23T11:17:27+01:00)
Maven home: /opt/maven
Java version: 1.8.0_112, vendor: Oracle Corporation
Java home: /usr/lib/jvm/java-8-jdk/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "4.4.31-1-lts", arch: "amd64", family: "unix"

Noting #100, #130, #138, #136

olivergondza added 3 commits November 7, 2016 13:46

Do not cache by URL

9a3381a

Do not perform Checksum caching in Checksum class

0089cec

We need a way to calculate reliable checksums and as implemented it causes temporary write-then-move files to have checksums cached never to use used.

[FIXED JENKINS-39547] Verify cached slave jars before using them.

9d7cab8

This moves checksum caching to FileSystemJarCache.

oleg-nenashev changed the title ~~Corrupt jar cache~~ [JENKINS-39547] - Corrupt jar cache Nov 7, 2016

oleg-nenashev added the needs-review label Nov 7, 2016

oleg-nenashev requested changes Nov 9, 2016

View reviewed changes

olivergondza commented Nov 10, 2016

View reviewed changes

oleg-nenashev requested changes Nov 10, 2016

View reviewed changes

Address review comments

faae1b5

oleg-nenashev reviewed Nov 23, 2016

View reviewed changes

oleg-nenashev approved these changes Nov 24, 2016

View reviewed changes

oleg-nenashev closed this Nov 24, 2016

oleg-nenashev reopened this Nov 24, 2016

oleg-nenashev merged commit cdd5bce into jenkinsci:master Dec 16, 2016

oleg-nenashev added a commit that referenced this pull request Dec 16, 2016

Changekig: Add relese notes for 3.3

5c0c81d

Noting #100, #130, #138, #136

oleg-nenashev mentioned this pull request Dec 16, 2016

Update Jenkins remoting to 3.3 jenkinsci/jenkins#2671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-39547] - Corrupt jar cache #130

[JENKINS-39547] - Corrupt jar cache #130

olivergondza commented Nov 7, 2016

oleg-nenashev left a comment

oleg-nenashev Nov 7, 2016

olivergondza Nov 10, 2016

oleg-nenashev Nov 9, 2016

oleg-nenashev Nov 9, 2016

oleg-nenashev Nov 9, 2016

oleg-nenashev Nov 23, 2016

oleg-nenashev Nov 9, 2016

olivergondza Nov 10, 2016

oleg-nenashev Nov 10, 2016

oleg-nenashev Nov 10, 2016

oleg-nenashev Nov 23, 2016

olivergondza Nov 24, 2016

oleg-nenashev Nov 24, 2016

oleg-nenashev Nov 23, 2016

oleg-nenashev left a comment

oleg-nenashev commented Nov 23, 2016

olivergondza commented Nov 24, 2016

oleg-nenashev commented Nov 24, 2016

olivergondza commented Nov 25, 2016

[JENKINS-39547] - Corrupt jar cache #130

[JENKINS-39547] - Corrupt jar cache #130

Conversation

olivergondza commented Nov 7, 2016

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Nov 23, 2016

olivergondza commented Nov 24, 2016

oleg-nenashev commented Nov 24, 2016

olivergondza commented Nov 25, 2016