changeset 780:28ac9a016c80

Fix classloading deadlock regression Resolve deadlock issue in JNLPClassLoader. See http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2013-December/025546.html * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: (loadClassLock) removed. (available, jarIndexes, classpaths, jarEntries, jarLocationSecurityMap) fields wrapped in Collections.synchronized*() to provide atomic read/write. Synchronized on while iterating over these collections. (loadClass) no longer uses implicit JNLPClassLoader instance lock nor dedicated loadClassLock object.
author Andrew Azores <aazores@redhat.com>
date Fri, 27 Dec 2013 09:51:24 -0500
parents c7bd4f6cf1b7
children 91e191cac118
files ChangeLog netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
diffstat 2 files changed, 128 insertions(+), 79 deletions(-) [+]
line wrap: on
line diff
--- a/ChangeLog	Fri Dec 20 13:25:38 2013 +0100
+++ b/ChangeLog	Fri Dec 27 09:51:24 2013 -0500
@@ -1,3 +1,13 @@
+2013-12-27  Andrew Azores  <aazores@redhat.com>
+
+	Resolve deadlock issue in JNLPClassLoader. See
+	http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2013-December/025546.html
+	* netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: (loadClassLock)
+	removed. (available, jarIndexes, classpaths, jarEntries, jarLocationSecurityMap)
+	fields wrapped in Collections.synchronized*() to provide atomic read/write.
+	Synchronized on while iterating over these collections. (loadClass) no longer
+	uses implicit JNLPClassLoader instance lock nor dedicated loadClassLock object.
+
 2013-12-20  Jiri Vanek  <jvanek@redhat.com>
 
 	backported singletons logic, logs and test cleanup/fixes
--- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Fri Dec 20 13:25:38 2013 +0100
+++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Fri Dec 27 09:51:24 2013 -0500
@@ -167,26 +167,41 @@
     /** Permissions granted by the user during runtime. */
     private ArrayList<Permission> runtimePermissions = new ArrayList<Permission>();
 
-    /** all jars not yet part of classloader or active */
-    private List<JARDesc> available = new ArrayList<JARDesc>();
+    /** all jars not yet part of classloader or active
+    * Synchronized since this field may become shared data between multiple classloading threads.
+    * See loadClass(String) and CodebaseClassLoader.findClassNonRecursive(String).
+    */
+    private List<JARDesc> available = Collections.synchronizedList(new ArrayList<JARDesc>());
 
     /** the jar cert verifier tool to verify our jars */
     private final JarCertVerifier jcv;
 
     private boolean signing = false;
 
-    /** ArrayList containing jar indexes for various jars available to this classloader */
-    private ArrayList<JarIndex> jarIndexes = new ArrayList<JarIndex>();
+    /** ArrayList containing jar indexes for various jars available to this classloader
+    * Synchronized since this field may become shared data between multiple classloading threads.
+    * See loadClass(String) and CodebaseClassLoader.findClassNonRecursive(String).
+    */
+    private List<JarIndex> jarIndexes = Collections.synchronizedList(new ArrayList<JarIndex>());
 
-    /** Set of classpath strings declared in the manifest.mf files */
-    private Set<String> classpaths = new HashSet<String>();
+    /** Set of classpath strings declared in the manifest.mf files
+    * Synchronized since this field may become shared data between multiple classloading threads.
+    * See loadClass(String) and CodebaseClassLoader.findClassNonRecursive(String).
+    */
+    private Set<String> classpaths = Collections.synchronizedSet(new HashSet<String>());
 
-    /** File entries in the jar files available to this classloader */
-    private TreeSet<String> jarEntries = new TreeSet<String>();
+    /** File entries in the jar files available to this classloader
+    * Synchronized since this field may become shared data between multiple classloading threads.
+    * See loadClass(String) and CodebaseClassLoader.findClassNonRecursive(String).
+    */
+    private Set<String> jarEntries = Collections.synchronizedSet(new TreeSet<String>());
 
-    /** Map of specific original (remote) CodeSource Urls  to securitydesc */
-    private HashMap<URL, SecurityDesc> jarLocationSecurityMap =
-            new HashMap<URL, SecurityDesc>();
+    /** Map of specific original (remote) CodeSource Urls  to securitydesc
+    * Synchronized since this field may become shared data between multiple classloading threads.
+    * See loadClass(String) and CodebaseClassLoader.findClassNonRecursive(String).
+    */
+    private Map<URL, SecurityDesc> jarLocationSecurityMap =
+            Collections.synchronizedMap(new HashMap<URL, SecurityDesc>());
 
     /*Set to prevent once tried-to-get resources to be tried again*/
     private Set<URL> alreadyTried = Collections.synchronizedSet(new HashSet<URL>());
