[RFC][PATCH][icedtea-web]: Added support for signed JNLP file- Updated Patch

[Jiri Vanek]

On 07/20/2011 10:26 PM, Deepak Bhole wrote:
> * Saad Mohammad<smohammad at redhat.com>  [2011-07-19 11:04]:
>> >  I have attached the updated copy of Patch1.
>> >
>> >  On 07/19/2011 02:47 AM, Jiri Vanek wrote:
>>>> >  >>
>>>> >  >>I hope this is the 110% for Patch1! ;)
>>> >  >99% ;)
>> >
>> >  This is for sure the 110% ;)
>> >
>> >  --
>> >  Cheers,
>> >  Saad Mohammad
>> >
> Hi Saad,
>
> I have a few minor corrections for this one...
>
>> >  +
>> >  +            if (appTemplate == null&&  launchJNLP == null)
>> >  +                throw new NullPointerException(
>> >  +                        "Template JNLP file and Launching JNLP file are both null.");
>> >  +            else if (appTemplate == null)
>> >  +                throw new NullPointerException("Template JNLP file is null.");
>> >  +            else if (launchJNLP == null)
>> >  +                throw new NullPointerException("Launching JNLP file is null.");
>> >  +
> Throwing RuntimeExceptions is generally not a good idea. Consider making
> the above a checked exception.

I believe nullpointer exception is correct here. Or it will make mess later.
Although to repalce it with JNLMatch exception can do no harm.

>
>> >  +            XMLElement appTemplateXML = new XMLElement();
>> >  +            XMLElement launchJNLPXML = new XMLElement();
>> >  +
>> >  +            // Remove the comments and CDATA from the JNLP file
>> >  +            final PipedInputStream pinTemplate = new PipedInputStream();
>> >  +            final PipedOutputStream poutTemplate = new PipedOutputStream(pinTemplate);
>> >  +            appTemplateXML.sanitizeInput(appTemplate, poutTemplate);
>> >  +
>> >  +            final PipedInputStream pinJNLPFile = new PipedInputStream();
>> >  +            final PipedOutputStream poutJNLPFile = new PipedOutputStream(pinJNLPFile);
>> >  +            launchJNLPXML.sanitizeInput(launchJNLP, poutJNLPFile);
>> >  +
> The above streams are never closed.

I' guess, ...

...see previous email..

...l) s.close();
}


around s.close() yo can possibly clsoe io exception into JNLPMatcherException.


>
>> >  +            // Parse both files
>> >  +            appTemplateXML.parseFromReader(new InputStreamReader(pinTemplate));
>> >  +            launchJNLPXML.parseFromReader(new InputStreamReader(pinJNLPFile));
>> >  +
>> >  +            // Initialize parent nodes
>> >  +            this.appTemplateNode = new Node(appTemplateXML);
>> >  +            this.launchJNLPNode = new Node(launchJNLPXML);
>> >  +            this.isTemplate = isTemplate;
>> >  +
>> >  +        } catch (Exception e) {
>> >  +            throw new JNLPMatcherException(
>> >  +                    "Failed to create an instance of JNLPVerify with specified InputStreamReader",
>> >  +                    e);
>> >  +        }
>> >  +    }
>> >  +
>> >  +    /**
>> >  +     * Compares both JNLP files
>> >  +     *
>> >  +     * @return true if both JNLP files are 'matched', otherwise false
>> >  +     */
>> >  +    public boolean isMatch() {
>> >  +
>> >  +        if (match == null)
>> >  +            match = matchNodes(appTemplateNode, launchJNLPNode);
>> >  +
>> >  +        return match;
>> >  +    }
>> >  +
> Is there a case where match needs to be cached (i.e. isMatch is called
> multiple times for the same JNLPMatcher)? If not, the above function is
> not needed.
>
> ...
>> >  +
>> >  +    /**
>> >  +     * Getter for application/template Node
>> >  +     *
>> >  +     * @return the Node of the signed application/template file
>> >  +     */
>> >  +    public Node getAppTemplateNode() {
>> >  +        return appTemplateNode;
>> >  +    }
>> >  +
>> >  +    /**
>> >  +     * Getter for launching application Node
>> >  +     *
>> >  +     * @return the Node of the launching JNLP file
>> >  +     */
>> >  +    public Node getLaunchJNLPNode() {
>> >  +        return launchJNLPNode;
>> >  +    }
>> >  +
> I don't see anything calling the above .. the functions should be
> removed in that case.
>
> ...
>> >  @@ -86,6 +102,7 @@
>> >        private ParsedXML tinyNode;
>> >        private Node next;
>> >        private Node children[];
>> >  +    private String attributeNames[];
>> >
>> >        Node(ParsedXML tinyNode) {
>> >            this.tinyNode = tinyNode;
>> >  @@ -127,6 +144,19 @@
>> >
>> >            return children;
>> >        }
>> >  +
>> >  +    String[] getAttributeNames() {
>> >  +        if (attributeNames == null) {
>> >  +            List<String>  list = new ArrayList<String>();
>> >  +
>> >  +            for (Enumeration e = xml.enumerateAttributeNames(); e.hasMoreElements();)
>> >  +                list.add(new String((String) e.nextElement()));
>> >  +
>> >  +            attributeNames = list.toArray(new String[list.size()]);
>> >  +
>> >  +        }
>> >  +        return attributeNames;
>> >  +    }
>> >
> Isn't the above all in commented code? Line 100 to 176 is commented out
> in HEAD:
> http://icedtea.classpath.org/hg/icedtea-web/file/7dd63058c234/netx/net/sourceforge/jnlp/Node.java

Yes, this was already commited. Suggested changes can be posted as new patch.
>
> I will leave all of the test case reviews to Jiri :)
>
> Cheers,
> Deepak

Deepak, Tahnx a lot for remarks!


Regards J.