From a416b79d865fd6ce11ecce74d53ce2dfd8be007f Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 20 Jan 2017 15:46:46 +0100 Subject: [PATCH] DnsNameResolver.resolve*(...) never notifies the Future when empty hostname is used. Motivation: When an empty hostname is used in DnsNameResolver.resolve*(...) it will never notify the future / promise. The root cause is that we not correctly guard against errors of IDN.toASCII(...) which will throw an IllegalArgumentException when it can not parse its input. That said we should also handle an empty hostname the same way as the JDK does and just use "localhost" when this happens. Modifications: - If the try to resolve an empty hostname we use localhost - Correctly guard against errors raised by IDN.toASCII(...) so we will always noify the future / promise - Add unit test. Result: DnsNameResolver.resolve*(...) will always notify the future. --- .../io/netty/util/internal/SocketUtils.java | 16 ++++ pom.xml | 3 + .../netty/resolver/dns/DnsNameResolver.java | 18 +++- .../resolver/dns/DnsNameResolverContext.java | 25 ++++-- .../resolver/dns/DnsNameResolverTest.java | 62 +++++++++++++ .../io/netty/resolver/InetNameResolver.java | 32 +++++++ .../netty/resolver/InetNameResolverTest.java | 90 +++++++++++++++++++ 7 files changed, 239 insertions(+), 7 deletions(-) create mode 100644 resolver/src/test/java/io/netty/resolver/InetNameResolverTest.java diff --git a/common/src/main/java/io/netty/util/internal/SocketUtils.java b/common/src/main/java/io/netty/util/internal/SocketUtils.java index 6d566d7d5c..63fb742651 100644 --- a/common/src/main/java/io/netty/util/internal/SocketUtils.java +++ b/common/src/main/java/io/netty/util/internal/SocketUtils.java @@ -182,6 +182,22 @@ public final class SocketUtils { }); } + public static InetAddress loopbackAddress() { + return AccessController.doPrivileged(new PrivilegedAction() { + @Override + public InetAddress run() { + if (PlatformDependent.javaVersion() >= 7) { + return InetAddress.getLoopbackAddress(); + } + try { + return InetAddress.getByName(null); + } catch (UnknownHostException e) { + throw new IllegalStateException(e); + } + } + }); + } + public static byte[] hardwareAddressFromNetworkInterface(final NetworkInterface intf) throws SocketException { try { return AccessController.doPrivileged(new PrivilegedExceptionAction() { diff --git a/pom.xml b/pom.xml index 6710c9d2b0..e0aea5700c 100644 --- a/pom.xml +++ b/pom.xml @@ -757,6 +757,9 @@ java.nio.ByteBuffer java.nio.CharBuffer + + + java.net.InetAddress 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 c0f88cbcbf..fdbf449cf5 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 @@ -411,8 +411,11 @@ public class DnsNameResolver extends InetNameResolver { */ public final Future resolve(String inetHost, Iterable additionals, Promise promise) { - checkNotNull(inetHost, "inetHost"); 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); @@ -445,8 +448,13 @@ public class DnsNameResolver extends InetNameResolver { */ public final Future> resolveAll(String inetHost, Iterable additionals, Promise> promise) { - checkNotNull(inetHost, "inetHost"); 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); @@ -492,6 +500,12 @@ public class DnsNameResolver extends InetNameResolver { } } + @Override + protected final InetAddress loopbackAddress() { + return preferredAddressType() == InternetProtocolFamily.IPv4 ? + NetUtil.LOCALHOST4 : NetUtil.LOCALHOST6; + } + /** * Hook designed for extensibility so one can pass a different cache on each resolution attempt * instead of using the global one. diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java index 68905cd7fa..41d7b72c63 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverContext.java @@ -146,7 +146,9 @@ abstract class DnsNameResolverContext { private void internalResolve(Promise promise) { InetSocketAddress nameServerAddrToTry = nameServerAddrs.next(); for (DnsRecordType type: parent.resolveRecordTypes()) { - query(nameServerAddrToTry, new DefaultDnsQuestion(hostname, type), promise); + if (!query(hostname, type, nameServerAddrToTry, promise)) { + return; + } } } @@ -382,7 +384,7 @@ abstract class DnsNameResolverContext { if (!triedCNAME) { // As the last resort, try to query CNAME, just in case the name server has it. triedCNAME = true; - query(nameServerAddrs.next(), new DefaultDnsQuestion(hostname, DnsRecordType.CNAME), promise); + query(hostname, DnsRecordType.CNAME, nameServerAddrs.next(), promise); return; } } @@ -509,14 +511,27 @@ abstract class DnsNameResolverContext { } final InetSocketAddress nextAddr = nameServerAddrs.next(); - if (parent.isCnameFollowARecords()) { - query(nextAddr, new DefaultDnsQuestion(cname, DnsRecordType.A), promise); + if (parent.isCnameFollowARecords() && !query(hostname, DnsRecordType.A, nextAddr, promise)) { + return; } if (parent.isCnameFollowAAAARecords()) { - query(nextAddr, new DefaultDnsQuestion(cname, DnsRecordType.AAAA), promise); + query(hostname, DnsRecordType.AAAA, nextAddr, promise); } } + private boolean query(String hostname, DnsRecordType type, final InetSocketAddress nextAddr, Promise promise) { + final DnsQuestion question; + try { + question = new DefaultDnsQuestion(hostname, type); + } catch (IllegalArgumentException e) { + // java.net.IDN.toASCII(...) may throw an IllegalArgumentException if it fails to parse the hostname + promise.tryFailure(e); + return false; + } + query(nextAddr, question, promise); + return true; + } + private void addTrace(InetSocketAddress nameServerAddr, String msg) { assert traceEnabled; 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 9ccfd46a48..b27c6aa36e 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 @@ -28,6 +28,7 @@ import io.netty.handler.codec.dns.DnsRecordType; import io.netty.handler.codec.dns.DnsResponse; import io.netty.handler.codec.dns.DnsResponseCode; import io.netty.handler.codec.dns.DnsSection; +import io.netty.util.NetUtil; import io.netty.util.concurrent.Future; import io.netty.util.internal.StringUtil; import io.netty.util.internal.logging.InternalLogger; @@ -496,6 +497,67 @@ public class DnsNameResolverTest { } } + @Test + public void testResolveEmptyIpv4() { + testResolve0(InternetProtocolFamily.IPv4, NetUtil.LOCALHOST4, StringUtil.EMPTY_STRING); + } + + @Test + public void testResolveEmptyIpv6() { + testResolve0(InternetProtocolFamily.IPv6, NetUtil.LOCALHOST6, StringUtil.EMPTY_STRING); + } + + @Test + public void testResolveNullIpv4() { + testResolve0(InternetProtocolFamily.IPv4, NetUtil.LOCALHOST4, null); + } + + @Test + public void testResolveNullIpv6() { + testResolve0(InternetProtocolFamily.IPv6, NetUtil.LOCALHOST6, null); + } + + private static void testResolve0(InternetProtocolFamily family, InetAddress expectedAddr, String name) { + DnsNameResolver resolver = newResolver(family).build(); + try { + InetAddress address = resolver.resolve(name).syncUninterruptibly().getNow(); + assertEquals(expectedAddr, address); + } finally { + resolver.close(); + } + } + + @Test + public void testResolveAllEmptyIpv4() { + testResolveAll0(InternetProtocolFamily.IPv4, NetUtil.LOCALHOST4, StringUtil.EMPTY_STRING); + } + + @Test + public void testResolveAllEmptyIpv6() { + testResolveAll0(InternetProtocolFamily.IPv6, NetUtil.LOCALHOST6, StringUtil.EMPTY_STRING); + } + + @Test + public void testResolveAllNullIpv4() { + testResolveAll0(InternetProtocolFamily.IPv4, NetUtil.LOCALHOST4, null); + } + + @Test + public void testResolveAllNullIpv6() { + testResolveAll0(InternetProtocolFamily.IPv6, NetUtil.LOCALHOST6, null); + } + + private static void testResolveAll0(InternetProtocolFamily family, InetAddress expectedAddr, String name) { + DnsNameResolver resolver = newResolver(family).build(); + try { + List addresses = resolver.resolveAll(name).syncUninterruptibly().getNow(); + assertEquals(1, addresses.size()); + assertEquals(expectedAddr, addresses.get(0)); + } finally { + resolver.close(); + } + } + private static void resolve(DnsNameResolver resolver, Map> futures, String hostname) { futures.put(hostname, resolver.resolve(hostname)); } diff --git a/resolver/src/main/java/io/netty/resolver/InetNameResolver.java b/resolver/src/main/java/io/netty/resolver/InetNameResolver.java index c0ca21c72c..dc07e799a6 100644 --- a/resolver/src/main/java/io/netty/resolver/InetNameResolver.java +++ b/resolver/src/main/java/io/netty/resolver/InetNameResolver.java @@ -17,15 +17,21 @@ 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 java.net.InetAddress; import java.net.InetSocketAddress; +import java.util.Collections; +import java.util.List; /** * A skeletal {@link NameResolver} implementation that resolves {@link InetAddress}. */ public abstract class InetNameResolver extends SimpleNameResolver { + private final InetAddress loopbackAddress; + private volatile AddressResolver addressResolver; /** @@ -34,6 +40,32 @@ 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/test/java/io/netty/resolver/InetNameResolverTest.java b/resolver/src/test/java/io/netty/resolver/InetNameResolverTest.java new file mode 100644 index 0000000000..6ef72c52e0 --- /dev/null +++ b/resolver/src/test/java/io/netty/resolver/InetNameResolverTest.java @@ -0,0 +1,90 @@ +/*@ + * 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()); + } + } +}