[icedtea-web] RFC: PR898: signed applications with big jnlp-file doesn't start

[Jiri Vanek]

On 03/22/2012 10:38 AM, Jiri Vanek wrote:
Just two more  minor nitpicks

Can you add bug number/link to reproducer comment and to changelog?

> 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.