[icedtea-web] RFC: PR898: signed applications with big jnlp-file doesn't start
Jiri Vanek
jvanek at redhat.com
Thu Mar 22 02:38:06 PDT 2012
On 03/21/2012 03:44 PM, Omair Majid wrote:
> Hi,
>
> The attached patch fixes PR898: signed applications with big jnlp-file
> doesn't start. We were using piped input and output streams without
> threads which would cause them to block and javaws to hang. Switching to
> ByteArrayOuputStream/StringReader takes care of this.
>
> Okay for HEAD and 1.2?
Hi Omair!
Ty for patch, I think that test can go in immediately (even separately).
For patch itself I have few comments inline
>
> Thanks,
> Omair
>
>
> pr898-01.patch
>
>
> diff --git a/ChangeLog b/ChangeLog
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,13 @@
> +2012-03-21 Omair Majid<omajid at redhat.com>
> +
> + * tests/netx/unit/net/sourceforge/jnlp/JNLPMatcherTest.java
> + (testIsMatchDoesNotHangOnLargeData): New method.
> +
> +2012-03-21 Lars Herschke<lhersch at dssgmbh.de>
> +
> + * netx/net/sourceforge/jnlp/JNLPMatcher.java (JNLPMatcher): Handle large
Little bit more like replacing pipes by ArrayStreams can be better, but I do not insists as You are
just pushing other's changeset.
> + files correctly.
> +
> 2012-03-19 Danesh Dadachanji<ddadacha at redhat.com>
>
> Fix failing unit test missing title/vendor tags in the JNLP stream.
> diff --git a/netx/net/sourceforge/jnlp/JNLPMatcher.java b/netx/net/sourceforge/jnlp/JNLPMatcher.java
> --- a/netx/net/sourceforge/jnlp/JNLPMatcher.java
> +++ b/netx/net/sourceforge/jnlp/JNLPMatcher.java
> @@ -38,11 +38,12 @@
> package net.sourceforge.jnlp;
>
> import java.util.List;
> +import java.io.ByteArrayOutputStream;
> import java.io.InputStream;
> import java.io.InputStreamReader;
> import java.io.OutputStream;
> -import java.io.PipedInputStream;
> -import java.io.PipedOutputStream;
> +import java.io.Reader;
> +import java.io.StringReader;
> import java.util.Arrays;
> import java.util.Collections;
> import java.util.LinkedList;
> @@ -75,7 +76,7 @@
> * if IOException, XMLParseException is thrown during parsing;
> * Or launchJNLP/appTemplate is null
> */
> - public JNLPMatcher(InputStreamReader appTemplate, InputStreamReader launchJNLP,
> + public JNLPMatcher(Reader appTemplate, Reader launchJNLP,
> boolean isTemplate) throws JNLPMatcherException {
>
> if (appTemplate == null&& launchJNLP == null)
> @@ -87,29 +88,25 @@
> throw new JNLPMatcherException("Launching JNLP file is null.");
>
> //Declare variables for signed JNLP file
> - PipedInputStream pinTemplate= null;
> - PipedOutputStream poutTemplate= null;
> + ByteArrayOutputStream poutTemplate= null;
>
> //Declare variables for launching JNLP file
> - PipedInputStream pinJNLPFile = null;
> - PipedOutputStream poutJNLPFile = null;
> + ByteArrayOutputStream poutJNLPFile = null;
>
> try {
> XMLElement appTemplateXML = new XMLElement();
> XMLElement launchJNLPXML = new XMLElement();
>
> // Remove the comments and CDATA from the JNLP file
> - pinTemplate = new PipedInputStream();
> - poutTemplate = new PipedOutputStream(pinTemplate);
> + poutTemplate = new ByteArrayOutputStream();
> appTemplateXML.sanitizeInput(appTemplate, poutTemplate);
>
> - pinJNLPFile = new PipedInputStream();
> - poutJNLPFile = new PipedOutputStream(pinJNLPFile);
> + poutJNLPFile = new ByteArrayOutputStream();
> launchJNLPXML.sanitizeInput(launchJNLP, poutJNLPFile);
>
> // Parse both files
> - appTemplateXML.parseFromReader(new InputStreamReader(pinTemplate));
> - launchJNLPXML.parseFromReader(new InputStreamReader(pinJNLPFile));
> + appTemplateXML.parseFromReader(new StringReader(poutTemplate.toString()));
those two toString() should be definietly called with encoding. Although it is not deprecated (as
methods without encoding are in most of other classes), it can cause mess.
http://docs.oracle.com/javase/6/docs/api/java/io/ByteArrayOutputStream.html#toString%28java.lang.String%29
There is small pitfall anyway - you should use the encoding defined in jnlp's xml file instead of
any "hardcoded". (and I think that with this new approach sting <=> byte[] without pipes you can
easily do this). I'm afraid most of jnlp files have been written on windows machines anyway:)
If you are ok now with default utf-8 encoding, I'm for, and if you will consider the encoding
reading and then re-encode if necessary, then it can be as separate patch.
> + launchJNLPXML.parseFromReader(new StringReader(poutJNLPFile.toString()));
>
> // Initialize parent nodes
> this.appTemplateNode = new Node(appTemplateXML);
> @@ -122,10 +119,8 @@
> e);
> } finally {
> // Close all stream
> - closeInputStream(pinTemplate);
> closeOutputStream(poutTemplate);
>
> - closeInputStream(pinJNLPFile);
> closeOutputStream(poutJNLPFile);
>
> }
One more idea I have to this patch is in Parser (net/sourceforge/jnlp/Parser.java) is also used
Piped mechanism. Isn't worthy to fix it too? And there is also some wired encoing recognition... At
least look at it (as I was gazing to it quite confused).
I was also thinking that get rid of buffering can cause unnecessary requirements but at he end I
think there is just minor increase if any. Please correct me If I'm judging it wrongly.
> diff --git a/tests/netx/unit/net/sourceforge/jnlp/JNLPMatcherTest.java b/tests/netx/unit/net/sourceforge/jnlp/JNLPMatcherTest.java
> --- a/tests/netx/unit/net/sourceforge/jnlp/JNLPMatcherTest.java
> +++ b/tests/netx/unit/net/sourceforge/jnlp/JNLPMatcherTest.java
> @@ -37,12 +37,13 @@
>
> package net.sourceforge.jnlp;
>
> -import static org.junit.Assert.fail;
> -
> import java.io.IOException;
> import java.io.InputStream;
> import java.io.InputStreamReader;
> -import junit.framework.Assert;
> +import java.io.StringReader;
> +import java.util.Random;
> +
> +import org.junit.Assert;
> import org.junit.Test;
>
> public class JNLPMatcherTest {
> @@ -461,4 +462,36 @@
> fileReader.close();
> launchReader.close();
> }
> +
> + @Test (timeout=1000 /*ms*/)
> + public void testIsMatchDoesNotHangOnLargeData() throws JNLPMatcherException {
> + /* construct an alphabet containing characters 'a' to 'z' */
> + final int ALPHABET_SIZE = 26;
> + char[] alphabet = new char[ALPHABET_SIZE];
> + for (int i = 0; i< ALPHABET_SIZE; i++) {
> + alphabet[i] = (char)('a' + i);
> + }
> + /* generate a long but random string using the alphabet */
> + final Random r = new Random();
> + final int STRING_SIZE = 1024 * 1024; // 1 MB
> + StringBuilder descriptionBuilder = new StringBuilder(STRING_SIZE);
> + for (int i = 0; i< STRING_SIZE; i++) {
> + descriptionBuilder.append(alphabet[r.nextInt(ALPHABET_SIZE)]);
> + }
> + String longDescription = descriptionBuilder.toString();
> +
> + String file =
> + "<jnlp>\n" +
> + "<information>\n" +
> + "<title>JNLPMatcher hanges on large file size</title>\n" +
> + "<vendor>IcedTea</vendor>\n" +
> + "<description>" + longDescription +"</description>\n" +
> + "</information>\n" +
> + "</jnlp>\n";
> +
> + StringReader reader1 = new StringReader(file);
> + StringReader reader2 = new StringReader(file);
> + JNLPMatcher matcher = new JNLPMatcher(reader1, reader2, false);
> + Assert.assertTrue(matcher.isMatch());
> + }
> }
>
Best regards and sorry if I read the patch wrongly
J.
More information about the distro-pkg-dev
mailing list