From 6c8b770a98aab5d1ac138f7a1f56291240f06b63 Mon Sep 17 00:00:00 2001
From: Luke Huang <huangluke@google.com>
Date: Tue, 11 Jun 2019 14:25:45 +0800
Subject: [PATCH] Fix race condition caused by fd reused for DnsResolver

There might be a gap between fd close and fd event listener unregister.
If fd is reused for another query during that gap, it might cause the
query failed with no response since addOnFileDescriptorEventListener
method failed. To fix this problem, we must ensure that fd event
listener is unregistered before fd closing.

Bug: 134310704
Test: atest DnsResolverTest
Change-Id: Ifb6bd34dc54dcf1c61fe8b87785124df4bc0f410
---
 core/java/android/net/DnsResolver.java | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/core/java/android/net/DnsResolver.java b/core/java/android/net/DnsResolver.java
index 7a85dcbfc8..0b1a84534e 100644
--- a/core/java/android/net/DnsResolver.java
+++ b/core/java/android/net/DnsResolver.java
@@ -34,6 +34,7 @@ import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.os.CancellationSignal;
 import android.os.Looper;
+import android.os.MessageQueue;
 import android.system.ErrnoException;
 import android.util.Log;
 
@@ -466,10 +467,20 @@ public final class DnsResolver {
     private void registerFDListener(@NonNull Executor executor,
             @NonNull FileDescriptor queryfd, @NonNull Callback<? super byte[]> answerCallback,
             @Nullable CancellationSignal cancellationSignal, @NonNull Object lock) {
-        Looper.getMainLooper().getQueue().addOnFileDescriptorEventListener(
+        final MessageQueue mainThreadMessageQueue = Looper.getMainLooper().getQueue();
+        mainThreadMessageQueue.addOnFileDescriptorEventListener(
                 queryfd,
                 FD_EVENTS,
                 (fd, events) -> {
+                    // b/134310704
+                    // Unregister fd event listener before resNetworkResult is called to prevent
+                    // race condition caused by fd reused.
+                    // For example when querying v4 and v6, it's possible that the first query ends
+                    // and the fd is closed before the second request starts, which might return
+                    // the same fd for the second request. By that time, the looper must have
+                    // unregistered the fd, otherwise another event listener can't be registered.
+                    mainThreadMessageQueue.removeOnFileDescriptorEventListener(fd);
+
                     executor.execute(() -> {
                         DnsResponse resp = null;
                         ErrnoException exception = null;
@@ -490,7 +501,11 @@ public final class DnsResolver {
                         }
                         answerCallback.onAnswer(resp.answerbuf, resp.rcode);
                     });
-                    // Unregister this fd listener
+
+                    // The file descriptor has already been unregistered, so it does not really
+                    // matter what is returned here. In spirit 0 (meaning "unregister this FD")
+                    // is still the closest to what the looper needs to do. When returning 0,
+                    // Looper knows to ignore the fd if it has already been unregistered.
                     return 0;
                 });
     }