You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by sebb <se...@gmail.com> on 2010/06/11 13:39:44 UTC
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
On 11/06/2010, Ray Chen <cl...@gmail.com> wrote:
> On 06/10/2010 01:26 AM, Mark Hindess wrote:
>
> > In message<4C...@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();
> + }
I think it would be clearer to just drop the redundant null check.
Adding the else clause after an unconditional return just causes
unnecessary nesting of the following elements.
Indeed this is one of the items the Eclipse IDE warns about (unless
the check is disabled).
> }
>
> return new Vector<InetAddress>(0).elements();
>
>