From e17bb2dff47c5769e17262877e71f75e12c24890 Mon Sep 17 00:00:00 2001
From: Luke Huang <huangluke@google.com>
Date: Tue, 26 Mar 2019 15:50:10 +0800
Subject: [PATCH] Fix cancellation race problem for aysnc DNS API

This problem might cause double-close fd and result in app crash
or unexpected behaviour

Bug: 129317069
Test: atest DnsResolverTest
      manual test with delaying response callback/cancel
Change-Id: I223234f527edafc51d34fa6be390419c05def8d8
---
 core/java/android/net/DnsResolver.java | 60 +++++++++++++++-----------
 core/jni/android_net_NetUtils.cpp      |  2 +
 2 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/core/java/android/net/DnsResolver.java b/core/java/android/net/DnsResolver.java
index 59802514c7..f9e0af227a 100644
--- a/core/java/android/net/DnsResolver.java
+++ b/core/java/android/net/DnsResolver.java
@@ -205,6 +205,7 @@ public final class DnsResolver {
         if (cancellationSignal != null && cancellationSignal.isCanceled()) {
             return;
         }
+        final Object lock = new Object();
         final FileDescriptor queryfd;
         try {
             queryfd = resNetworkSend((network != null
@@ -214,8 +215,8 @@ public final class DnsResolver {
             return;
         }
 
-        maybeAddCancellationSignal(cancellationSignal, queryfd);
-        registerFDListener(executor, queryfd, callback);
+        maybeAddCancellationSignal(cancellationSignal, queryfd, lock);
+        registerFDListener(executor, queryfd, callback, cancellationSignal, lock);
     }
 
     /**
@@ -242,6 +243,7 @@ public final class DnsResolver {
         if (cancellationSignal != null && cancellationSignal.isCanceled()) {
             return;
         }
+        final Object lock = new Object();
         final FileDescriptor queryfd;
         try {
             queryfd = resNetworkQuery((network != null
@@ -251,31 +253,37 @@ public final class DnsResolver {
             return;
         }
 
-        maybeAddCancellationSignal(cancellationSignal, queryfd);
-        registerFDListener(executor, queryfd, callback);
+        maybeAddCancellationSignal(cancellationSignal, queryfd, lock);
+        registerFDListener(executor, queryfd, callback, cancellationSignal, lock);
     }
 
     private <T> void registerFDListener(@NonNull Executor executor,
-            @NonNull FileDescriptor queryfd, @NonNull AnswerCallback<T> answerCallback) {
+            @NonNull FileDescriptor queryfd, @NonNull AnswerCallback<T> answerCallback,
+            @Nullable CancellationSignal cancellationSignal, @NonNull Object lock) {
         Looper.getMainLooper().getQueue().addOnFileDescriptorEventListener(
                 queryfd,
                 FD_EVENTS,
                 (fd, events) -> {
                     executor.execute(() -> {
-                        byte[] answerbuf = null;
-                        try {
-                            answerbuf = resNetworkResult(fd);
-                        } catch (ErrnoException e) {
-                            Log.e(TAG, "resNetworkResult:" + e.toString());
-                            answerCallback.onQueryException(e);
-                            return;
-                        }
+                        synchronized (lock) {
+                            if (cancellationSignal != null && cancellationSignal.isCanceled()) {
+                                return;
+                            }
+                            byte[] answerbuf = null;
+                            try {
+                                answerbuf = resNetworkResult(fd);
+                            } catch (ErrnoException e) {
+                                Log.e(TAG, "resNetworkResult:" + e.toString());
+                                answerCallback.onQueryException(e);
+                                return;
+                            }
 
-                        try {
-                            answerCallback.onAnswer(
-                                    answerCallback.parser.parse(answerbuf));
-                        } catch (ParseException e) {
-                            answerCallback.onParseException(e);
+                            try {
+                                answerCallback.onAnswer(
+                                        answerCallback.parser.parse(answerbuf));
+                            } catch (ParseException e) {
+                                answerCallback.onParseException(e);
+                            }
                         }
                     });
                     // Unregister this fd listener
@@ -284,14 +292,16 @@ public final class DnsResolver {
     }
 
     private void maybeAddCancellationSignal(@Nullable CancellationSignal cancellationSignal,
-            @NonNull FileDescriptor queryfd) {
+            @NonNull FileDescriptor queryfd, @NonNull Object lock) {
         if (cancellationSignal == null) return;
-        cancellationSignal.setOnCancelListener(
-                () -> {
-                    Looper.getMainLooper().getQueue()
-                            .removeOnFileDescriptorEventListener(queryfd);
-                    resNetworkCancel(queryfd);
-            });
+        cancellationSignal.setOnCancelListener(() -> {
+            synchronized (lock)  {
+                if (!queryfd.valid()) return;
+                Looper.getMainLooper().getQueue()
+                        .removeOnFileDescriptorEventListener(queryfd);
+                resNetworkCancel(queryfd);
+            }
+        });
     }
 
     private static class DnsAddressAnswer extends DnsPacket {
diff --git a/core/jni/android_net_NetUtils.cpp b/core/jni/android_net_NetUtils.cpp
index d7a981ed3e..82acf6fb43 100644
--- a/core/jni/android_net_NetUtils.cpp
+++ b/core/jni/android_net_NetUtils.cpp
@@ -470,6 +470,7 @@ static jbyteArray android_net_utils_resNetworkResult(JNIEnv *env, jobject thiz,
     std::vector<uint8_t> buf(MAXPACKETSIZE, 0);
 
     int res = resNetworkResult(fd, &rcode, buf.data(), MAXPACKETSIZE);
+    jniSetFileDescriptorOfFD(env, javaFd, -1);
     if (res < 0) {
         throwErrnoException(env, "resNetworkResult", -res);
         return nullptr;
@@ -490,6 +491,7 @@ static jbyteArray android_net_utils_resNetworkResult(JNIEnv *env, jobject thiz,
 static void android_net_utils_resNetworkCancel(JNIEnv *env, jobject thiz, jobject javaFd) {
     int fd = jniGetFDFromFileDescriptor(env, javaFd);
     resNetworkCancel(fd);
+    jniSetFileDescriptorOfFD(env, javaFd, -1);
 }
 
 static jobject android_net_utils_getTcpRepairWindow(JNIEnv *env, jobject thiz, jobject javaFd) {