[rfc] [icedtea-web] providing little bit more debug outputs for few methods

Jiri Vanek jvanek at redhat.com
Fri May 11 04:08:24 PDT 2012


Back to list.

All suggested grammar and  formatting improvements have been applied.

> <off-list>
>
> Hi Jiri,
>
> There are a few grammar issues noted below. Can you please repost with
> the fixes and I will approve.
>
> * Jiri Vanek<jvanek at redhat.com>  [2012-04-30 08:12]:
>> On 04/27/2012 05:29 PM, Deepak Bhole wrote:
>>>
>> Thank you for review!
>>> This patch is increasing verbosity and changing behavior (throwing a
>> The verbosity is not increased so much:)
>> -one message in (3) when SecurityDesc is null
>> -one message in debug mode enriched  for one path (1)
>> -one stacktrace printed out in debug mode (is printed out (without debug) in  in-browser-applet already)
>>
>
> Thanks! Comments below:
>
>> If you do not like any of this changesets just say, I do not insists
>> on any of them. Thy just come to me handy (and in case of consumed
>> exception correct)
>>> different type of exception in getPermissions.
>>
>> arrrgggh. Thanx for catch. Fixed. Exception is  printed and truly
>> re-thrown (not copy of it at it was:-/) now.
>>>
>>> IMO it should be split..
>> As you wish :)
>>>
>> Changelogs:
>> debuggingEnchancements2_1_pathToKeystores.diff
>>
>> 2012-04-30 Jiri Vanek<jvanek at redhat.com>
>>
>> 	* netx/net/sourceforge/jnlp/security/KeyStores.java: (getPathToKeystore):
>> 	new method, able to search for file used for creating of KeyStore if possible
>> 	* netx/net/sourceforge/jnlp/security/CertificateUtils.java: (inKeyStores)
>> 	using getPathToKeystore for debug output.
>>
>> debuggingEnchancements2_2_consumedExceptionPrintOut.diff
>>
>> 2012-04-30  Jiri Vanek<jvanek at redhat.com>
>>
>> 	* netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: (getPermissions):
>> 	Any exception from this method is consumed somewhere. I have cough exception,
>> 	reprint it in debug mode and re-throw (to be lost). Main condition in this
>> 	method had several possible NullPointer exceptions. Separated and thrown before
>> 	this condition.
>> 	
>>
>> debuggingEnchancements2_3_printOutMissingSecurityDesc.diff
>>
>> 2012-04-30  Jiri Vanek<jvanek at redhat.com>
>>
>> 	Added more debuging outputs
>> 	* netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
>> 	(getCodeSourceSecurity): added output message when no SecurityDesc is found
>> 	for some url/resource
>> 	
>>
>> Best regards, J.
>
>> diff -r 82e908d46d70 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Tue Apr 24 14:43:34 2012 -0400
>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Mon Apr 30 13:44:55 2012 +0200
>> @@ -1732,7 +1743,12 @@
>>        */
>>
>>       protected SecurityDesc getCodeSourceSecurity(URL source) {
>> -        return jarLocationSecurityMap.get(source);
>> +        SecurityDesc sec=jarLocationSecurityMap.get(source);
>> +        if (sec==null){
>> +            System.out.println("Error! No security instance for signed "+source.toString()+" This source was loaded outside of netx, and application will have troubles to continue");
>> +        }
>> +        return sec;
>> +
>
> Can we change the above to use the R function? Also, string should be:
> "Error: No security instance for "+source.toString()+". The application may have trouble continuing"
>
> This is because although right now getCodeSourceSecurity() may always be
> receiving signed jars, this may not always be the case in the future.
>
>>       }
>>
>>       /**
>
>> diff -r 82e908d46d70 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Tue Apr 24 14:43:34 2012 -0400
>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Mon Apr 30 13:44:55 2012 +0200
>> @@ -895,6 +895,7 @@
>>        * Returns the permissions for the CodeSource.
>>        */
>>       protected PermissionCollection getPermissions(CodeSource cs) {
>> +        try{
>>           Permissions result = new Permissions();
>>
>
> Looks like the code inside try/catch needs re-indenting.
>
>>           // should check for extensions or boot, automatically give all
>> @@ -912,6 +913,10 @@
>>               // If more than default is needed:
>>               // 1. Code must be signed
>>               // 2. ALL or J2EE permissions must be requested (note: plugin requests ALL automatically)
>> +            if (cs==null) throw new RuntimeException("cs!=null but was");
>
> "Code source was null"
>
>> +            if (cs.getLocation()==null) throw new RuntimeException("cs.getLocation!=null but was");
>
> "Code source location was null"
>
>> +            if (getCodeSourceSecurity(cs.getLocation())==null) throw new RuntimeException("getCodeSourceSecurity(cs.getLocation())!=null but was");
>
> "Code source security was null"
>
>> +            if (getCodeSourceSecurity(cs.getLocation()).getSecurityType()==null) throw new RuntimeException("getCodeSourceSecurity(cs.getLocation()).getSecurityType()!=null but was");
>
> "Code source security type was null"
>
>>               if (cs.getCodeSigners() != null&&
>>                       (getCodeSourceSecurity(cs.getLocation()).getSecurityType().equals(SecurityDesc.ALL_PERMISSIONS) ||
>>                        getCodeSourceSecurity(cs.getLocation()).getSecurityType().equals(SecurityDesc.J2EE_PERMISSIONS))) {
>> @@ -933,6 +938,12 @@
>>               result.add(runtimePermissions.get(i));
>>
>>           return result;
>> +         }catch(RuntimeException ex){
>> +                if (JNLPRuntime.isDebug()){
>> +                ex.printStackTrace();
>> +                }
>
> Indenting :)
>
>> +                throw ex;
>> +            }
>>       }
>>
>>       protected void addPermission(Permission p) {
>
>> diff -r 82e908d46d70 netx/net/sourceforge/jnlp/security/CertificateUtils.java
>> --- a/netx/net/sourceforge/jnlp/security/CertificateUtils.java	Tue Apr 24 14:43:34 2012 -0400
>> +++ b/netx/net/sourceforge/jnlp/security/CertificateUtils.java	Mon Apr 30 13:44:55 2012 +0200
>> @@ -173,7 +173,7 @@
>>
>>                       if (c.equals(keyStores[i].getCertificate(alias))) {
>>                           if (JNLPRuntime.isDebug()) {
>> -                            System.out.println(c.getSubjectX500Principal().getName() + " found in cacerts");
>> +                            System.out.println(c.getSubjectX500Principal().getName() + " found in cacerts ("+KeyStores.getPathToKeystore(keyStores[i].hashCode())+")");
>
> I know you just replaced the string, but can we move the above to use R
> as well?
I have did this too, but i disagree. It is debug message and so it should remains not-localised (as 
eg some automated stdout reader can depend on it O:)

But final deccission is up to you. I can apply this old one or new one.

>
> Rest looks great!
>
> Cheers,
> Deepak

Changelogs: (I know that properties file change is duplicated but I will commit it correctly)
debuggingEnchancements3_1_pathToKeystores.diff

2012-05-11 Jiri Vanek  <jvanek at redhat.com>

     * netx/net/sourceforge/jnlp/security/KeyStores.java: (getPathToKeystore):
     new method, able to search for file used for creating of KeyStore if possible
     * netx/net/sourceforge/jnlp/security/CertificateUtils.java: (inKeyStores)
     using getPathToKeystore for debug output.
     * netx/net/sourceforge/jnlp/resources/Messages.properties: added LCertFoundIn
     value

debuggingEnchancements3_2_consumedExceptionPrintOut.diff

2012-05-11  Jiri Vanek  <jvanek at redhat.com>

     * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: (getPermissions):
     Any exception from this method is consumed somewhere. I have cough exception,
     reprint it in debug mode and re-throw (to be lost). Main condition in this
     method had several possible NullPointer exceptions. Separated and thrown before
     this condition.


debuggingEnchancements3_3_printOutMissingSecurityDesc.diff

2012-05-11  Jiri Vanek  <jvanek at redhat.com>

     Added more debuging outputs
     * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
     (getCodeSourceSecurity): added output message when no SecurityDesc is found
     for some url/resource
     * netx/net/sourceforge/jnlp/resources/Messages.properties: added LNoSecInstance
     value


When I was digging in Message.properties, i found possible error. It is fixed in 
debuggingEnchancements3_typoInMessagesProperties. According to code:
             if (base == null)
                 throw new ParseException(R("PBadNonrelativeUrl", node.getNodeName(), href));
             else
                 throw new ParseException(R("PBadRelativeUrl", node.getNodeName(), href, base));
and eg  PBadRelativeUrl=Invalid relative URL (node={0}, href={1}, base={2})
I believe my fix is doing right thing

2012-05-11  Jiri Vanek  <jvanek at redhat.com>

     * netx/net/sourceforge/jnlp/resources/Messages.properties: fixed error
     in PBadNonrelativeUrl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: debuggingEnchancements3_1_pathToKeystores.diff
Type: text/x-patch
Size: 3465 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120511/52075ffc/debuggingEnchancements3_1_pathToKeystores.diff 
-------------- next part --------------
diff -r 11029e99d733 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
--- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Wed May 02 12:53:07 2012 +0200
+++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Fri May 11 12:06:43 2012 +0200
@@ -895,44 +895,66 @@
      * Returns the permissions for the CodeSource.
      */
     protected PermissionCollection getPermissions(CodeSource cs) {
-        Permissions result = new Permissions();
+        try {
+            Permissions result = new Permissions();
 
-        // should check for extensions or boot, automatically give all
-        // access w/o security dialog once we actually check certificates.
+            // should check for extensions or boot, automatically give all
+            // access w/o security dialog once we actually check certificates.
 
-        // copy security permissions from SecurityDesc element
-        if (security != null) {
-            // Security desc. is used only to track security settings for the
-            // application. However, an application may comprise of multiple
-            // jars, and as such, security must be evaluated on a per jar basis.
+            // copy security permissions from SecurityDesc element
+            if (security != null) {
+                // Security desc. is used only to track security settings for the
+                // application. However, an application may comprise of multiple
+                // jars, and as such, security must be evaluated on a per jar basis.
 
-            // set default perms
-            PermissionCollection permissions = security.getSandBoxPermissions();
+                // set default perms
+                PermissionCollection permissions = security.getSandBoxPermissions();
 
-            // If more than default is needed:
-            // 1. Code must be signed
-            // 2. ALL or J2EE permissions must be requested (note: plugin requests ALL automatically)
-            if (cs.getCodeSigners() != null &&
-                    (getCodeSourceSecurity(cs.getLocation()).getSecurityType().equals(SecurityDesc.ALL_PERMISSIONS) ||
-                     getCodeSourceSecurity(cs.getLocation()).getSecurityType().equals(SecurityDesc.J2EE_PERMISSIONS))) {
+                // If more than default is needed:
+                // 1. Code must be signed
+                // 2. ALL or J2EE permissions must be requested (note: plugin requests ALL automatically)
+                if (cs == null) {
+                    throw new RuntimeException("Code source was null");
+                }
+                if (cs.getLocation() == null) {
+                    throw new RuntimeException("Code source location was null");
+                }
+                if (getCodeSourceSecurity(cs.getLocation()) == null) {
+                    throw new RuntimeException("Code source security was null");
+                }
+                if (getCodeSourceSecurity(cs.getLocation()).getSecurityType() == null) {
+                    throw new RuntimeException("Code source security type was null");
+                }
+                if (cs.getCodeSigners() != null
+                        && (getCodeSourceSecurity(cs.getLocation()).getSecurityType().equals(SecurityDesc.ALL_PERMISSIONS)
+                        || getCodeSourceSecurity(cs.getLocation()).getSecurityType().equals(SecurityDesc.J2EE_PERMISSIONS))) {
 
-                permissions = getCodeSourceSecurity(cs.getLocation()).getPermissions(cs);
+                    permissions = getCodeSourceSecurity(cs.getLocation()).getPermissions(cs);
+                }
+
+                Enumeration<Permission> e = permissions.elements();
+                while (e.hasMoreElements()) {
+                    result.add(e.nextElement());
+                }
             }
 
-            Enumeration<Permission> e = permissions.elements();
-            while (e.hasMoreElements())
-                result.add(e.nextElement());
+            // add in permission to read the cached JAR files
+            for (int i = 0; i < resourcePermissions.size(); i++) {
+                result.add(resourcePermissions.get(i));
+            }
+
+            // add in the permissions that the user granted.
+            for (int i = 0; i < runtimePermissions.size(); i++) {
+                result.add(runtimePermissions.get(i));
+            }
+
+            return result;
+        } catch (RuntimeException ex) {
+            if (JNLPRuntime.isDebug()) {
+                ex.printStackTrace();
+            }
+            throw ex;
         }
-
-        // add in permission to read the cached JAR files
-        for (int i = 0; i < resourcePermissions.size(); i++)
-            result.add(resourcePermissions.get(i));
-
-        // add in the permissions that the user granted.
-        for (int i = 0; i < runtimePermissions.size(); i++)
-            result.add(runtimePermissions.get(i));
-
-        return result;
     }
 
     protected void addPermission(Permission p) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: debuggingEnchancements3_3_printOutMissingSecurityDesc.diff.diff
Type: text/x-patch
Size: 1411 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120511/52075ffc/debuggingEnchancements3_3_printOutMissingSecurityDesc.diff.diff 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: debuggingEnchancements3_typoInMessagesProperties.diff
Type: text/x-patch
Size: 782 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120511/52075ffc/debuggingEnchancements3_typoInMessagesProperties.diff 


More information about the distro-pkg-dev mailing list