Re: svn commit: r953015 - in /harmony/enhanced/java/trunk/classlib/modules/luni/src: main/java/java/net/NetworkInterface.java test/api/common/org/apache/harmony/luni/tests/java/net/NetworkInterfaceTest.java

Thu, 10 Jun 2010 22:24:01 -0700 

On 06/10/2010 01:26 AM, Mark Hindess wrote:

In message<4c0fbf28.3000...@gmail.com>, Tim Ellison writes:

I've not benched it but this looks like a reasonable amount of code to
run each time the hostname is checked against this permission.

Maybe not too bad for getting these addresses from the interface, but
if they are checked for each connect...

Is it worth caching the full form of the IPv6 address in the
InetAddress itself instance?

Though not obvious from the commit diff, in this context, I don't think
it is a problem ... since you probably don't lookup addresses associated
with an interface very often.

The question in my mind, that I was still thinking about is:

   Are there other calls to SecurityManager.checkConnect(...) that
   might have compressed hostnames?  We probably need to audit them to
   check.

Looking at the context is a little worrying... first I was puzzled by
the (security != null) check in the loop when it could be moved outside:

   for (InetAddress element : addresses) {
     if (security != null) {
       try {
         /*
          * since we don't have a port in this case we pass in
          * NO_PORT
          */
         String hostName = element.getHostName();
         if(hostName.contains("::")){
           hostName = getFullFormOfCompressedIPV6Address(hostName);
         }
         security.checkConnect(hostName, CHECK_CONNECT_NO_PORT);
         accessibleAddresses.add(element);
       } catch (SecurityException e) {
       }
     }
   }

Then I noticed that the code immediately preceding this was:

   /*
    * get the security manager. If one does not exist just return the full
    * list
    */
   SecurityManager security = System.getSecurityManager();
   if (security == null) {
     return (new Vector<InetAddress>(Arrays.asList(addresses))).elements();
   }

   /*
    * ok security manager exists so check each address and return those
    * that pass
    */
   for (InetAddress element : addresses) {
     if (security != null) {
       ...
     }
   }

So the security != null isn't needed anyway.

I added an item to my todo list to take a better look at this code.  I
would not mind if someone beats me to it though.

Regards,
  Mark.


On 09/Jun/2010 15:05, hinde...@apache.org wrote:

Author: hindessm
Date: Wed Jun  9 14:05:56 2010
New Revision: 953015

URL: http://svn.apache.org/viewvc?rev=953015&view=rev
Log:
Applying patches from "[#HARMONY-6532] [classlib][luni]SocketPermission
does NOT support compressed IPV6 address, should pass in full form
address".

Modified:
     harmony/enhanced/java/trunk/classlib/modules/luni/src/main/java/java/ne

t/NetworkInterface.java

     harmony/enhanced/java/trunk/classlib/modules/luni/src/test/api/common/o

rg/apache/harmony/luni/tests/java/net/NetworkInterfaceTest.java

Modified: harmony/enhanced/java/trunk/classlib/modules/luni/src/main/java/j

ava/net/NetworkInterface.java

URL: http://svn.apache.org/viewvc/harmony/enhanced/java/trunk/classlib/modu

les/luni/src/main/java/java/net/NetworkInterface.java?rev=953015&r1=953014&r2
=953015&view=diff

===========================================================================

===

--- harmony/enhanced/java/trunk/classlib/modules/luni/src/main/java/java/ne

t/NetworkInterface.java (original)

+++ harmony/enhanced/java/trunk/classlib/modules/luni/src/main/java/java/ne

t/NetworkInterface.java Wed Jun  9 14:05:56 2010

@@ -166,7 +166,11 @@ public final class NetworkInterface exte
                       * since we don't have a port in this case we pass in
                       * NO_PORT
                       */
-                    security.checkConnect(element.getHostName(),
+                    String hostName = element.getHostName();
+                    if(hostName.contains("::")){
+                        hostName = getFullFormOfCompressedIPV6Address(host

Name);

+                    }
+                    security.checkConnect(hostName,
                              CHECK_CONNECT_NO_PORT);
                      accessibleAddresses.add(element);
                  } catch (SecurityException e) {
@@ -182,6 +186,62 @@ public final class NetworkInterface exte

          return new Vector<InetAddress>(0).elements();
      }
+
+    private String getFullFormOfCompressedIPV6Address(String compressed) {
+        StringBuilder fullForm = new StringBuilder(39);
+        final int NUM_OF_IPV6_FIELDS = 8;
+
+        String[] fields = compressed.split(":");
+        // the number of compressed fields
+        int numOfCompressedFields;
+
+        if (compressed.startsWith("::")) { // compressed head part
+            compressed = compressed.replace("::", "");
+            fields = compressed.split(":");
+            numOfCompressedFields = NUM_OF_IPV6_FIELDS - fields.length;
+            restoreCompressedFieldsWithZero(fullForm, numOfCompressedField

s);

+            appendNonZeroFields(fullForm, fields);
+        } else if (compressed.endsWith("::")) { // compressed tail part
+            compressed = compressed.replace("::", "");
+            fields = compressed.split(":");
+            numOfCompressedFields = NUM_OF_IPV6_FIELDS - fields.length;
+            appendNonZeroFields(fullForm, fields);
+            restoreCompressedFieldsWithZero(fullForm, numOfCompressedField

s);

+        } else { // compressed middle part
+            numOfCompressedFields = NUM_OF_IPV6_FIELDS - fields.length + 1

;

+            for (String field : fields) {
+                if (field.equals("")) {
+                    // for compressed fields add 0
+                    restoreCompressedFieldsWithZero(fullForm, numOfCompres

sedFields);

+                } else {
+                    fullForm.append(field);
+                    // add colon
+                    fullForm.append(":");
+                }
+            }
+        }
+        // delete the excess colon
+        fullForm.deleteCharAt(fullForm.length() - 1);
+
+        return fullForm.toString();
+    }
+
+    private void appendNonZeroFields(StringBuilder fullForm,
+            String[] fields) {
+        for (int i = 0; i<  fields.length; i++) {
+            fullForm.append(fields[i]);
+            fullForm.append(":");
+        }
+    }
+
+    private void restoreCompressedFieldsWithZero(StringBuilder fullForm,
+            int numOfCompressedFields) {
+        for (int i = 0; i<  numOfCompressedFields; i++) {
+            fullForm.append("0");
+            // add colon
+            fullForm.append(":");
+        }
+    }

      /**
       * Gets the human-readable name associated with this network interface

.

Modified: harmony/enhanced/java/trunk/classlib/modules/luni/src/test/api/co

mmon/org/apache/harmony/luni/tests/java/net/NetworkInterfaceTest.java

URL: http://svn.apache.org/viewvc/harmony/enhanced/java/trunk/classlib/modu

les/luni/src/test/api/common/org/apache/harmony/luni/tests/java/net/NetworkIn
terfaceTest.java?rev=953015&r1=953014&r2=953015&view=diff

===========================================================================

===

--- harmony/enhanced/java/trunk/classlib/modules/luni/src/test/api/common/o

rg/apache/harmony/luni/tests/java/net/NetworkInterfaceTest.java (original)

+++ harmony/enhanced/java/trunk/classlib/modules/luni/src/test/api/common/o

rg/apache/harmony/luni/tests/java/net/NetworkInterfaceTest.java Wed Jun  9 14
:05:56 2010

@@ -376,12 +376,22 @@ public class NetworkInterfaceTest extend
                                        networkInterface1.toString());
                        assertFalse("validate that non-zero length string is ge

nerated",

                                        networkInterface1.toString().equals("")

);

+
+            SecurityManager backup = System.getSecurityManager();
+            System.setSecurityManager(new SecurityManager());
+            assertNotNull(networkInterface1.toString());
+            System.setSecurityManager(backup);
                }
                if (atLeastTwoInterfaces) {
                        assertFalse(
                                        "Validate strings are different for dif

ferent interfaces",

                                        networkInterface1.toString().equals(
                                                        networkInterface2.toStr

ing()));

+
+            SecurityManager backup = System.getSecurityManager();
+            System.setSecurityManager(new SecurityManager());
+            assertNotNull(networkInterface2.toString());
+            System.setSecurityManager(backup);
                }
        }







Hi Mark,
How about this? :
Index: src/main/java/java/net/NetworkInterface.java
===================================================================
--- src/main/java/java/net/NetworkInterface.java    (revision 953551)
+++ src/main/java/java/net/NetworkInterface.java    (working copy)
@@ -153,14 +153,8 @@
         if (security == null) {
             return (new Vector<InetAddress>(Arrays.asList(addresses)))
                     .elements();
-        }
-
-        /*
- * ok security manager exists so check each address and return those 
-         * that pass
-         */
-        for (InetAddress element : addresses) {
-            if (security != null) {
+        } else {
+            for (InetAddress element : addresses) {
                 try {
                     /*
                      * since we don't have a port in this case we pass in
@@ -176,12 +170,11 @@
                 } catch (SecurityException e) {
                 }
             }
-        }
- Enumeration<InetAddress> theAccessibleElements = accessibleAddresses 
-                .elements();
-        if (theAccessibleElements.hasMoreElements()) {
-            return accessibleAddresses.elements();
+ Enumeration<InetAddress> theAccessibleElements = accessibleAddresses.elements(); 
+            if (theAccessibleElements.hasMoreElements()) {
+                return accessibleAddresses.elements();
+            }
         }

         return new Vector<InetAddress>(0).elements();

Reply via email to