[rfc][icedtea-web] Ignore invalid .jar files in applets [resubmit]
Jiri Vanek
jvanek at redhat.com
Mon Jul 9 07:09:11 PDT 2012
On 07/04/2012 05:57 PM, Adam Domurad wrote:
> Here is a version with ResourceNotFoundException renamed to
> IllegalResourceDescriptorException.
> On Wed, 2012-07-04 at 10:31 -0400, Adam Domurad wrote:
>> > I took the approach of making the tracker throw a new type of exception,
>> > ResourceNotFoundException extends IllegalArgumentException.
>> > C&C welcome. I can easily take a different approach. Point taken @
>> > IllegalArgumentException.
Thak you. I'm ok with this now. One minnor on bottom.
J.
>> >
>> > Thanks,
>> > Adam
>> > On Wed, 2012-07-04 at 13:13 +0200, Jiri Vanek wrote:
>>> > > On 07/03/2012 09:40 PM, Adam Domurad wrote:
>>>> > > > On Tue, 2012-07-03 at 20:59 +0200, Jiri Vanek wrote:
>>>>>> > > >> > On 06/18/2012 04:42 PM, Adam Domurad wrote:
>>> > > ...
>>>>>> > > >> >
>>>>>> > > >> > this removed line scares me a bit....
>>>> > > > It was moved to the top. Confirmed OK with Omair.
>>> > >
>>> > > yy.
>>> > >
>>>>>>> > > >>> > >
>>>>>>> > > >>> > > 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;
>>>> > > > This seems consistent with the follow message slightly at the
>>>> > > > top:
>>>> > > > if(cachedFile == null) {
>>>> > > > System.err.println("JAR " + jarDesc.getLocation() + " not found.
>>>> > > > Continuing.");
>>>> > > > continue; // JAR not found. Keep going.
>>>> > > > }
>>>> > > >
>>>> > > > If the other message makes sense, this makes sense... Both or none imo
>>> > > Then I'm definitely for both. Print out stacktraces in debug mode, and localised message to stdout anywhay.
>>>> > > >
>>>>>>> > > >>> > > }
>>>>>>> > > >>> > > }
>>>>>>> > > >>> > > activateJars(initialJars);
>>>>>> > > >> >
>>>> > > >
>>>> > > > I have fixed the locality of the exception. It is now clearly to handle
>>>> > > > an exception in getting the resource, which was removed. Good catch.
>>>> > > >
>>>> > > >
>>> > > Although the place looks much more better now, I wanted the transformation even deeper.
>>> > >
>>> > > eg in ResourceTracker.getCacheFile or even deeper cough the particular isolated IllegalArgumentException, transform it to your own exception and then, on place like now, cough your new exception. IAE is really very common. Can came from anywhere.
>>> > >
>>> > >
>>> > > J.
>>> > >
>>> > >
>>> > >
>>> > >
>> >
>
>
> patch-attempt-6.patch
>
>
> diff --git a/netx/net/sourceforge/jnlp/cache/IllegalResourceDescriptorException.java b/netx/net/sourceforge/jnlp/cache/IllegalResourceDescriptorException.java
> new file mode 100644
> --- /dev/null
> +++ b/netx/net/sourceforge/jnlp/cache/IllegalResourceDescriptorException.java
> @@ -0,0 +1,13 @@
> +package net.sourceforge.jnlp.cache;
> +
> + at SuppressWarnings("serial")
> +public class IllegalResourceDescriptorException extends IllegalArgumentException {
> + /**
> + * Constructs a<code>IllegalResourceDescriptorException</code> with the
> + * specified detail message.
> + * @param msg the detail message.
> + */
> + public IllegalResourceDescriptorException(String msg) {
> + super(msg);
> + }
> +}
> diff --git a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> --- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> @@ -184,7 +184,7 @@ public class ResourceTracker {
> */
> public void addResource(URL location, Version version, DownloadOptions options, UpdatePolicy updatePolicy) {
> if (location == null)
> - throw new IllegalArgumentException("location==null");
> + throw new IllegalResourceDescriptorException("location==null");
> try {
> location = normalizeUrl(location, JNLPRuntime.isDebug());
> } catch (Exception ex) {
> @@ -225,7 +225,7 @@ public class ResourceTracker {
> * not required as resources are reclaimed when the tracker is
> * collected.
> *
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> */
> public void removeResource(URL location) {
> synchronized (resources) {
> @@ -351,7 +351,7 @@ public class ResourceTracker {
> *
> * @param location the resource location
> * @return the resource, or null if it could not be downloaded
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> * @see CacheUtil#isCacheable
> */
> public URL getCacheURL(URL location) {
> @@ -378,7 +378,7 @@ public class ResourceTracker {
> *
> * @param location the resource location
> * @return a local file containing the resource, or null
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> * @see CacheUtil#isCacheable
> */
> public File getCacheFile(URL location) {
> @@ -418,7 +418,7 @@ public class ResourceTracker {
> * the cache.
> *
> * @throws IOException if there was an error opening the stream
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> */
> public InputStream getInputStream(URL location) throws IOException {
> try {
> @@ -442,7 +442,7 @@ public class ResourceTracker {
> * @param urls the resources to wait for
> * @param timeout the time in ms to wait before returning, 0 for no timeout
> * @return whether the resources downloaded before the timeout
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> */
> public boolean waitForResources(URL urls[], long timeout) throws InterruptedException {
> Resource resources[] = new Resource[urls.length];
> @@ -468,7 +468,7 @@ public class ResourceTracker {
> * @param timeout the timeout, or 0 to wait until completed
> * @return whether the resource downloaded before the timeout
> * @throws InterruptedException if another thread interrupted the wait
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> */
> public boolean waitForResource(URL location, long timeout) throws InterruptedException {
> return wait(new Resource[] { getResource(location) }, timeout);
> @@ -479,7 +479,7 @@ public class ResourceTracker {
> *
> * @param location the resource location
> * @return the number of bytes transferred
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> */
> public long getAmountRead(URL location) {
> // not atomic b/c transferred is a long, but so what (each
> @@ -491,7 +491,7 @@ public class ResourceTracker {
> * Returns whether a resource is available for use (ie, can be
> * accessed with the getCacheFile method).
> *
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> */
> public boolean checkResource(URL location) {
> return getResource(location).isSet(DOWNLOADED | ERROR); // isSet atomic
> @@ -505,7 +505,7 @@ public class ResourceTracker {
> * resource at a time to conserve system resources.
> *
> * @return true if the resource is already downloaded (or an error occurred)
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> */
> public boolean startResource(URL location) {
> Resource resource = getResource(location);
> @@ -518,7 +518,7 @@ public class ResourceTracker {
> * enqueues the resource if not already started.
> *
> * @return true if the resource is already downloaded (or an error occurred)
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> */
> private boolean startResource(Resource resource) {
> boolean enqueue = false;
> @@ -550,7 +550,7 @@ public class ResourceTracker {
> *
> * @param location the resource location
> * @return the number of bytes, or -1
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> */
> public long getTotalSize(URL location) {
> return getResource(location).size; // atomic
> @@ -606,7 +606,7 @@ public class ResourceTracker {
> private void queueResource(Resource resource) {
> synchronized (lock) {
> if (!resource.isSet(CONNECT | DOWNLOAD))
> - throw new IllegalArgumentException("Invalid resource state (resource: " + resource + ")");
> + throw new IllegalResourceDescriptorException("Invalid resource state (resource: " + resource + ")");
>
> queue.add(resource);
> startThread();
> @@ -1030,7 +1030,7 @@ public class ResourceTracker {
> /**
> * Return the resource matching the specified URL.
> *
> - * @throws IllegalArgumentException if the resource is not being tracked
> + * @throws IllegalResourceDescriptorException if the resource is not being tracked
> */
> private Resource getResource(URL location) {
> synchronized (resources) {
> @@ -1040,7 +1040,7 @@ public class ResourceTracker {
> }
> }
>
> - throw new IllegalArgumentException("Location does not specify a resource being tracked.");
> + throw new IllegalResourceDescriptorException("Location does not specify a resource being tracked.");
> }
>
> /**
> 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
> @@ -47,6 +47,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;
> @@ -73,6 +74,7 @@ import net.sourceforge.jnlp.ResourcesDes
> import net.sourceforge.jnlp.SecurityDesc;
> import net.sourceforge.jnlp.Version;
> import net.sourceforge.jnlp.cache.CacheUtil;
> +import net.sourceforge.jnlp.cache.IllegalResourceDescriptorException;
> import net.sourceforge.jnlp.cache.ResourceTracker;
> import net.sourceforge.jnlp.cache.UpdatePolicy;
> import net.sourceforge.jnlp.security.SecurityDialogs;
> @@ -439,6 +441,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.
> */
> @@ -488,10 +523,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.
>
> try {
> jcv = verifyJars(initialJars);
> @@ -554,7 +605,16 @@ public class JNLPClassLoader extends URL
>
> for (JARDesc jarDesc : file.getResources().getJARs()) {
> try {
> - File cachedFile = tracker.getCacheFile(jarDesc.getLocation());
> +
> + File cachedFile;
> +
> + try {
> + cachedFile = tracker.getCacheFile(jarDesc.getLocation());
> + } catch (IllegalResourceDescriptorException rnte){
> + //Caused by ignored resource being removed due to not being valid
> + System.err.println("JAR " + jarDesc.getLocation() + " is not a valid jar file. Continuing.");
> + continue;
> + }
>
> if (cachedFile == null) {
> System.err.println("JAR " + jarDesc.getLocation() + " not found. Continuing.");
Please push this minor fix as different changeset.
> diff --git a/tests/reproducers/custom/AppletFolderInArchiveTag/srcs/Makefile b/tests/reproducers/custom/AppletFolderInArchiveTag/srcs/Makefile
> --- a/tests/reproducers/custom/AppletFolderInArchiveTag/srcs/Makefile
> +++ b/tests/reproducers/custom/AppletFolderInArchiveTag/srcs/Makefile
> @@ -7,7 +7,7 @@ INDEX_HTML_BODY="<html><body><h1>Require
> prepare-reproducer:
> echo PREPARING REPRODUCER $(TESTNAME)
> mkdir -p $(DEPLOY_SUBDIR)
> - echo INDEX_HTML_BODY> $(DEPLOY_SUBDIR)/index.html
> + echo $(INDEX_HTML_BODY)> $(DEPLOY_SUBDIR)/index.html
> $(EXPORTED_JAVAC) -classpath $(JAVAC_CLASSPATH) -d $(DEPLOY_SUBDIR) $(TESTNAME).java
> echo PREPARED REPRODUCER $(TESTNAME)
>
More information about the distro-pkg-dev
mailing list