From dev-return-39454-apmail-harmony-dev-archive=harmony.apache.org@harmony.apache.org Fri Jun 11 11:40:16 2010 Return-Path: Delivered-To: apmail-harmony-dev-archive@www.apache.org Received: (qmail 88861 invoked from network); 11 Jun 2010 11:40:16 -0000 Received: from unknown (HELO mail.apache.org) (140.211.11.3) by 140.211.11.9 with SMTP; 11 Jun 2010 11:40:16 -0000 Received: (qmail 8183 invoked by uid 500); 11 Jun 2010 11:40:16 -0000 Delivered-To: apmail-harmony-dev-archive@harmony.apache.org Received: (qmail 7920 invoked by uid 500); 11 Jun 2010 11:40:13 -0000 Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@harmony.apache.org Delivered-To: mailing list dev@harmony.apache.org Received: (qmail 7911 invoked by uid 99); 11 Jun 2010 11:40:12 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 11 Jun 2010 11:40:12 +0000 X-ASF-Spam-Status: No, hits=-0.3 required=10.0 tests=AWL,FREEMAIL_FROM,SPF_PASS,T_TO_NO_BRKTS_FREEMAIL X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of sebbaz@gmail.com designates 209.85.161.49 as permitted sender) Received: from [209.85.161.49] (HELO mail-fx0-f49.google.com) (209.85.161.49) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 11 Jun 2010 11:40:06 +0000 Received: by fxm15 with SMTP id 15so656258fxm.36 for ; Fri, 11 Jun 2010 04:39:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:received:in-reply-to :references:date:message-id:subject:from:to:content-type; bh=b05FMfj+x/wMzyUQuYwpgucvWi5mUa1/vGbN+XyMmXE=; b=AdaT1qnq5+4tbIouLzTWjxLzlfmPAx9cSKSxGFjLCvHBcFAgBgT9ZR8sNTLdo6teFz JgTrXRYrpfcCkH0V1uyIi4P9+IAPUCPfwQ4S1Q0BSE64s4OB+ZTDaQA3mmYrDjFoPwv/ R0emGf3aZogxt60uPSXEWpX7Uc4uLJ/qeq/Lc= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; b=vhNpet+7Ij0qg4J7tTYUt9QSI4YlZ4qBPj8wd6cfafwJulWHmWAOSPbzwigCfkoWQl vYbryhtgHqW3/t8F9XSJxHi/Pu8dB+B3zyqK6tvTTScj70k6CIo/oSuwu1ENAOSt3FLG aa3NGsubSVmvjNSZm2bNzeFqJNyCTcl1q0Cd4= MIME-Version: 1.0 Received: by 10.239.177.212 with SMTP id w20mr100770hbf.200.1276256384640; Fri, 11 Jun 2010 04:39:44 -0700 (PDT) Received: by 10.239.190.70 with HTTP; Fri, 11 Jun 2010 04:39:44 -0700 (PDT) In-Reply-To: <4C11C7AC.3000401@gmail.com> References: <20100609140556.8B12A2388A70@eris.apache.org> <4C0FBF28.3000902@gmail.com> <201006091726.o59HQ1sm030030@d06av04.portsmouth.uk.ibm.com> <4C11C7AC.3000401@gmail.com> Date: Fri, 11 Jun 2010 12:39:44 +0100 Message-ID: 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 From: sebb To: dev@harmony.apache.org Content-Type: text/plain; charset=ISO-8859-1 On 11/06/2010, Ray Chen wrote: > 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(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(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(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 theAccessibleElements = > accessibleAddresses > - .elements(); > - if (theAccessibleElements.hasMoreElements()) { > - return accessibleAddresses.elements(); > + Enumeration 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(0).elements(); > >