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

Saad Mohammad smohammad at redhat.com
Mon Jul 18 13:37:30 PDT 2011


I have attached the updated copy of Patch1 that involves changes that 
you recommended.
Patch2 has not been attached in this email because I haven't made any 
changes from my previous email.

...[snip]...
>> As you have requested, I have added the copyright headers to 
>> JNLPMatcher.java and JNLPMatcherTest.java. I am not sure if you would 
>> also like me to add it to the jnlp files that are being used for test 
>> purposes.
>> The reason why I didn't add the header to the jnlp files is because I 
>> didn't want anything from the copyright header to interfere with the 
>> test.
>> So, that's why I have only added the headers to the two files but 
>> please let me know if you would also like me to add them to the jnlp 
>> files.
>>
>
> Personally I agree with you. I But I think that headers files must be 
> in each file. But plase wait with this change until jnlpMatcher is in 
> and also/or patch for classloader(+ reproducers) is in to protect 
> readability (then I'm afraid it will be necessary).
>
> Still some concerns remains (100% of new code :D )

Okay, I will wait and then implement this change :)

>
>>
>> Patch1a.patch
>>
>>
>> diff -r 86abbf8be0b1 Makefile.am
>> --- a/Makefile.am    Thu Jun 23 15:29:45 2011 +0200
>> +++ b/Makefile.am    Thu Jul 14 17:11:49 2011 -0400
>> @@ -538,7 +538,14 @@
>>
>>   run-netx-unit-tests: stamps/netx-unit-tests-compile.stamp \
>>    $(JUNIT_RUNNER_JAR) $(TESTS_DIR)/$(REPORT_STYLES_DIRNAME)
>> -    cp 
>> {$(NETX_UNIT_TEST_SRCDIR),$(NETX_UNIT_TEST_DIR)}/net/sourceforge/jnlp/basic.jnlp
>> +    filename=" " ; \
>> +    cd $(NETX_UNIT_TEST_SRCDIR) ; \
>> +    for file in `find . -type f`; do\
>> +        filename=`echo $$file `; \
>> +        if [[ $$filename != *$.java ]]; then \
>> +            cp --parents $$filename $(NETX_UNIT_TEST_DIR) ; \
>> +        fi ; \
>> +    done ; \
>>       cd $(NETX_UNIT_TEST_DIR) ; \
>>       class_names= ; \
>>       for test in `find -type f` ; do \
>
> OK here ^, let i t be. Just FYI, find have argument to find all "NOT 
> this suffix" options. (negation by ! or attribute not...)

Changed this!

