[rfc][icedtea-web] RH976833 ClassLoader deadlock
Jiri Vanek
jvanek at redhat.com
Thu Sep 5 04:42:20 PDT 2013
On 08/15/2013 10:10 PM, Andrew Azores wrote:
> Changelog:
>
> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: (loadClass) made unsynchronized.
> (available, jarIndexes, classpaths, jarEntries, jarLocationSecurityMap) wrapped in synchronized
> classes.
>
> An earlier change to the classloader made it possible for two threads to enter deadlock. This is
Can we have an reproducer for this? But those are mostly very hard to reproduce, so do not vaste to
much time on it (no metter that reprodcuer can have random behaviour as it "can deadlock")
> resolved by not giving the thread which calls loadClass the lock on the JNLPClassLoader instance,
> and instead synchronizing the member variables which are accessed in loadClass and other methods
> called by loadClasss.
>
> Also made some for-loops into for-each-loops for readability.
Sorry for being nitpicker, but please separate refactoring form fix. Both looks good but the
readability of patch is strongly reduced by the mixture.
Also I'm fan of "refactoring is good time to do an unittest"
So the best would be for each method, you touch with "for-loops into fpor-each-loops " refactoring
is an excelent candidate for unittest. When I wrote this to Adam an year ago, he stopepd todo this
:) So I do not wont to force you to do, but please think about it.
>
> Thanks,
>
> --
> Andrew A
>
>
> fix.patch
>
>
> 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
> @@ -17,11 +17,6 @@
>
> import static net.sourceforge.jnlp.runtime.Translator.R;
>
> -import java.util.concurrent.locks.ReentrantLock;
> -
> -import java.util.concurrent.locks.Lock;
> -
> -import java.io.Closeable;
> import java.io.File;
> import java.io.FileOutputStream;
> import java.io.FileReader;
> @@ -59,11 +54,10 @@
> import java.util.TreeSet;
> import java.util.Vector;
> import java.util.concurrent.ConcurrentHashMap;
> +import java.util.concurrent.locks.ReentrantLock;
> import java.util.jar.JarEntry;
> -import net.sourceforge.jnlp.util.JarFile;
> import java.util.jar.Manifest;
>
> -import net.sourceforge.jnlp.security.appletextendedsecurity.UnsignedAppletTrustConfirmation;
> import net.sourceforge.jnlp.AppletDesc;
> import net.sourceforge.jnlp.ApplicationDesc;
> import net.sourceforge.jnlp.DownloadOptions;
> @@ -90,8 +84,9 @@
> import net.sourceforge.jnlp.security.JNLPAppVerifier;
> import net.sourceforge.jnlp.security.PluginAppVerifier;
> import net.sourceforge.jnlp.security.SecurityDialogs;
> +import net.sourceforge.jnlp.security.appletextendedsecurity.UnsignedAppletTrustConfirmation;
> import net.sourceforge.jnlp.tools.JarCertVerifier;
> -import net.sourceforge.jnlp.util.FileUtils;
> +import net.sourceforge.jnlp.util.JarFile;
> import net.sourceforge.jnlp.util.StreamUtils;
> import sun.misc.JarIndex;
>
> @@ -166,7 +161,7 @@
> private ArrayList<Permission> runtimePermissions = new ArrayList<Permission>();
>
> /** all jars not yet part of classloader or active */
> - private List<JARDesc> available = new ArrayList<JARDesc>();
> + private List<JARDesc> available = Collections.synchronizedList(new ArrayList<JARDesc>());
>
> /** the jar cert verifier tool to verify our jars */
> private final JarCertVerifier jcv;
> @@ -174,17 +169,16 @@
> private boolean signing = false;
>
> /** ArrayList containing jar indexes for various jars available to this classloader */
> - private ArrayList<JarIndex> jarIndexes = new ArrayList<JarIndex>();
> + 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>();
> + 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>();
> + 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>();
> + 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>());
> @@ -617,18 +611,18 @@
> */
> List<JARDesc> initialJars = new ArrayList<JARDesc>();
>
> - for (int i = 0; i < jars.length; i++) {
> + for (JARDesc jar : jars) {
>
> - available.add(jars[i]);
> + available.add(jar);
>
> - if (jars[i].isEager())
> - initialJars.add(jars[i]); // regardless of part
> + if (jar.isEager())
> + initialJars.add(jar); // regardless of part
>
> - tracker.addResource(jars[i].getLocation(),
> - jars[i].getVersion(),
> - getDownloadOptionsForJar(jars[i]),
> - jars[i].isCacheable() ? JNLPRuntime.getDefaultUpdatePolicy() : UpdatePolicy.FORCE
> - );
> + tracker.addResource(jar.getLocation(),
> + jar.getVersion(),
> + getDownloadOptionsForJar(jar),
> + jar.isCacheable() ? JNLPRuntime.getDefaultUpdatePolicy() : UpdatePolicy.FORCE
> + );
> }
>
> //If there are no eager jars, initialize the first jar
> @@ -1170,15 +1164,17 @@
> * in the same part).
> */
> protected void fillInPartJars(List<JARDesc> jars) {
> - for (int i = 0; i < jars.size(); i++) {
> - String part = jars.get(i).getPart();
> + for (JARDesc jar : jars) {
> + String part = jar.getPart();
>
> - 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);
> + synchronized (available) {
> + for (JARDesc availJar : available) {
> + if (part != null && part.equals(availJar.getPart())) {
> + if (!jars.contains(availJar)) {
> + jars.add(availJar);
> + }
> + }
> + }
> }
> }
> }
> @@ -1199,9 +1195,7 @@
> // transfer the Jars
> waitForJars(jars);
>
> - for (int i = 0; i < jars.size(); i++) {
> - JARDesc jar = jars.get(i);
> -
> + for (JARDesc jar : jars) {
> available.remove(jar);
>
> // add jar
> @@ -1429,7 +1423,7 @@
> * classloader, or one of the classloaders for the JNLP file's
> * extensions.
> */
> - public synchronized Class<?> loadClass(String name) throws ClassNotFoundException {
> + public Class<?> loadClass(String name) throws ClassNotFoundException {
>
> Class<?> result = findLoadedClassAll(name);
>
> @@ -1457,15 +1451,17 @@
>
> // Look in 'Class-Path' as specified in the manifest file
> 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);
> @@ -1480,31 +1476,33 @@
>
> // 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<String> jarList = index.get(name.replace('.', '/'));
> + synchronized (jarIndexes) {
> + for (JarIndex index : jarIndexes) {
> + // Non-generic code in sun.misc.JarIndex
> + @SuppressWarnings("unchecked")
> + LinkedList<String> 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);
> }
> }
> }
> @@ -1661,7 +1659,7 @@
> // add resources until found
> while (true) {
> JNLPClassLoader addedTo = null;
> -
> +
> try {
> addedTo = addNextResource();
> } catch (LaunchException e) {
> @@ -1822,7 +1820,7 @@
> * @throws LaunchException Thrown if the signed JNLP file, within the main jar, fails to be verified or does not match
> */
> protected JNLPClassLoader addNextResource() throws LaunchException {
> - if (available.size() == 0) {
> + if (available.isEmpty()) {
> for (int i = 1; i < loaders.length; i++) {
> JNLPClassLoader result = loaders[i].addNextResource();
>
> @@ -1885,22 +1883,24 @@
> */
>
> 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) {
> + SecurityDesc sec = jarLocationSecurityMap.get(source);
> + 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 +1936,11 @@
> }
>
> // security descriptors
> - for (URL key : extLoader.jarLocationSecurityMap.keySet()) {
> - jarLocationSecurityMap.put(key, extLoader.jarLocationSecurityMap.get(key));
> + synchronized (extLoader.jarLocationSecurityMap) {
> + Set<URL> keys = extLoader.jarLocationSecurityMap.keySet();
> + for (URL key : keys) {
> + jarLocationSecurityMap.put(key, extLoader.jarLocationSecurityMap.get(key));
> + }
> }
> }
>
> @@ -2186,9 +2189,12 @@
> }
>
> // Permissions for all remote hosting urls
> - for (URL u: jarLocationSecurityMap.keySet()) {
> - permissions.add(new SocketPermission(u.getHost(),
> - "connect, accept"));
> + synchronized (jarLocationSecurityMap) {
> + Set<URL> keys = jarLocationSecurityMap.keySet();
> + for (URL u : keys) {
> + permissions.add(new SocketPermission(u.getHost(),
> + "connect, accept"));
> + }
> }
>
> // Permissions for codebase urls (if there is a loader)
>
More information about the distro-pkg-dev
mailing list