RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

Alan Bateman Alan.Bateman at oracle.com
Sat Jan 2 07:40:44 UTC 2016


On 31/12/2015 14:30, Mark Sheppard wrote:
> Hi
>   please oblige and review the current version of the fix  for
> https://bugs.openjdk.java.net/browse/JDK-8134577
>
> at
> http://cr.openjdk.java.net/~msheppar/8134577/webrev.05/
>
> which is  based on feedback from the second review.
I looked through the latest webrev and I think this is starting to look 
good. Having the hosts file be the same format (albeit a subset) as 
/etc/hosts is a big improvement on previous iterations.

InetAddress.nameServices is still a List<NameService> but the List will 
always have one element, should this be changed to 
InetAddress.nameService (singular)?

I think it would be cleaner if NameService were changed to an interface 
with two implementations, say PlatformNameService and 
HostsFileNameService. That way you could eliminate the 
useLocalHostsFileLookup and hostsFileName fields from InetAddress and 
HostsFileNameService could encapsulate the parsing of the hosts file 
rather than NameService trying to support both ways.

The property name jdk.net.hosts.file is good but if set to a file that 
doesn't exist then the current patch falls back to use the platform name 
service. I assume this is not the original intention.

The Scanner is created without specifying a charset, is this deliberate 
because the platform /etc/hosts is in platform encoding? For tests then 
it might be better to use UTF-8 because these hosts file will be used on 
several different platforms.

Is there any reason to use legacy StringTokenizer in 
createAddressByteArray? In other areas of the patch then it using 
Scanner or String.split so it seems inconsistent to see legacy 
StringTokenizer in use too.

You mentioned in the mail about # supported as a comment when the first 
character. It doesn't seem to be much effort to just drop everything 
after the # so that it is more consistent with the platform hosts file.

UHE is thrown in a few places without any exception message. For the 
hosts file then it would be useful to include some message to make 
configuration issues easier to diagnose.

The comment in NameService has a historical reference to the JNDI DNS 
provider, I assume that is not needed.

I also looked through the test changes. Several tests set test.src and 
not clear that this is needed. I assume that what you really want is:
String testSrc = System.getProperty("test.src", ".");

addMappingToHostsFile is added to a number of tests. It would be good if 
this could use try-with-resources to avoid leaving a file open then the 
write fails (say a test machine with a full file system).

sun/security/x509/URICertStore/ExtensionsWithLDAP.java has been added to 
the exclude list with JDK-8134577 as the issue number. Is there is a 
different issue number for this?

Are all of the tests in test/sun/net/InetAddress/nameservice still 
needed? Some of these tests, as least in the 'dns' directory, are tests 
for the JNDI DNS provider and maybe they should be deleted.

The update to KDC means that any test using this library will need to 
run in othervm mode. That may be true already but we should check. 
Related to this is that setting the system property in the main method 
might be fragile in that it's possible for code to execute before the 
main method that triggers the initialization of InetAddress. Something 
to keep in mind as we might have to re-visit these tests again.

-Alan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20160102/46f5cbf7/attachment.htm>


More information about the security-dev mailing list