From 4af1b5084b6747a33791c35ce85daacd91555425 Mon Sep 17 00:00:00 2001
From: Kweku Adams <kwekua@google.com>
Date: Thu, 23 May 2019 16:23:48 -0700
Subject: [PATCH] Handling race condition when dumping heaps.

1. The IPC to ActivityThread.dumpHeap() dups the input file descriptor but
closes it when the IPC returns. Since the heap dump is generated
asynchronously, a race condition ensues between the returning close and
the dump being generated. For the intra-system-process call, the race is
with ActivityManagerService closing the created file descriptor. Duping
the file descriptor on the ActivityThread side should deal with this.
2. For some reason, the file descriptor wasn't closed for native heap
dumps. Closing the fd in those cases as well.
3. Catch the RuntimeException from Debug.dumpHprofData in case anything
else was missed.

Bug: 133424499
Test: adb shell am dumpheap com.android.systemui
Test: adb shell am dumpheap system
Test: Use the "Capture System Heap Dump" option in Developer Settings
Change-Id: I44817161533359766250de04e35902587ea9cc40
---
 core/java/android/app/ActivityThread.java | 50 ++++++++++++++---------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/core/java/android/app/ActivityThread.java b/core/java/android/app/ActivityThread.java
index 2db1bcc0faad9..1e982bc48c195 100644
--- a/core/java/android/app/ActivityThread.java
+++ b/core/java/android/app/ActivityThread.java
@@ -879,6 +879,7 @@ public final class ActivityThread extends ClientTransactionHandler {
     }
 
     static final class DumpHeapData {
+        // Whether to dump the native or managed heap.
         public boolean managed;
         public boolean mallocInfo;
         public boolean runGc;
@@ -1137,7 +1138,14 @@ public final class ActivityThread extends ClientTransactionHandler {
             dhd.mallocInfo = mallocInfo;
             dhd.runGc = runGc;
             dhd.path = path;
-            dhd.fd = fd;
+            try {
+                // Since we're going to dump the heap asynchronously, dup the file descriptor before
+                // it's closed on returning from the IPC call.
+                dhd.fd = fd.dup();
+            } catch (IOException e) {
+                Slog.e(TAG, "Failed to duplicate heap dump file descriptor", e);
+                return;
+            }
             dhd.finishCallback = finishCallback;
             sendMessage(H.DUMP_HEAP, dhd, 0, 0, true /*async*/);
         }
@@ -3106,9 +3114,10 @@ public final class ActivityThread extends ClientTransactionHandler {
     }
 
     private void sendMessage(int what, Object obj, int arg1, int arg2, boolean async) {
-        if (DEBUG_MESSAGES) Slog.v(
-            TAG, "SCHEDULE " + what + " " + mH.codeToString(what)
-            + ": " + arg1 + " / " + obj);
+        if (DEBUG_MESSAGES) {
+            Slog.v(TAG,
+                    "SCHEDULE " + what + " " + mH.codeToString(what) + ": " + arg1 + " / " + obj);
+        }
         Message msg = Message.obtain();
         msg.what = what;
         msg.obj = obj;
@@ -5827,23 +5836,24 @@ public final class ActivityThread extends ClientTransactionHandler {
             System.runFinalization();
             System.gc();
         }
-        if (dhd.managed) {
-            try {
-                Debug.dumpHprofData(dhd.path, dhd.fd.getFileDescriptor());
-            } catch (IOException e) {
-                Slog.w(TAG, "Managed heap dump failed on path " + dhd.path
-                        + " -- can the process access this path?");
-            } finally {
-                try {
-                    dhd.fd.close();
-                } catch (IOException e) {
-                    Slog.w(TAG, "Failure closing profile fd", e);
-                }
+        try (ParcelFileDescriptor fd = dhd.fd) {
+            if (dhd.managed) {
+                Debug.dumpHprofData(dhd.path, fd.getFileDescriptor());
+            } else if (dhd.mallocInfo) {
+                Debug.dumpNativeMallocInfo(fd.getFileDescriptor());
+            } else {
+                Debug.dumpNativeHeap(fd.getFileDescriptor());
             }
-        } else if (dhd.mallocInfo) {
-            Debug.dumpNativeMallocInfo(dhd.fd.getFileDescriptor());
-        } else {
-            Debug.dumpNativeHeap(dhd.fd.getFileDescriptor());
+        } catch (IOException e) {
+            if (dhd.managed) {
+                Slog.w(TAG, "Managed heap dump failed on path " + dhd.path
+                        + " -- can the process access this path?", e);
+            } else {
+                Slog.w(TAG, "Failed to dump heap", e);
+            }
+        } catch (RuntimeException e) {
+            // This should no longer happening now that we're copying the file descriptor.
+            Slog.wtf(TAG, "Heap dumper threw a runtime exception", e);
         }
         try {
             ActivityManager.getService().dumpHeapFinished(dhd.path);