>> diff -r 86abbf8be0b1 netx/net/sourceforge/jnlp/JNLPMatcher.java
>> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> +++ b/netx/net/sourceforge/jnlp/JNLPMatcher.java    Thu Jul 14 
>> 17:11:49 2011 -0400
>> @@ -0,0 +1,275 @@
>> +/* JNLPMatcher.java
>> +   Copyright (C) 2011 Red Hat, Inc.
>> +
>> +This file is part of IcedTea.
>> +
>> +IcedTea is free software; you can redistribute it and/or
>> +modify it under the terms of the GNU General Public License as 
>> published by
>> +the Free Software Foundation, version 2.
>> +
>> +IcedTea is distributed in the hope that it will be useful,
>> +but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +General Public License for more details.
>> +
>> +You should have received a copy of the GNU General Public License
>> +along with IcedTea; see the file COPYING.  If not, write to
>> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, 
>> Boston, MA
>> +02110-1301 USA.
>> +
>> +Linking this library statically or dynamically with other modules is
>> +making a combined work based on this library.  Thus, the terms and
>> +conditions of the GNU General Public License cover the whole
>> +combination.
>> +
>> +As a special exception, the copyright holders of this library give you
>> +permission to link this library with independent modules to produce an
>> +executable, regardless of the license terms of these independent
>> +modules, and to copy and distribute the resulting executable under
>> +terms of your choice, provided that you also meet, for each linked
>> +independent module, the terms and conditions of the license of that
>> +module.  An independent module is a module which is not derived from
>> +or based on this library.  If you modify this library, you may extend
>> +this exception to your version of the library, but you are not
>> +obligated to do so.  If you do not wish to do so, delete this
>> +exception statement from your version.
>> + */
>> +
>> +package net.sourceforge.jnlp;
>> +
>> +import java.util.List;
>> +import java.io.InputStreamReader;
>> +import java.io.PipedInputStream;
>> +import java.io.PipedOutputStream;
>> +import java.util.ArrayList;
>> +import java.util.Arrays;
>> +import java.util.Collections;
>> +import java.util.LinkedList;
>> +import net.sourceforge.nanoxml.XMLElement;
>> +
>> +/**
>> + * To compare launching JNLP file with signed APPLICATION.JNLP or
>> + * APPLICATION_TEMPLATE.jnlp.
>> + *
>> + * Used by net.sourceforge.jnlp.runtime.JNLPCLassLoader
>> + */
>> +
>> +public final class JNLPMatcher {
>> +
>> +    private final Node appTemplateNode;
>> +    private final Node launchJNLPNode;
>> +    private final boolean isTemplate;
>> +    private String match = null;
> WHY oh why String :-/// !!! This should.. have to ..be Boolean 
> (capital B)

sorry, changed this too. I was unaware of wrapper classes :(

>> +
>> +    /**
>> +     * Public constructor
>> +     *
>> +     * @param appTemplate
>> +     *            the reader stream of the signed APPLICATION.jnlp or
>> +     *            APPLICATION_TEMPLATE.jnlp
>> +     * @param launchJNLP
>> +     *            the reader stream of the launching JNLP file
>> +     * @param isTemplate
>> +     *            a boolean that specifies if appTemplateFile is a 
>> template
>> +     * @throws JNLPMatcherException
>> +     *             if IOException, XMLParseException is thrown 
>> during parsing;
>> +     *             Or launchJNLP/appTemplate is null
>> +     */
>> +    public JNLPMatcher(InputStreamReader appTemplate, 
>> InputStreamReader launchJNLP,
>> +            boolean isTemplate) throws JNLPMatcherException {
>> +
>> +        try {
>> +
>> +            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.");
>> +
>> +            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);
>> +
>> +            // 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) ? 
>> "true" : "false";
>> +
>> +        if (match.equals("true"))
>> +            return true;
>> +        else
>> +            return false;
>> +    }
>> +
>
> This is really an art how NOT to do it :) Ideal solution can be object 
> wrapping boolean. And we have this one in java...:
>
>     public boolean isMatch() {
>
>         if (match == null){
>             match = matchNodes(appTemplateNode, launchJNLPNode) ;
>         }
>         return match;
>
>     }
>
>
> Study a little bit about Boolean x boolean and about autoboxing in 
> Java. No more changes here then is already written (eg do NOT return 
> BBBBolean from matchNodes)

Changed this as recommended.

>
>> +    /**
>> +     * Compares two Nodes regardless of the order of their 
>> children/attributes
>> +     *
>> +     * @param appTemplate
>> +     *            signed application or template's Node
>> +     * @param launchJNLP
>> +     *            launching JNLP file's Node
>> +     *
>> +     * @return true if both Nodes are 'matched', otherwise false
>> +     */
>> +    private boolean matchNodes(Node appTemplate, Node launchJNLP) {
>> +
>> +        if (appTemplate != null&&  launchJNLP != null) {
>> +
>> +            Node templateNode = appTemplate;
>> +            Node launchNode = launchJNLP;
>> +            // Store children of Node
>> +            List<Node>  appTemplateChild = new 
>> LinkedList<Node>(Arrays.asList(templateNode
>> +                    .getChildNodes()));
>> +            List<Node>  launchJNLPChild = new 
>> LinkedList<Node>(Arrays.asList(launchNode
>> +                    .getChildNodes()));
>> +
>> +            // Compare only if both Nodes have the same name, else 
>> return false
>> +            if 
>> (templateNode.getNodeName().equals(launchNode.getNodeName())) {
>> +
>> +                if (appTemplateChild.size() == 
>> launchJNLPChild.size()) { // Compare
>> +                                                                         
>> // children
>> +
>> +                    int childLength = appTemplateChild.size();
>> +
>> +                    for (int i = 0; i<  childLength;) {
>> +                        for (int j = 0; j<  childLength; j++) {
>> +                            boolean isSame = 
>> matchNodes(appTemplateChild.get(i),
>> +                                    launchJNLPChild.get(j));
>> +
>> +                            if (!isSame&&  j == childLength - 1)
>> +                                return false;
>> +                            else if (isSame) { // If both child 
>> matches, remove them from the list of children
>> +                                appTemplateChild.remove(i);
>> +                                launchJNLPChild.remove(j);
>> +                                --childLength;
>> +                                break;
>> +                            }
>> +                        }
>> +                    }
>> +
>> +                    if 
>> (!templateNode.getNodeValue().equals(launchNode.getNodeValue())) {
>> +
>> +                        // If it's a template and the template's 
>> value is NOT '*'
>> +                        if (isTemplate&&  
>> !templateNode.getNodeValue().equals("*"))
>> +                            return false;
>> +                        // Else if it's not a template, then return 
>> false
>> +                        else if (!isTemplate)
>> +                            return false;
>> +                    }
>> +                    // Compare attributes of both Nodes
>> +                    return matchAttributes(templateNode, launchNode);
>> +                }
>> +
>> +            }
>> +        }
>> +        return false;
>> +    }
>> +
>> +    /**
>> +     * Compares attributes of two Nodes regardless of order
>> +     *
>> +     * @param appTemplateNode
>> +     *            signed application or template's Node with attributes
>> +     * @param launchJNLPNode
>> +     *            launching JNLP file's Node with attributes
>> +     *
>> +     * @return true if both Nodes have 'matched' attributes, 
>> otherwise false
>> +     */
>> +    private boolean matchAttributes(Node templateNode, Node 
>> launchNode) {
>> +
>> +        if (templateNode != null&&  launchNode != null) {
>> +
>> +            ArrayList<String>  appTemplateAttributes = 
>> templateNode.getAttributeNames();
>> +            ArrayList<String>  launchJNLPAttributes = 
>> launchNode.getAttributeNames();
>> +
>
> Same mistake again. Please use interface where-ever it is possible.
>
> List<String>  appTemplateAttributes = templateNode.getAttributeNames();
> List<String>  launchJNLPAttributes = launchNode.getAttributeNames();
>
> Will serve you well.

Changed to List<String>.

>
>> +            Collections.sort(appTemplateAttributes);
>> +            Collections.sort(launchJNLPAttributes);
>> +
>> +            if (appTemplateAttributes.size() == 
>> launchJNLPAttributes.size()) {
>> +
>> +                int size = appTemplateAttributes.size(); // Number 
>> of attributes
>> +
>> +                for (int i = 0; i<  size; i++) {
>> +
>> +                    if 
>> (launchJNLPAttributes.get(i).equals(appTemplateAttributes.get(i))) { 
>> // If both Node's attribute name are the
>> +                                                                                            
>> // same then compare the values
>> +
>> +                        String attribute = launchJNLPAttributes.get(i);
>> +                        boolean isSame = 
>> templateNode.getAttribute(attribute).equals( // Check if the 
>> Attribute values match
>> +                                launchNode.getAttribute(attribute));
>> +
>> +                        if (!isTemplate&&  !isSame)
>> +                            return false;
>> +                        else if (isTemplate&&  !isSame
>> +&&  !templateNode.getAttribute(attribute).equals("*"))
>> +                            return false;
>> +
>> +                    } else
>> +                        // If attributes names do not match, return 
>> false
>> +                        return false;
>> +                }
>> +                return true;
>> +            }
>> +        }
>> +        return false;
>> +    }
>> +
>> +    /**
>> +     * 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;
>> +    }
>> +
>> +    /**
>> +     * Getter for isTemplate
>> +     *
>> +     * @return true if a signed template is being used for matching; 
>> otherwise
>> +     *         false.
>> +     */
>> +    public boolean isTemplate() {
>> +        return isTemplate;
>> +    }
>> +}
>> diff -r 86abbf8be0b1 netx/net/sourceforge/jnlp/JNLPMatcherException.java
>> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> +++ b/netx/net/sourceforge/jnlp/JNLPMatcherException.java    Thu Jul 
>> 14 17:11:49 2011 -0400
>> @@ -0,0 +1,16 @@
>> +package net.sourceforge.jnlp;
>> +
>> +public class JNLPMatcherException extends Exception
>> +{
>> +    private static final long serialVersionUID = 1L;
>> +
>> +    public JNLPMatcherException(String message)
>> +    {
>> +        super(message);
>> +    }
>> +
>> +    public JNLPMatcherException(String message, Throwable cause)
>> +    {
>> +        super(message, cause);
>> +    }
>> +}
>> diff -r 86abbf8be0b1 netx/net/sourceforge/jnlp/Node.java
>> --- a/netx/net/sourceforge/jnlp/Node.java    Thu Jun 23 15:29:45 2011 
>> +0200
>> +++ b/netx/net/sourceforge/jnlp/Node.java    Thu Jul 14 17:11:49 2011 
>> -0400
>> @@ -19,6 +19,7 @@
>>       private XMLElement xml;
>>       private Node next;
>>       private Node children[];
>> +    private ArrayList<String>  attributeNames= null;
>
> Oh dear :) no No NO
> private List<String>  attributeNames= null; for sure!

