harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Hindess <mark.hind...@googlemail.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 Wed, 09 Jun 2010 17:26:01 GMT

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);
> >  		}
> >  	}
> >  
> > 
> > 
> > 
> 



Mime
View raw message