From 007048dddda0eb922952f8b07ea2521d9a5eadef Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Tue, 7 Feb 2017 14:09:55 -0800 Subject: [PATCH] DnsNameResolver empty/null hostname missed by a416b79 Motivation: a416b79 introduced a check for null or empty host name to be compatible with the JDK resolution. However the doResolve(String, Promise) method, and if the doResolve(String, DnsRecord[], Promise, DnsCache) method was overridden the empty/null hostname would not be correctly resolved. Modifications: - Move the empty/null host name check into the lowest level doResolve method in DnsNameResolver - Remove the duplicate logic in InetNameResolver.java which can be bypassed anyways Result: By default (unless behavior is overridden) DnsNameResolver resolves null/empty host names to local host just like the JDK. --- .../netty/resolver/dns/DnsNameResolver.java | 32 +++---- .../resolver/dns/DnsNameResolverTest.java | 42 +++++++++ .../io/netty/resolver/InetNameResolver.java | 35 +------- .../io/netty/resolver/SimpleNameResolver.java | 2 - .../netty/resolver/InetNameResolverTest.java | 90 ------------------- 5 files changed, 61 insertions(+), 140 deletions(-) delete mode 100644 resolver/src/test/java/io/netty/resolver/InetNameResolverTest.java diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java index b61e7aa6d4..fcc7417f9f 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java @@ -31,10 +31,10 @@ import io.netty.channel.socket.DatagramChannel; import io.netty.channel.socket.InternetProtocolFamily; import io.netty.handler.codec.dns.DatagramDnsQueryEncoder; import io.netty.handler.codec.dns.DatagramDnsResponse; -import io.netty.handler.codec.dns.DnsRawRecord; -import io.netty.handler.codec.dns.DnsRecord; import io.netty.handler.codec.dns.DatagramDnsResponseDecoder; import io.netty.handler.codec.dns.DnsQuestion; +import io.netty.handler.codec.dns.DnsRawRecord; +import io.netty.handler.codec.dns.DnsRecord; import io.netty.handler.codec.dns.DnsRecordType; import io.netty.handler.codec.dns.DnsResponse; import io.netty.resolver.HostsFileEntriesResolver; @@ -64,7 +64,10 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Set; -import static io.netty.util.internal.ObjectUtil.*; +import static io.netty.util.internal.ObjectUtil.checkNonEmpty; +import static io.netty.util.internal.ObjectUtil.checkNotNull; +import static io.netty.util.internal.ObjectUtil.checkPositive; +import static io.netty.util.internal.ObjectUtil.checkPositiveOrZero; /** * A DNS-based {@link InetNameResolver}. @@ -479,10 +482,6 @@ public class DnsNameResolver extends InetNameResolver { public final Future resolve(String inetHost, Iterable additionals, Promise promise) { checkNotNull(promise, "promise"); - if (inetHost == null || inetHost.isEmpty()) { - // If an empty hostname is used we should use "localhost", just like InetAddress.getByName(...) does. - return promise.setSuccess(loopbackAddress()); - } DnsRecord[] additionalsArray = toArray(additionals, true); try { doResolve(inetHost, additionalsArray, promise, resolveCache); @@ -516,12 +515,6 @@ public class DnsNameResolver extends InetNameResolver { public final Future> resolveAll(String inetHost, Iterable additionals, Promise> promise) { checkNotNull(promise, "promise"); - - if (inetHost == null || inetHost.isEmpty()) { - // If an empty hostname is used we should use "localhost", just like InetAddress.getAllByName(...) does. - return promise.setSuccess(Collections.singletonList(loopbackAddress())); - } - DnsRecord[] additionalsArray = toArray(additionals, true); try { doResolveAll(inetHost, additionalsArray, promise, resolveCache); @@ -567,8 +560,7 @@ public class DnsNameResolver extends InetNameResolver { } } - @Override - protected final InetAddress loopbackAddress() { + private InetAddress loopbackAddress() { return preferredAddressType() == InternetProtocolFamily.IPv4 ? NetUtil.LOCALHOST4 : NetUtil.LOCALHOST6; } @@ -581,6 +573,11 @@ public class DnsNameResolver extends InetNameResolver { DnsRecord[] additionals, Promise promise, DnsCache resolveCache) throws Exception { + if (inetHost == null || inetHost.isEmpty()) { + // If an empty hostname is used we should use "localhost", just like InetAddress.getByName(...) does. + promise.setSuccess(loopbackAddress()); + return; + } final byte[] bytes = NetUtil.createByteArrayFromIpAddressString(inetHost); if (bytes != null) { // The inetHost is actually an ipaddress. @@ -706,6 +703,11 @@ public class DnsNameResolver extends InetNameResolver { DnsRecord[] additionals, Promise> promise, DnsCache resolveCache) throws Exception { + if (inetHost == null || inetHost.isEmpty()) { + // If an empty hostname is used we should use "localhost", just like InetAddress.getAllByName(...) does. + promise.setSuccess(Collections.singletonList(loopbackAddress())); + return; + } final byte[] bytes = NetUtil.createByteArrayFromIpAddressString(inetHost); if (bytes != null) { // The unresolvedAddress was created via a String that contains an ipaddress. diff --git a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java index 693685049d..ddd86b3597 100644 --- a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java +++ b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java @@ -34,6 +34,7 @@ import io.netty.handler.codec.dns.DnsSection; import io.netty.util.NetUtil; import io.netty.resolver.HostsFileEntriesResolver; import io.netty.util.concurrent.Future; +import io.netty.util.internal.SocketUtils; import io.netty.util.internal.StringUtil; import io.netty.util.internal.logging.InternalLogger; import io.netty.util.internal.logging.InternalLoggerFactory; @@ -374,6 +375,47 @@ public class DnsNameResolverTest { } } + @Test(timeout = 5000) + public void testNonCachedResolveEmptyHostName() throws Exception { + testNonCachedResolveEmptyHostName(""); + } + + @Test(timeout = 5000) + public void testNonCachedResolveNullHostName() throws Exception { + testNonCachedResolveEmptyHostName(null); + } + + public void testNonCachedResolveEmptyHostName(String inetHost) throws Exception { + DnsNameResolver resolver = newNonCachedResolver(InternetProtocolFamily.IPv4).build(); + try { + InetAddress addr = resolver.resolve(inetHost).syncUninterruptibly().getNow(); + assertEquals(SocketUtils.addressByName(inetHost), addr); + } finally { + resolver.close(); + } + } + + @Test(timeout = 5000) + public void testNonCachedResolveAllEmptyHostName() throws Exception { + testNonCachedResolveAllEmptyHostName(""); + } + + @Test(timeout = 5000) + public void testNonCachedResolveAllNullHostName() throws Exception { + testNonCachedResolveAllEmptyHostName(null); + } + + private static void testNonCachedResolveAllEmptyHostName(String inetHost) throws UnknownHostException { + DnsNameResolver resolver = newNonCachedResolver(InternetProtocolFamily.IPv4).build(); + try { + List addrs = resolver.resolveAll(inetHost).syncUninterruptibly().getNow(); + assertEquals(Arrays.asList( + SocketUtils.allAddressesByName(inetHost)), addrs); + } finally { + resolver.close(); + } + } + private static Map testResolve0(DnsNameResolver resolver, Set excludedDomains) throws InterruptedException { diff --git a/resolver/src/main/java/io/netty/resolver/InetNameResolver.java b/resolver/src/main/java/io/netty/resolver/InetNameResolver.java index dc07e799a6..eb5cfe092a 100644 --- a/resolver/src/main/java/io/netty/resolver/InetNameResolver.java +++ b/resolver/src/main/java/io/netty/resolver/InetNameResolver.java @@ -17,21 +17,16 @@ package io.netty.resolver; import io.netty.util.concurrent.EventExecutor; import io.netty.util.concurrent.Future; -import io.netty.util.concurrent.Promise; -import io.netty.util.internal.SocketUtils; +import io.netty.util.internal.UnstableApi; import java.net.InetAddress; import java.net.InetSocketAddress; -import java.util.Collections; -import java.util.List; /** * A skeletal {@link NameResolver} implementation that resolves {@link InetAddress}. */ +@UnstableApi public abstract class InetNameResolver extends SimpleNameResolver { - - private final InetAddress loopbackAddress; - private volatile AddressResolver addressResolver; /** @@ -40,32 +35,6 @@ public abstract class InetNameResolver extends SimpleNameResolver { */ protected InetNameResolver(EventExecutor executor) { super(executor); - loopbackAddress = SocketUtils.loopbackAddress(); - } - - /** - * Returns the {@link InetAddress} for loopback. - */ - protected InetAddress loopbackAddress() { - return loopbackAddress; - } - - @Override - public Future resolve(String inetHost, Promise promise) { - if (inetHost == null || inetHost.isEmpty()) { - // If an empty hostname is used we should use "localhost", just like InetAddress.getByName(...) does. - return promise.setSuccess(loopbackAddress()); - } - return super.resolve(inetHost, promise); - } - - @Override - public Future> resolveAll(String inetHost, Promise> promise) { - if (inetHost == null || inetHost.isEmpty()) { - // If an empty hostname is used we should use "localhost", just like InetAddress.getByName(...) does. - return promise.setSuccess(Collections.singletonList(loopbackAddress())); - } - return super.resolveAll(inetHost, promise); } /** diff --git a/resolver/src/main/java/io/netty/resolver/SimpleNameResolver.java b/resolver/src/main/java/io/netty/resolver/SimpleNameResolver.java index e5083975ff..c8af37a4e3 100644 --- a/resolver/src/main/java/io/netty/resolver/SimpleNameResolver.java +++ b/resolver/src/main/java/io/netty/resolver/SimpleNameResolver.java @@ -55,7 +55,6 @@ public abstract class SimpleNameResolver implements NameResolver { @Override public Future resolve(String inetHost, Promise promise) { - checkNotNull(inetHost, "inetHost"); checkNotNull(promise, "promise"); try { @@ -74,7 +73,6 @@ public abstract class SimpleNameResolver implements NameResolver { @Override public Future> resolveAll(String inetHost, Promise> promise) { - checkNotNull(inetHost, "inetHost"); checkNotNull(promise, "promise"); try { diff --git a/resolver/src/test/java/io/netty/resolver/InetNameResolverTest.java b/resolver/src/test/java/io/netty/resolver/InetNameResolverTest.java deleted file mode 100644 index 6ef72c52e0..0000000000 --- a/resolver/src/test/java/io/netty/resolver/InetNameResolverTest.java +++ /dev/null @@ -1,90 +0,0 @@ -/*@ - * Copyright 2017 The Netty Project - * - * The Netty Project licenses this file to you under the Apache License, - * version 2.0 (the "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at: - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations - * under the License. - */ -package io.netty.resolver; - -import io.netty.util.concurrent.ImmediateEventExecutor; -import io.netty.util.concurrent.Promise; -import io.netty.util.internal.SocketUtils; -import io.netty.util.internal.StringUtil; -import org.junit.Test; - -import java.net.InetAddress; -import java.net.UnknownHostException; -import java.util.Arrays; -import java.util.List; - -import static org.junit.Assert.assertEquals; - -public class InetNameResolverTest { - - @Test - public void testResolveEmpty() throws UnknownHostException { - testResolve0(SocketUtils.addressByName(StringUtil.EMPTY_STRING), StringUtil.EMPTY_STRING); - } - - @Test - public void testResolveNull() throws UnknownHostException { - testResolve0(SocketUtils.addressByName(null), null); - } - - @Test - public void testResolveAllEmpty() throws UnknownHostException { - testResolveAll0(Arrays.asList( - SocketUtils.allAddressesByName(StringUtil.EMPTY_STRING)), StringUtil.EMPTY_STRING); - } - - @Test - public void testResolveAllNull() throws UnknownHostException { - testResolveAll0(Arrays.asList( - SocketUtils.allAddressesByName(null)), null); - } - - private static void testResolve0(InetAddress expectedAddr, String name) { - InetNameResolver resolver = new TestInetNameResolver(); - try { - InetAddress address = resolver.resolve(name).syncUninterruptibly().getNow(); - assertEquals(expectedAddr, address); - } finally { - resolver.close(); - } - } - - private static void testResolveAll0(List expectedAddrs, String name) { - InetNameResolver resolver = new TestInetNameResolver(); - try { - List addresses = resolver.resolveAll(name).syncUninterruptibly().getNow(); - assertEquals(expectedAddrs, addresses); - } finally { - resolver.close(); - } - } - - private static final class TestInetNameResolver extends InetNameResolver { - public TestInetNameResolver() { - super(ImmediateEventExecutor.INSTANCE); - } - - @Override - protected void doResolve(String inetHost, Promise promise) throws Exception { - promise.setFailure(new UnsupportedOperationException()); - } - - @Override - protected void doResolveAll(String inetHost, Promise> promise) throws Exception { - promise.setFailure(new UnsupportedOperationException()); - } - } -}