commit 5386f49418a62ce8564139071707c3b01fbbe3ac
author Luke Huang <huangluke@google.com> Tue Mar 26 15:50:10 2019 +0800
committer Luke Huang <huangluke@google.com> Thu Mar 28 19:46:56 2019 +0800
tree 7a021ce73c8479f5953786ffd7304f43c6b07246
parent eea398a690496127b3626917bd800a5d57ae911c

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

diff --git a/core/java/android/net/DnsResolver.java b/core/java/android/net/DnsResolver.java
index 5980251..f9e0af2 100644
--- a/core/java/android/net/DnsResolver.java
+++ b/core/java/android/net/DnsResolver.java
@@ -205,6 +205,7 @@
         if (cancellationSignal != null && cancellationSignal.isCanceled()) {
             return;
         }
+        final Object lock = new Object();
         final FileDescriptor queryfd;
         try {
             queryfd = resNetworkSend((network != null
@@ -214,8 +215,8 @@
             return;
         }
 
-        maybeAddCancellationSignal(cancellationSignal, queryfd);
-        registerFDListener(executor, queryfd, callback);
+        maybeAddCancellationSignal(cancellationSignal, queryfd, lock);
+        registerFDListener(executor, queryfd, callback, cancellationSignal, lock);
     }
 
     /**
@@ -242,6 +243,7 @@
         if (cancellationSignal != null && cancellationSignal.isCanceled()) {
             return;
         }
+        final Object lock = new Object();
         final FileDescriptor queryfd;
         try {
             queryfd = resNetworkQuery((network != null
@@ -251,31 +253,37 @@
             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 @@
     }
 
     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 {