Changed this to List<String> too :)

>
>>
>>       Node(XMLElement xml) {
>>           this.xml = xml;
>> @@ -60,6 +61,21 @@
>>
>>           return children;
>>       }
>> +
>> +    /**
>> +     * To retrieve all attribute names
>> +     * @return all attribute names of the Node in ArrayList<String>
>> +     */
>> +    ArrayList<String>  getAttributeNames() {
> :-/
>
> again List<String>  getAttributeNames()
>
> and in this case  this is most important!

Changed.

>
>> +        if (attributeNames == null) {
>> +            attributeNames= new ArrayList<String>();
>> +
>> +            for (Enumeration e = xml.enumerateAttributeNames(); 
>> e.hasMoreElements();)
>> +                attributeNames.add(new String((String) 
>> e.nextElement()));
>> +        }
>> +
>> +        return attributeNames;
>> +    }
>>
>>       String getAttribute(String name) {
>>           return (String) xml.getAttribute(name);
>> @@ -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;
>> +    }
>>
>>       String getAttribute(String name) {
>>           return tinyNode.getAttribute(name);
>
> Generally I'm ok with test. I have just idea to test what happens with 
> your engine when something unexpected will happen. Eg one of the files 
> will be un-parse-able (=> matchNodes will proably throw an runtime 
> exception is match probably also....) . Can you add this tests as 
> separate patch after this one is in? ( Which will be next round I 
> suppose;)
>

Sure, I will work on a separate patch that will implement some more tests.

>> diff -r 86abbf8be0b1 
>> tests/netx/unit/net/sourceforge/jnlp/JNLPMatcherTest.java
>> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/netx/unit/net/sourceforge/jnlp/JNLPMatcherTest.java    
>> Thu Jul 14 17:11:49 2011 -0400
>> @@ -0,0 +1,464 @@
>> +/* JNLPMatcherTest.java
>> +   Copyright (C) 2011 Red Hat, Inc.
>> +
>
> ...snip...
>
>>
>> Patch2a.patch
>>
>>
...[snip]...
>
> Generally ok ^, But it will definitely goes in after patch1a and after 
> some reproducers will be added. How much time do you have to have this 
> finished?

I have until the first week of September to complete everything.

>
> Regards J... and.. study interfaces;)

I hope this is the 110% for Patch1! ;)

-- 
Cheers,
Saad Mohammad

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: Patch1b.patch
Url: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110718/27fc6bfe/Patch1b.patch 


More information about the distro-pkg-dev mailing list