Repository: netty/netty
Issue: DnsNameResolver should not bind locally
Labels: defect

Netty version: 4.1.1.Final (and master)

Context:
Dns resolution failures when using the `DnsNameResolver` and the JVM is not authorized to bind datagram channels.

Steps to reproduce:
Use the `DnsNameResolver` with an environment that does not allow to bind datagram channels.

The current `DnsNameResolver` binds locally a `DatagramChannel` which is not necessary (and not always permitted).

The bind is used to obtain a channel and it is possible to create a `DatagramChannel` directly without the need to bind. The method `newChannel` in `DnsNameResolver` can be changed from:

```
  private ChannelFuture newChannel(
      ChannelFactory<? extends DatagramChannel> channelFactory, InetSocketAddress localAddress) {

    Bootstrap b = new Bootstrap();
    b.group(executor());
    b.channelFactory(channelFactory);
    final DnsResponseHandler responseHandler = new DnsResponseHandler();
    b.handler(new ChannelInitializer<DatagramChannel>() {
      @Override
      protected void initChannel(DatagramChannel ch) throws Exception {
        ch.pipeline().addLast(DECODER, ENCODER, responseHandler);
      }
    });

    ChannelFuture bindFuture = b.bind(localAddress);
    bindFuture.channel().closeFuture().addListener(new ChannelFutureListener() {
      @Override
      public void operationComplete(ChannelFuture future) throws Exception {
        resolveCache.clear();
      }
    });

    return bindFuture;
  }
```

to

```
  private ChannelFuture newChannel(
      ChannelFactory<? extends DatagramChannel> channelFactory, InetSocketAddress localAddress) {

    DatagramChannel channel = channelFactory.newChannel();
    channel.config().setOption(ChannelOption.DATAGRAM_CHANNEL_ACTIVE_ON_REGISTRATION, true);
    channel.config().setRecvByteBufAllocator(new FixedRecvByteBufAllocator(maxPayloadSize));
    final DnsResponseHandler responseHandler = new DnsResponseHandler();
    channel.pipeline().addLast(DECODER, ENCODER, responseHandler);
    channel.closeFuture().addListener(new ChannelFutureListener() {
      @Override
      public void operationComplete(ChannelFuture future) throws Exception {
        resolveCache.clear();
      }
    });

    return executor().register(channel);
  }
```

to achieve the same without binding.


Issue comments:
@vietj +1 ... Want to provide a pr?

will do soon @normanmaurer 

@vietj thanks a lot!

I'm seing an issue in tests because the channel is not yet initialized on registration, i.e the code:

```
        b.handler(new ChannelInitializer<DatagramChannel>() {
            @Override
            protected void initChannel(DatagramChannel ch) throws Exception {
                ch.pipeline().addLast(DECODER, ENCODER, responseHandler);
            }
        });
b.option(ChannelOption.DATAGRAM_CHANNEL_ACTIVE_ON_REGISTRATION, true);

ChannelFuture channelFuture = b.register();
```

does not guarantee that the `initChannel` is invoked when the `channelFuture` is succeeded. As consequence sometimes during tests the `DatagramDnsQuery` are not encoded as the `ENCODER` is not yet added to the pipeline.

One solution is to use a promise that is resolved after the pipeline has been modified instead of using the channelFuture.

Thoughts ?

@vietj sounds good.

@vietj I think even better would be to have the promise be notified once channelActive is called.

Fixed by https://github.com/netty/netty/pull/5461


Pull request review comments:
woups

@vietj make the constructor package private


Patch that resolved the issue:
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolverGroup.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolverGroup.java
index 26f99905e47..27f7d2ab011 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolverGroup.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolverGroup.java
@@ -34,7 +34,6 @@
 import java.util.List;
 import java.util.concurrent.ConcurrentMap;
 
