Bugreporting API: Take ownership of fds.

BUG: 126434607
FIXES: 127649051
Test: manual
Change-Id: I39e8421925c53061b6bc2954dffe3bccb7b3314d

diff --git a/core/java/android/os/BugreportManager.java b/core/java/android/os/BugreportManager.java
index 89b6577..bc979b4 100644 (file)
--- a/core/java/android/os/BugreportManager.java
+++ b/core/java/android/os/BugreportManager.java
@@ -27,6 +27,8 @@ import android.content.Context;
 
 import com.android.internal.util.Preconditions;
 
+import libcore.io.IoUtils;
+
 import java.io.FileDescriptor;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
@@ -123,6 +125,8 @@ public class BugreportManager {
      * <p>The bugreport artifacts will be copied over to the given file descriptors only if the
      * user consents to sharing with the calling app.
      *
+     * <p>{@link BugreportManager} takes ownership of {@code bugreportFd} and {@code screenshotFd}.
+     *
      * @param bugreportFd file to write the bugreport. This should be opened in write-only,
      *     append mode.
      * @param screenshotFd file to write the screenshot, if necessary. This should be opened
@@ -136,12 +140,13 @@ public class BugreportManager {
             @NonNull BugreportParams params,
             @NonNull @CallbackExecutor Executor executor,
             @NonNull BugreportCallback callback) {
-        Preconditions.checkNotNull(bugreportFd);
-        Preconditions.checkNotNull(params);
-        Preconditions.checkNotNull(executor);
-        Preconditions.checkNotNull(callback);
-        DumpstateListener dsListener = new DumpstateListener(executor, callback);
         try {
+            Preconditions.checkNotNull(bugreportFd);
+            Preconditions.checkNotNull(params);
+            Preconditions.checkNotNull(executor);
+            Preconditions.checkNotNull(callback);
+
+            DumpstateListener dsListener = new DumpstateListener(executor, callback);
             // Note: mBinder can get callingUid from the binder transaction.
             mBinder.startBugreport(-1 /* callingUid */,
                     mContext.getOpPackageName(),
@@ -151,6 +156,12 @@ public class BugreportManager {
                     params.getMode(), dsListener);
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
+        } finally {
+            // We can close the file descriptors here because binder would have duped them.
+            IoUtils.closeQuietly(bugreportFd);
+            if (screenshotFd != null) {
+                IoUtils.closeQuietly(screenshotFd);
+            }
         }
     }
 
@@ -170,7 +181,7 @@ public class BugreportManager {
         private final Executor mExecutor;
         private final BugreportCallback mCallback;
 
-        DumpstateListener(Executor executor, @Nullable BugreportCallback callback) {
+        DumpstateListener(Executor executor, BugreportCallback callback) {
             mExecutor = executor;
             mCallback = callback;
         }

diff --git a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
index f4454ae..653b65c 100644 (file)
--- a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
+++ b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
@@ -38,10 +38,6 @@ import com.android.internal.util.Preconditions;
 
 import java.io.FileDescriptor;
 
-// TODO(b/111441001):
-// Intercept onFinished() & implement death recipient here and shutdown
-// bugreportd service.
-
 /**
  * Implementation of the service that provides a privileged API to capture and consume bugreports.
  *
@@ -157,9 +153,12 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
             reportError(listener, IDumpstateListener.BUGREPORT_ERROR_RUNTIME_ERROR);
             return;
         }
+
+        // Wrap the listener so we can intercept binder events directly.
+        IDumpstateListener myListener = new DumpstateListener(listener, ds);
         try {
             ds.startBugreport(callingUid, callingPackage,
-                    bugreportFd, screenshotFd, bugreportMode, listener);
+                    bugreportFd, screenshotFd, bugreportMode, myListener);
         } catch (RemoteException e) {
             reportError(listener, IDumpstateListener.BUGREPORT_ERROR_RUNTIME_ERROR);
         }
@@ -226,4 +225,73 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
         Slog.w(TAG, message);
         throw new IllegalArgumentException(message);
     }
+
+
+    private final class DumpstateListener extends IDumpstateListener.Stub
+            implements DeathRecipient {
+        private final IDumpstateListener mListener;
+        private final IDumpstate mDs;
+        private boolean mDone = false;
+
+        DumpstateListener(IDumpstateListener listener, IDumpstate ds) {
+            mListener = listener;
+            mDs = ds;
+            try {
+                mDs.asBinder().linkToDeath(this, 0);
+            } catch (RemoteException e) {
+                Slog.e(TAG, "Unable to register Death Recipient for IDumpstate", e);
+            }
+        }
+
+        @Override
+        public void onProgress(int progress) throws RemoteException {
+            mListener.onProgress(progress);
+        }
+
+        @Override
+        public void onError(int errorCode) throws RemoteException {
+            synchronized (mLock) {
+                mDone = true;
+            }
+            mListener.onError(errorCode);
+        }
+
+        @Override
+        public void onFinished() throws RemoteException {
+            synchronized (mLock) {
+                mDone = true;
+            }
+            mListener.onFinished();
+        }
+
+        @Override
+        public void binderDied() {
+            synchronized (mLock) {
+                if (!mDone) {
+                    // If we have not gotten a "done" callback this must be a crash.
+                    Slog.e(TAG, "IDumpstate likely crashed. Notifying listener");
+                    try {
+                        mListener.onError(IDumpstateListener.BUGREPORT_ERROR_RUNTIME_ERROR);
+                    } catch (RemoteException ignored) {
+                        // If listener is not around, there isn't anything to do here.
+                    }
+                }
+            }
+            mDs.asBinder().unlinkToDeath(this, 0);
+        }
+
+        // Old methods; unused in the API flow.
+        @Override
+        public void onProgressUpdated(int progress) throws RemoteException {
+        }
+
+        @Override
+        public void onMaxProgressUpdated(int maxProgress) throws RemoteException {
+        }
+
+        @Override
+        public void onSectionComplete(String title, int status, int size, int durationMs)
+                throws RemoteException {
+        }
+    }
 }