diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java @@ -165,26 +165,41 @@ /** Permissions granted by the user during runtime. */ private ArrayList runtimePermissions = new ArrayList(); - /** all jars not yet part of classloader or active */ - private List available = new ArrayList(); + /** all jars not yet part of classloader or active. + * Synchronized since this field may become shared data between multiple classloading threads. + * See loadClass() and CodebaseClassLoader.findClassNonRecursive() + */ + private List available = Collections.synchronizedList(new ArrayList()); /** 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 jarIndexes = new ArrayList(); + /** 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() and CodebaseClassLoader.findClassNonRecursive() + */ + private List jarIndexes = Collections.synchronizedList(new ArrayList()); - /** Set of classpath strings declared in the manifest.mf files */ - private Set classpaths = new HashSet(); + /** Set of classpath strings declared in the manifest.mf files + * Synchronized since this field may become shared data between multiple classloading threads. + * See loadClass() and CodebaseClassLoader.findClassNonRecursive() + */ + private Set classpaths = Collections.synchronizedSet(new HashSet()); - /** File entries in the jar files available to this classloader */ - private TreeSet jarEntries = new TreeSet(); + /** File entries in the jar files available to this classloader + * Synchronized since this field may become shared data between multiple classloading threads. + * See loadClass() and CodebaseClassLoader.findClassNonRecursive() + */ + private Set jarEntries = Collections.synchronizedSet(new TreeSet()); - /** Map of specific original (remote) CodeSource Urls to securitydesc */ - private HashMap jarLocationSecurityMap = - new HashMap(); + /** Map of specific original (remote) CodeSource Urls to securitydesc + * Synchronized since this field may become shared data between multiple classloading threads. + * See loadClass() and CodebaseClassLoader.findClassNonRecursive() + */ + private Map jarLocationSecurityMap = + Collections.synchronizedMap(new HashMap()); /*Set to prevent once tried-to-get resources to be tried again*/ private Set alreadyTried = Collections.synchronizedSet(new HashSet()); @@ -1173,12 +1188,16 @@ for (int i = 0; i < jars.size(); i++) { String part = jars.get(i).getPart(); - for (int a = 0; a < available.size(); a++) { - JARDesc jar = available.get(a); + // "available" field can be affected by two different threads + // working in loadClass() + synchronized (available) { + for (int a = 0; a < available.size(); a++) { + JARDesc jar = available.get(a); - if (part != null && part.equals(jar.getPart())) - if (!jars.contains(jar)) - jars.add(jar); + if (part != null && part.equals(jar.getPart())) + if (!jars.contains(jar)) + jars.add(jar); + } } } } @@ -1428,8 +1447,32 @@ * 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. This problem could also be avoided by using a construct + * like a ReentrantLock to restrict one thread to calling this method at a time, + * so that the affected fields are still not shared data, but the calling thread also + * does not take the instance lock. However, this is more of a hack and work-around + * than simply ensuring that the fields are kept thread safe on their own. This is + * accomplished by wrapping them in Collections.synchronized{List,Set,etc.} to provide + * atomic add/remove operations, and synchronizing on them when iterating or performing + * multiple mutations. + * + * See bug report RH976833. On some systems the 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 synchronized Class loadClass(String name) throws ClassNotFoundException { + public Class loadClass(String name) throws ClassNotFoundException { Class result = findLoadedClassAll(name); @@ -1456,16 +1499,20 @@ // Not found in external loader either // Look in 'Class-Path' as specified in the manifest file + // This field synchronized before iterating over it since it may + // be shared data between threads try { - for (String classpath: classpaths) { - JARDesc desc; - try { - URL jarUrl = new URL(file.getCodeBase(), classpath); - desc = new JARDesc(jarUrl, null, null, false, true, false, true); - } catch (MalformedURLException mfe) { - throw new ClassNotFoundException(name, mfe); + synchronized (classpaths) { + for (String classpath: classpaths) { + JARDesc desc; + try { + URL jarUrl = new URL(file.getCodeBase(), classpath); + desc = new JARDesc(jarUrl, null, null, false, true, false, true); + } catch (MalformedURLException mfe) { + throw new ClassNotFoundException(name, mfe); + } + addNewJar(desc); } - addNewJar(desc); } result = loadClassExt(name); @@ -1479,32 +1526,36 @@ // 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 - for (JarIndex index : jarIndexes) { - // Non-generic code in sun.misc.JarIndex - @SuppressWarnings("unchecked") - LinkedList jarList = index.get(name.replace('.', '/')); + // 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") + LinkedList jarList = index.get(name.replace('.', '/')); - if (jarList != null) { - for (String jarName : jarList) { - JARDesc desc; - try { - desc = new JARDesc(new URL(file.getCodeBase(), jarName), - null, null, false, true, false, true); - } catch (MalformedURLException mfe) { - throw new ClassNotFoundException(name); - } - try { - addNewJar(desc); - } catch (Exception e) { - if (JNLPRuntime.isDebug()) { - e.printStackTrace(); + if (jarList != null) { + for (String jarName : jarList) { + JARDesc desc; + try { + desc = new JARDesc(new URL(file.getCodeBase(), jarName), + null, null, false, true, false, true); + } catch (MalformedURLException mfe) { + throw new ClassNotFoundException(name); + } + try { + addNewJar(desc); + } catch (Exception e) { + if (JNLPRuntime.isDebug()) { + e.printStackTrace(); + } } } + + // If it still fails, let it error out + result = loadClassExt(name); } - - // If it still fails, let it error out - result = loadClassExt(name); } } } @@ -1535,6 +1586,9 @@ */ private void addNewJar(final JARDesc desc, UpdatePolicy updatePolicy) { + // not synchronized on since this field is wrapped in Collections.synchronizedList + // and so provides atomic add/remove already. Adding an element to a list that + // already contains the element does nothing. So synchronization is not needed. available.add(desc); tracker.addResource(desc.getLocation(), @@ -1558,7 +1612,10 @@ final URL remoteURL = desc.getLocation(); final URL cachedUrl = tracker.getCacheURL(remoteURL); // blocks till download - available.remove(desc); // Resource downloaded. Remove from available list. + // Resource downloaded. Remove from available list. Remove is an atomic operation since + // this is a Collections.synchronizedList. Removing an element from a list that does + // not contain the element does nothing. So synchronization is not needed. + available.remove(desc); try { @@ -1886,21 +1943,25 @@ 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) { + // synchronize on alreadyTried since we are checking if the set contains an element, + // then potentially adding an element immediately after + 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){ @@ -1936,8 +1997,11 @@ } // security descriptors - for (URL key : extLoader.jarLocationSecurityMap.keySet()) { - jarLocationSecurityMap.put(key, extLoader.jarLocationSecurityMap.get(key)); + // synchronize on this before iterating and mutating since it may be shared between threads + synchronized (jarLocationSecurityMap) { + for (URL key : extLoader.jarLocationSecurityMap.keySet()) { + jarLocationSecurityMap.put(key, extLoader.jarLocationSecurityMap.get(key)); + } } } @@ -2182,9 +2246,12 @@ } // Permissions for all remote hosting urls - for (URL u: jarLocationSecurityMap.keySet()) { - permissions.add(new SocketPermission(u.getHost(), - "connect, accept")); + // synchronize on this before iterating since it may be shared between threads + synchronized (jarLocationSecurityMap) { + for (URL u: jarLocationSecurityMap.keySet()) { + permissions.add(new SocketPermission(u.getHost(), + "connect, accept")); + } } // Permissions for codebase urls (if there is a loader)