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

[Dr Andrew John Hughes]

On 16:26 Wed 20 Jul     , 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...
> 

I was going to comment on this yesterday, but it appears it was already committed.
I suggest a new patch is made wrt. mine and Deepak's comments.

> > +
> > +            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 do think RuntimeExceptions are appropriate as regards argument checks like this,
but it depends on the context from which they are called.

However, these are thrown here but then caught by the block below...  This seems wrong.
Just throw the JNLPMatcherExcpetion up here if that's the right course of action.

> > +            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.
> 
> > +            // 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);
> > +        }

I think the handling here should be more specific.

> > +    }
> > +
> > +    /**
> > +     * 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
> 

The use of new String and casting seems wrong.  I'm not sure what e.nextElement is returning
but that enumeration should be using generics.

Can xml not return the number of element names beforehand?

I'm not familiar with this XML API.  Where is it from?  Doesn't look like JAXP.

> I will leave all of the test case reviews to Jiri :)
> 
> Cheers,
> Deepak

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37