[rfc][icedtea-web] Ignore invalid .jar files in applets [resubmit]

[Jiri Vanek]

On 06/18/2012 04:42 PM, Adam Domurad wrote:
> Hey all, re-submitting my patch, as Jiri requested.
>
> ChangeLog:
> 2012-05-28  Adam Domurad<adomurad at redhat.com>
>
> 	Ignore invalid jar files in applets, like the oracle plugin does.
> 	* netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
> 	(isInvalidJar): New, checks for ZipException in new JarFile(...)
> 	(shouldFilterInvalidJars): New, checks if we are in an applet
> 	(initializeResources): if 'shouldFilterInvalidJars()' is true and a jar
> 	is not a valid jar file, the jar is filtered out and normal execution
> 	continues.
>

Looks very eell. Just few question inline
>
>
> On Mon, 2012-05-28 at 15:26 -0400, Adam Domurad wrote:
>> >  Hey all. Second try at a patch to ignore invalid .jar files, this time
>> >  only affecting applets (ie, not pages that use jnlp_href).
>> >
>> >  The proprietary plug-in seems to just skip over any malformed .jar files
>> >  and carry on loading in, while in applets. This patch emulates that
>> >  behaviour. Pages with jnlp_href still crash with a ZipException on jar
>> >  verification (the proprietary plugin also fatally errors).
>> >
>> >  This alleviates some of the symptoms of
>> >  http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1011
>> >  Once this patch has been accepted in some form, I hope to make it so
>> >  that the plugin will parse folders differently than jar files and look
>> >  for resources in folders like the proprietary plug-in does.
>> >
>> >  Le ChangeLog:
>> >  2012-05-28  Adam Domurad<adomurad at redhat.com>
>> >
>> >  	Ignore invalid jar files in applets, like the oracle plugin does.
>> >  	* netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
>> >  	(isValidJar): New, checks for ZipException in new JarFile(...)
>> >  	(shouldIgnoreInvalidJars): New, checks if we are in an applet
>> >  	(initializeResources): if 'shouldIgnoreInvalidJars()' is true and a jar
>> >  	is not a valid jar file, the jar is filtered out and normal execution
>> >  	continues.
>> >
>
>
> patch-attempt-3.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
> @@ -42,6 +42,7 @@ import java.util.Collections;
>   import java.util.Enumeration;
>   import java.util.HashMap;
>   import java.util.HashSet;
> +import java.util.Iterator;
>   import java.util.LinkedList;
>   import java.util.List;
>   import java.util.Map;
> @@ -431,6 +432,39 @@ public class JNLPClassLoader extends URL
>       }
>
>       /**
> +     * Check if a described jar file is invalid
> +     * @param jar the jar to check
> +     * @return true if file exists AND is an invalid jar, false otherwise
> +     */
> +    private boolean isInvalidJar(JARDesc jar){
> +        File cacheFile = tracker.getCacheFile(jar.getLocation());
> +        if (cacheFile == null)
> +            return false;//File cannot be retrieved, do not claim it is an invalid jar
> +        boolean isInvalid = false;
> +        try {
> +            JarFile jarFile = new JarFile(cacheFile.getAbsolutePath());
> +            jarFile.close();
> +        } catch (IOException ioe){
> +            //Catch a ZipException or any other read failure
> +            isInvalid = true;
> +        }
> +        return isInvalid;
> +    }
> +
> +    /**
> +     * Determine how invalid jars should be handled
> +     * @return whether to filter invalid jars, or error later on
> +     */
> +    private boolean shouldFilterInvalidJars(){
> +        if (file instanceof PluginBridge){
> +            PluginBridge pluginBridge = (PluginBridge)file;
> +            /*Ignore on applet, ie !useJNLPHref*/
> +            return !pluginBridge.useJNLPHref();
> +        }
> +        return false;//Error is default behaviour
> +    }
> +
> +    /**
>        * Load all of the JARs used in this JNLP file into the
>        * ResourceTracker for downloading.
>        */
> @@ -467,10 +501,26 @@ public class JNLPClassLoader extends URL
>           if (strict)
>               fillInPartJars(initialJars); // add in each initial part's lazy jars
>
> +        waitForJars(initialJars); //download the jars first.
> +
> +        //A ZipException will propagate later on if the jar is invalid and not checked here
> +        if (shouldFilterInvalidJars()){
> +            //We filter any invalid jars
> +            Iterator<JARDesc>  iterator = initialJars.iterator();
> +            while (iterator.hasNext()){
> +                JARDesc jar = iterator.next();
> +                if (isInvalidJar(jar)) {
> +                    //Remove this jar as an available jar
> +                    iterator.remove();
> +                    tracker.removeResource(jar.getLocation());
> +                    available.remove(jar);
> +                }
> +            }
> +        }
> +
>           if (JNLPRuntime.isVerifying()) {
>
>               JarCertVerifier jcv;
> -            waitForJars(initialJars); //download the jars first.

this removed line scares me a bit....
>
>               try {
>                   jcv = verifyJars(initialJars);
> @@ -533,6 +583,7 @@ public class JNLPClassLoader extends URL
>
>           for (JARDesc jarDesc : file.getResources().getJARs()) {
>               try {
> +
>                   File cachedFile = tracker.getCacheFile(jarDesc.getLocation());
>
>                   if (cachedFile == null) {
> @@ -570,6 +621,10 @@ public class JNLPClassLoader extends URL
>                   jarLocationSecurityMap.put(jarDesc.getLocation(), jarSecurity);
>               } catch (MalformedURLException mfe) {
>                   System.err.println(mfe.getMessage());
> +            } catch (IllegalArgumentException iae){

Do you think it is possible to caught this exception on better place and rethrow our custom one, which will be then catches here? IllegalaRgument one is pretty common one.

> +                //Caused by ignored resource being removed due to not being valid

this should be in debug mode or message should go from Message.properties.

> +                System.err.println("JAR " + jarDesc.getLocation() + " is not a valid jar file. Continuing.");
> +                continue;
>               }
>           }
>           activateJars(initialJars);