@@ -206,16 +221,6 @@
      */
     private int useCount = 0;
 
-    /* This Object is used as a lock on the loadClass(String) method. This method should not
-     * be entered by multiple threads simultaneously. This Object should be used for no other
-     * purpose than synchronizing the body of the loadClass(String) method. The intrinsic instance
-     * lock is not suitable for this purpose or else deadlock may occur.
-     *
-     * See bug RH976833 and the mailing list archive discussion:
-     * http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2013-September/024536.html
-     */
-    private final Object loadClassLock = new Object();
-
     /**
      * Create a new JNLPClassLoader from the specified file.
      *
@@ -1222,10 +1227,14 @@
     protected void fillInPartJars(List<JARDesc> jars) {
         for (JARDesc desc : jars) {
             String part = desc.getPart();
-            for (JARDesc jar : available) {
-                if (part != null && part.equals(jar.getPart()))
-                    if (!jars.contains(jar))
-                        jars.add(jar);
+            // "available" field can be affected by two different threads
+            // working in loadClass(String)
+            synchronized (available) {
+                for (JARDesc jar : available) {
+                    if (part != null && part.equals(jar.getPart()))
+                        if (!jars.contains(jar))
+                            jars.add(jar);
+                }
             }
         }
     }
@@ -1571,35 +1580,56 @@
      * Find a JAR in the shared 'extension' classloaders, this
      * classloader, or one of the classloaders for the JNLP file's
      * extensions.
+     * This method used to be qualified "synchronized." This was done solely for the
+     * purpose of ensuring only one thread entered the method at a time. This was not
+     * strictly necessary - ensuring that all affected fields are thread-safe is
+     * sufficient. Locking on the JNLPClassLoader instance when this method is called
+     * can result in deadlock if another thread is dealing with the CodebaseClassLoader
+     * at the same time. This solution is very heavy-handed as the instance lock is not
+     * truly required, and taking the lock on the classloader instance when not needed is
+     * not in general a good idea because it can and will lead to deadlock when multithreaded
+     * classloading is in effect. The solution is to keep the fields thread safe on their own.
+     * This is accomplished by wrapping them in Collections.synchronized* to provide
+     * atomic add/remove operations, and synchronizing on them when iterating or performing
+     * multiple mutations.
+     * See bug report RH976833. On some systems this bug will manifest itself as deadlock on
+     * every webpage with more than one Java applet, potentially also causing the browser
+     * process to hang.
+     * More information in the mailing list archives:
+     * http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2013-September/024536.html
+     * 
+     * Affected fields: available, classpaths, jarIndexes, jarEntries, jarLocationSecurityMap
      */
     public Class<?> loadClass(String name) throws ClassNotFoundException {
-        synchronized (loadClassLock) {
-            Class<?> result = findLoadedClassAll(name);
+        Class<?> result = findLoadedClassAll(name);
 
-            // try parent classloader
-            if (result == null) {
-                try {
-                    ClassLoader parent = getParent();
-                    if (parent == null)
-                        parent = ClassLoader.getSystemClassLoader();
+        // try parent classloader
+        if (result == null) {
+            try {
+                ClassLoader parent = getParent();
+                if (parent == null)
+                    parent = ClassLoader.getSystemClassLoader();
 
-                    return parent.loadClass(name);
-                } catch (ClassNotFoundException ex) {
-                }
+                return parent.loadClass(name);
+            } catch (ClassNotFoundException ex) {
             }
+        }
 
-            // filter out 'bad' package names like java, javax
-            // validPackage(name);
+        // filter out 'bad' package names like java, javax
+        // validPackage(name);
 
-            // search this and the extension loaders
-            if (result == null) {
+        // search this and the extension loaders
+        if (result == null) {
+            try {
+                result = loadClassExt(name);
+            } catch (ClassNotFoundException cnfe) {
+                // Not found in external loader either
+
+                // Look in 'Class-Path' as specified in the manifest file
                 try {
-                    result = loadClassExt(name);
-                } catch (ClassNotFoundException cnfe) {
-                    // Not found in external loader either
-
-                    // Look in 'Class-Path' as specified in the manifest file
-                    try {
+                    // This field synchronized before iterating over it since it may
+                    // be shared data between threads
+                    synchronized (classpaths) {
                         for (String classpath: classpaths) {
                             JARDesc desc;
                             try {
@@ -1610,19 +1640,22 @@
                             }
                             addNewJar(desc);
                         }
-
-                        result = loadClassExt(name);
-                        return result;
-                    } catch (ClassNotFoundException cnfe1) {
-                        if (JNLPRuntime.isDebug()) {
-                            cnfe1.printStackTrace();
-                        }
                     }
 
-                    // As a last resort, look in any available indexes
+                    result = loadClassExt(name);
+                    return result;
+                } catch (ClassNotFoundException cnfe1) {
+                    if (JNLPRuntime.isDebug()) {
+                        cnfe1.printStackTrace();
+                    }
+                }
 
-                    // Currently this loads jars directly from the site. We cannot cache it because this
-                    // call is initiated from within the applet, which does not have disk read/write permissions
+                // As a last resort, look in any available indexes
+                // Currently this loads jars directly from the site. We cannot cache it because this
+                // call is initiated from within the applet, which does not have disk read/write permissions
+                // This field synchronized before iterating over it since it may
+                // be shared data between threads
+                synchronized (jarIndexes) {
                     for (JarIndex index : jarIndexes) {
                         // Non-generic code in sun.misc.JarIndex
                         @SuppressWarnings("unchecked")
@@ -1652,13 +1685,13 @@
                     }
                 }
             }
+        }
 
-            if (result == null) {
-                throw new ClassNotFoundException(name);
-            }
+        if (result == null) {
+            throw new ClassNotFoundException(name);
+        }
 
-            return result;
-            }
+        return result;
     }
 
     /**
@@ -2030,21 +2063,23 @@
 
     protected SecurityDesc getCodeSourceSecurity(URL source) {
         SecurityDesc sec=jarLocationSecurityMap.get(source);
-        if (sec == null && !alreadyTried.contains(source)) {
-            alreadyTried.add(source);
-            //try to load the jar which is requesting the permissions, but was NOT downloaded by standard way
-            if (JNLPRuntime.isDebug()) {
-                System.out.println("Application is trying to get permissions for " + source.toString() + ", which was not added by standard way. Trying to download and verify!");
-            }
-            try {
-                JARDesc des = new JARDesc(source, null, null, false, false, false, false);
-                addNewJar(des);
-                sec = jarLocationSecurityMap.get(source);
-            } catch (Throwable t) {
+        synchronized (alreadyTried) {
+            if (sec == null && !alreadyTried.contains(source)) {
+                alreadyTried.add(source);
+                //try to load the jar which is requesting the permissions, but was NOT downloaded by standard way
                 if (JNLPRuntime.isDebug()) {
-                    t.printStackTrace();
+                    System.out.println("Application is trying to get permissions for " + source.toString() + ", which was not added by standard way. Trying to download and verify!");
                 }
-                sec = null;
+                try {
+                    JARDesc des = new JARDesc(source, null, null, false, false, false, false);
+                    addNewJar(des);
+                    sec = jarLocationSecurityMap.get(source);
+                } catch (Throwable t) {
+                    if (JNLPRuntime.isDebug()) {
+                        t.printStackTrace();
+                    }
+                    sec = null;
+                }
             }
         }
         if (sec == null){
@@ -2079,8 +2114,10 @@
             addNativeDirectory(nativeDirectory);
 
         // security descriptors
-        for (URL key : extLoader.jarLocationSecurityMap.keySet()) {
-            jarLocationSecurityMap.put(key, extLoader.jarLocationSecurityMap.get(key));
+        synchronized (jarLocationSecurityMap) {
+            for (URL key : extLoader.jarLocationSecurityMap.keySet()) {
+                jarLocationSecurityMap.put(key, extLoader.jarLocationSecurityMap.get(key));
+            }
         }
     }
 
@@ -2325,9 +2362,11 @@
         }
 
         // Permissions for all remote hosting urls
-        for (URL u: jarLocationSecurityMap.keySet()) {
-            permissions.add(new SocketPermission(u.getHost(),
-                                                 "connect, accept"));
+        synchronized (jarLocationSecurityMap) {
+            for (URL u: jarLocationSecurityMap.keySet()) {
+                permissions.add(new SocketPermission(u.getHost(),
+                                                     "connect, accept"));
+            }
         }
 
         // Permissions for codebase urls (if there is a loader)