From 74e35d8b6bd00944ad1e4a2e5caacdd6832219af Mon Sep 17 00:00:00 2001
From: Kohsuke Kawaguchi <kk@kohsuke.org>
Date: Tue, 10 May 2011 18:34:44 -0700
Subject: [PATCH] [FIXED JENKINS-9017] Fixed incorrect de-allocation of a
 classloader from the exported object table.

The fix is a defense-in-depth; it prevents classloaders referenced in
the object graph from doubly released, then we also make it impossible
for bugs like this to deallocate the key classloader.
---
 changelog.html                                |  3 +++
 .../java/hudson/slaves/SlaveComputer.java     |  5 +++++
 .../main/java/hudson/remoting/Channel.java    | 22 ++++++++++++++++---
 .../java/hudson/remoting/ExportTable.java     | 21 +++++++++++++++---
 .../hudson/remoting/RemoteClassLoader.java    | 10 +++++++++
 5 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/changelog.html b/changelog.html
index 86209f4e88..97a79549d2 100644
--- a/changelog.html
+++ b/changelog.html
@@ -55,6 +55,9 @@ Upcoming changes</a>
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
+  <li class=bug>
+    Fixed a <tt>ClassCastException</tt> caused by multiple loading of the same class in different classloaders.
+    (<a href="http://issues.jenkins-ci.org/browse/JENKINS-9017">issue 9017</a>)
   <li class=rfe>
     Gracefully handle old slave.jar to avoid <tt>AbstractMethodError</tt>
     (<a href="https://groups.google.com/d/topic/jenkinsci-dev/KqFw4nfiQdE/discussion">thread</a>)
diff --git a/core/src/main/java/hudson/slaves/SlaveComputer.java b/core/src/main/java/hudson/slaves/SlaveComputer.java
index 477501f773..b3ba1c07cc 100644
--- a/core/src/main/java/hudson/slaves/SlaveComputer.java
+++ b/core/src/main/java/hudson/slaves/SlaveComputer.java
@@ -342,6 +342,11 @@ public class SlaveComputer extends Computer {
             log.println("WARNING: "+remoteFs+" looks suspiciously like Windows path. Maybe you meant "+remoteFs.replace('\\','/')+"?");
         FilePath root = new FilePath(channel,getNode().getRemoteFS());
 
+        // reference counting problem is known to happen, such as JENKINS-9017, and so as a preventive measure
+        // we pin the base classloader so that it'll never get GCed. When this classloader gets released,
+        // it'll have a catastrophic impact on the communication.
+        channel.pinClassLoader(getClass().getClassLoader());
+
         channel.call(new SlaveInitializer());
         channel.call(new WindowsSlaveInstaller(remoteFs));
         for (ComputerListener cl : ComputerListener.all())
diff --git a/remoting/src/main/java/hudson/remoting/Channel.java b/remoting/src/main/java/hudson/remoting/Channel.java
index 94049f82b1..94a449bec9 100644
--- a/remoting/src/main/java/hudson/remoting/Channel.java
+++ b/remoting/src/main/java/hudson/remoting/Channel.java
@@ -526,9 +526,9 @@ public class Channel implements VirtualChannel, IChannel {
                 logger.log(Level.WARNING, "Unable to send GC command",e);
             }
 
-        // proxy will unexport this instance when it's GC-ed on the remote machine.
-        final int id = export(instance);
-        return RemoteInvocationHandler.wrap(null,id,type,userProxy,exportedObjects.isRecording());
+        // proxy will unexport this instance when it's GC-ed on the remote machine, so don't unexport on our side automatically at the end of a call.
+        final int id = export(instance,false);
+        return RemoteInvocationHandler.wrap(null, id, type, userProxy, exportedObjects.isRecording());
     }
 
     /*package*/ int export(Object instance) {
@@ -547,6 +547,22 @@ public class Channel implements VirtualChannel, IChannel {
         exportedObjects.unexportByOid(id);
     }
 
+    /**
+     * Increase reference count so much to effectively prevent de-allocation.
+     *
+     * @see ExportTable.Entry#pin()
+     */
+    public void pin(Object instance) {
+        exportedObjects.pin(instance);
+    }
+
+    /**
+     * {@linkplain #pin(Object) Pin down} the exported classloader.
+     */
+    public void pinClassLoader(ClassLoader cl) {
+        RemoteClassLoader.pin(cl,this);
+    }
+
     /**
      * Preloads jar files on the remote side.
      *
diff --git a/remoting/src/main/java/hudson/remoting/ExportTable.java b/remoting/src/main/java/hudson/remoting/ExportTable.java
index ba844e770f..3bee95282f 100644
--- a/remoting/src/main/java/hudson/remoting/ExportTable.java
+++ b/remoting/src/main/java/hudson/remoting/ExportTable.java
@@ -67,7 +67,6 @@ final class ExportTable<T> {
             // force the computation of the stack trace in a Java friendly data structure,
             // so that the call stack can be seen from the heap dump after the fact. 
             allocationTrace.getStackTrace();
-            addRef();
 
             table.put(id,this);
             reverse.put(object,this);
@@ -77,6 +76,16 @@ final class ExportTable<T> {
             referenceCount++;
         }
 
+        /**
+         * Increase reference count so much to effectively prevent de-allocation.
+         * If the reference counting is correct, we just need to increment by one,
+         * but this makes it safer even in case of some reference counting errors
+         * (and we can still detect the problem by comparing the reference count with the magic value.
+         */
+        void pin() {
+            referenceCount += Integer.MAX_VALUE/2;
+        }
+
         void release() {
             if(--referenceCount==0) {
                 table.remove(id);
@@ -155,8 +164,7 @@ final class ExportTable<T> {
         Entry e = reverse.get(t);
         if(e==null)
             e = new Entry(t);
-        else
-            e.addRef();
+        e.addRef();
 
         if(notifyListener) {
             ExportList l = lists.get();
@@ -166,6 +174,13 @@ final class ExportTable<T> {
         return e.id;
     }
 
+    /*package*/ synchronized void pin(T t) {
+        Entry e = reverse.get(t);
+        if(e==null)
+            e = new Entry(t);
+        e.pin();
+    }
+
     public synchronized T get(int id) {
         Entry e = table.get(id);
         if(e!=null) return e.object;
diff --git a/remoting/src/main/java/hudson/remoting/RemoteClassLoader.java b/remoting/src/main/java/hudson/remoting/RemoteClassLoader.java
index be266fbd0c..d357cfd8db 100644
--- a/remoting/src/main/java/hudson/remoting/RemoteClassLoader.java
+++ b/remoting/src/main/java/hudson/remoting/RemoteClassLoader.java
@@ -352,6 +352,16 @@ final class RemoteClassLoader extends URLClassLoader {
         return local.export(IClassLoader.class, new ClassLoaderProxy(cl,local), false);
     }
 
+    public static void pin(ClassLoader cl, Channel local) {
+        if (cl instanceof RemoteClassLoader) {
+            // check if this is a remote classloader from the channel
+            final RemoteClassLoader rcl = (RemoteClassLoader) cl;
+            int oid = RemoteInvocationHandler.unwrap(rcl.proxy, local);
+            if(oid!=-1) return;
+        }
+        local.pin(new ClassLoaderProxy(cl,local));
+    }
+
     /**
      * Exports and just returns the object ID, instead of obtaining the proxy.
      */
-- 
GitLab