-import static io.netty.resolver.dns.DnsNameResolver.ANY_LOCAL_ADDR;
 import static io.netty.util.internal.PlatformDependent.newConcurrentHashMap;
 
 /**
@@ -44,33 +43,21 @@
 public class DnsAddressResolverGroup extends AddressResolverGroup<InetSocketAddress> {
 
     private final ChannelFactory<? extends DatagramChannel> channelFactory;
-    private final InetSocketAddress localAddress;
     private final DnsServerAddresses nameServerAddresses;
 
     private final ConcurrentMap<String, Promise<InetAddress>> resolvesInProgress = newConcurrentHashMap();
     private final ConcurrentMap<String, Promise<List<InetAddress>>> resolveAllsInProgress = newConcurrentHashMap();
 
-    public DnsAddressResolverGroup(
-            Class<? extends DatagramChannel> channelType, DnsServerAddresses nameServerAddresses) {
-        this(channelType, ANY_LOCAL_ADDR, nameServerAddresses);
-    }
-
     public DnsAddressResolverGroup(
             Class<? extends DatagramChannel> channelType,
-            InetSocketAddress localAddress, DnsServerAddresses nameServerAddresses) {
-        this(new ReflectiveChannelFactory<DatagramChannel>(channelType), localAddress, nameServerAddresses);
-    }
-
-    public DnsAddressResolverGroup(
-            ChannelFactory<? extends DatagramChannel> channelFactory, DnsServerAddresses nameServerAddresses) {
-        this(channelFactory, ANY_LOCAL_ADDR, nameServerAddresses);
+            DnsServerAddresses nameServerAddresses) {
+        this(new ReflectiveChannelFactory<DatagramChannel>(channelType), nameServerAddresses);
     }
 
     public DnsAddressResolverGroup(
             ChannelFactory<? extends DatagramChannel> channelFactory,
-            InetSocketAddress localAddress, DnsServerAddresses nameServerAddresses) {
+            DnsServerAddresses nameServerAddresses) {
         this.channelFactory = channelFactory;
-        this.localAddress = localAddress;
         this.nameServerAddresses = nameServerAddresses;
     }
 
@@ -83,20 +70,20 @@ protected final AddressResolver<InetSocketAddress> newResolver(EventExecutor exe
                     " (expected: " + StringUtil.simpleClassName(EventLoop.class));
         }
 
-        return newResolver((EventLoop) executor, channelFactory, localAddress, nameServerAddresses);
+        return newResolver((EventLoop) executor, channelFactory, nameServerAddresses);
     }
 
     /**
-     * @deprecated Override {@link #newNameResolver(EventLoop, ChannelFactory, InetSocketAddress, DnsServerAddresses)}.
+     * @deprecated Override {@link #newNameResolver(EventLoop, ChannelFactory, DnsServerAddresses)}.
      */
     @Deprecated
     protected AddressResolver<InetSocketAddress> newResolver(
             EventLoop eventLoop, ChannelFactory<? extends DatagramChannel> channelFactory,
-            InetSocketAddress localAddress, DnsServerAddresses nameServerAddresses) throws Exception {
+            DnsServerAddresses nameServerAddresses) throws Exception {
 
         final NameResolver<InetAddress> resolver = new InflightNameResolver<InetAddress>(
                 eventLoop,
-                newNameResolver(eventLoop, channelFactory, localAddress, nameServerAddresses),
+                newNameResolver(eventLoop, channelFactory, nameServerAddresses),
                 resolvesInProgress,
                 resolveAllsInProgress);
 
@@ -109,11 +96,9 @@ protected AddressResolver<InetSocketAddress> newResolver(
      */
     protected NameResolver<InetAddress> newNameResolver(EventLoop eventLoop,
                                                         ChannelFactory<? extends DatagramChannel> channelFactory,
-                                                        InetSocketAddress localAddress,
                                                         DnsServerAddresses nameServerAddresses) throws Exception {
         return new DnsNameResolverBuilder(eventLoop)
                 .channelFactory(channelFactory)
-                .localAddress(localAddress)
                 .nameServerAddresses(nameServerAddresses)
                 .build();
     }
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 9fed94cbf1c..1fc557ac078 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
@@ -17,12 +17,14 @@
 
 import io.netty.bootstrap.Bootstrap;
 import io.netty.channel.AddressedEnvelope;
+import io.netty.channel.Channel;
 import io.netty.channel.ChannelFactory;
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelFutureListener;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInboundHandlerAdapter;
 import io.netty.channel.ChannelInitializer;
+import io.netty.channel.ChannelOption;
 import io.netty.channel.EventLoop;
 import io.netty.channel.FixedRecvByteBufAllocator;
 import io.netty.channel.socket.DatagramChannel;
@@ -65,8 +67,6 @@ public class DnsNameResolver extends InetNameResolver {
     private static final String LOCALHOST = "localhost";
     private static final InetAddress LOCALHOST_ADDRESS;
 
-    static final InetSocketAddress ANY_LOCAL_ADDR = new InetSocketAddress(0);
-
     static final InternetProtocolFamily[] DEFAULT_RESOLVE_ADDRESS_TYPES = new InternetProtocolFamily[2];
 
     static {
@@ -88,7 +88,7 @@ public class DnsNameResolver extends InetNameResolver {
     private static final DatagramDnsQueryEncoder ENCODER = new DatagramDnsQueryEncoder();
 
     final DnsServerAddresses nameServerAddresses;
-    final ChannelFuture bindFuture;
+    final Future<Channel> channelFuture;
     final DatagramChannel ch;
 
     /**
@@ -123,7 +123,6 @@ protected DnsServerAddressStream initialValue() throws Exception {
      *
      * @param eventLoop the {@link EventLoop} which will perform the communication with the DNS servers
      * @param channelFactory the {@link ChannelFactory} that will create a {@link DatagramChannel}
-     * @param localAddress the local address of the {@link DatagramChannel}
      * @param nameServerAddresses the addresses of the DNS server. For each DNS query, a new stream is created from
      *                            this to determine which DNS server should be contacted for the next retry in case
      *                            of failure.
@@ -140,9 +139,8 @@ protected DnsServerAddressStream initialValue() throws Exception {
     public DnsNameResolver(
             EventLoop eventLoop,
             ChannelFactory<? extends DatagramChannel> channelFactory,
-            InetSocketAddress localAddress,
             DnsServerAddresses nameServerAddresses,
-            DnsCache resolveCache,
+            final DnsCache resolveCache,
             long queryTimeoutMillis,
             InternetProtocolFamily[] resolvedAddressTypes,
             boolean recursionDesired,
@@ -154,7 +152,6 @@ public DnsNameResolver(
 
         super(eventLoop);
         checkNotNull(channelFactory, "channelFactory");
-        checkNotNull(localAddress, "localAddress");
         this.nameServerAddresses = checkNotNull(nameServerAddresses, "nameServerAddresses");
         this.queryTimeoutMillis = checkPositive(queryTimeoutMillis, "queryTimeoutMillis");
         this.resolvedAddressTypes = checkNonEmpty(resolvedAddressTypes, "resolvedAddressTypes");
@@ -166,18 +163,11 @@ public DnsNameResolver(
         this.hostsFileEntriesResolver = checkNotNull(hostsFileEntriesResolver, "hostsFileEntriesResolver");
         this.resolveCache = resolveCache;
 
-        bindFuture = newChannel(channelFactory, localAddress);
-        ch = (DatagramChannel) bindFuture.channel();
-        ch.config().setRecvByteBufAllocator(new FixedRecvByteBufAllocator(maxPayloadSize));
-    }
-
-    private ChannelFuture newChannel(
-            ChannelFactory<? extends DatagramChannel> channelFactory, InetSocketAddress localAddress) {
-
         Bootstrap b = new Bootstrap();
         b.group(executor());
         b.channelFactory(channelFactory);
-        final DnsResponseHandler responseHandler = new DnsResponseHandler();
+        b.option(ChannelOption.DATAGRAM_CHANNEL_ACTIVE_ON_REGISTRATION, true);
+        final DnsResponseHandler responseHandler = new DnsResponseHandler(executor().<Channel>newPromise());
         b.handler(new ChannelInitializer<DatagramChannel>() {
             @Override
             protected void initChannel(DatagramChannel ch) throws Exception {
@@ -185,15 +175,16 @@ protected void initChannel(DatagramChannel ch) throws Exception {
             }
         });
 
-        ChannelFuture bindFuture = b.bind(localAddress);
-        bindFuture.channel().closeFuture().addListener(new ChannelFutureListener() {
+        channelFuture = responseHandler.channelActivePromise;
+        ch = (DatagramChannel) b.register().channel();
+        ch.config().setRecvByteBufAllocator(new FixedRecvByteBufAllocator(maxPayloadSize));
+
+        ch.closeFuture().addListener(new ChannelFutureListener() {
             @Override
             public void operationComplete(ChannelFuture future) throws Exception {
                 resolveCache.clear();
             }
         });
-
-        return bindFuture;
     }
 
     /**
@@ -606,6 +597,13 @@ private static Promise<AddressedEnvelope<DnsResponse, InetSocketAddress>> cast(P
     }
 
     private final class DnsResponseHandler extends ChannelInboundHandlerAdapter {
+
+        private final Promise<Channel> channelActivePromise;
+
+        DnsResponseHandler(Promise<Channel> channelActivePromise) {
+            this.channelActivePromise = channelActivePromise;
+        }
+
         @Override
         public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
             try {
@@ -628,6 +626,12 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
             }
         }
 
+        @Override
+        public void channelActive(ChannelHandlerContext ctx) throws Exception {
+            super.channelActive(ctx);
+            channelActivePromise.setSuccess(ctx.channel());
+        }
+
         @Override
         public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
             logger.warn("{} Unexpected exception: ", ch, cause);
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java
index 6e9a56f3bb7..f6887e69dfc 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolverBuilder.java
@@ -26,7 +26,6 @@
 import io.netty.util.internal.InternalThreadLocalMap;
 import io.netty.util.internal.UnstableApi;
 
-import java.net.InetSocketAddress;
 import java.util.List;
 
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
@@ -39,7 +38,6 @@ public final class DnsNameResolverBuilder {
 
     private final EventLoop eventLoop;
     private ChannelFactory<? extends DatagramChannel> channelFactory;
-    private InetSocketAddress localAddress = DnsNameResolver.ANY_LOCAL_ADDR;
     private DnsServerAddresses nameServerAddresses = DnsServerAddresses.defaultAddresses();
     private DnsCache resolveCache;
     private Integer minTtl;
@@ -86,17 +84,6 @@ public DnsNameResolverBuilder channelType(Class<? extends DatagramChannel> chann
         return channelFactory(new ReflectiveChannelFactory<DatagramChannel>(channelType));
     }
 
-    /**
-     * Sets the local address of the {@link DatagramChannel}
-     *
-     * @param localAddress the local address
-     * @return {@code this}
-     */
-    public DnsNameResolverBuilder localAddress(InetSocketAddress localAddress) {
-        this.localAddress = localAddress;
-        return this;
-    }
-
     /**
      * Sets the addresses of the DNS server.
      *
@@ -318,7 +305,6 @@ public DnsNameResolver build() {
         return new DnsNameResolver(
                 eventLoop,
                 channelFactory,
-                localAddress,
                 nameServerAddresses,
                 cache,
                 queryTimeoutMillis,
diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsQueryContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsQueryContext.java
index ae72b09545f..897d2f3b40e 100644
--- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsQueryContext.java
+++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsQueryContext.java
@@ -17,6 +17,7 @@
 
 import io.netty.buffer.Unpooled;
 import io.netty.channel.AddressedEnvelope;
+import io.netty.channel.Channel;
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelFutureListener;
 import io.netty.handler.codec.dns.DatagramDnsQuery;
@@ -27,6 +28,8 @@
 import io.netty.handler.codec.dns.DnsRecordType;
 import io.netty.handler.codec.dns.DnsResponse;
 import io.netty.handler.codec.dns.DnsSection;
+import io.netty.util.concurrent.Future;
+import io.netty.util.concurrent.GenericFutureListener;
 import io.netty.util.concurrent.Promise;
 import io.netty.util.concurrent.ScheduledFuture;
 import io.netty.util.internal.StringUtil;
@@ -107,12 +110,12 @@ void query() {
     }
 
     private void sendQuery(final DnsQuery query) {
-        if (parent.bindFuture.isDone()) {
+        if (parent.channelFuture.isDone()) {
             writeQuery(query);
         } else {
-            parent.bindFuture.addListener(new ChannelFutureListener() {
+            parent.channelFuture.addListener(new GenericFutureListener<Future<? super Channel>>() {
                 @Override
-                public void operationComplete(ChannelFuture future) throws Exception {
+                public void operationComplete(Future<? super Channel> future) throws Exception {
                     if (future.isSuccess()) {
                         writeQuery(query);
                     } else {


Test patch: