harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ray Chen <clrayc...@gmail.com>
Subject 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
Date Fri, 11 Jun 2010 05:20:44 GMT
On 06/10/2010 01:26 AM, Mark Hindess wrote:
> In message<4C0FBF28.3000902@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, hindessm@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();


Mime
View raw message