From be80e309eaa163d0228bc8f02c30d9826a2af268 Mon Sep 17 00:00:00 2001 From: Nandana Dutt Date: Wed, 13 Mar 2019 15:17:01 +0000 Subject: [PATCH 1/2] Don't manage dumpstate lifecycle from system server Dumpstate now exits correctly when it's finished. So we don't have to manage its lifecycle from system server. BUG: 123571915 Test: manually verified dumpstate service dies Change-Id: I8d0e12cb607cda74b5cc36e26306ea106da67ba3 --- core/java/android/os/BugreportManager.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/java/android/os/BugreportManager.java b/core/java/android/os/BugreportManager.java index 684369a6f720e..89b6577c79ba7 100644 --- a/core/java/android/os/BugreportManager.java +++ b/core/java/android/os/BugreportManager.java @@ -208,8 +208,6 @@ public class BugreportManager { }); } finally { Binder.restoreCallingIdentity(identity); - // The bugreport has finished. Let's shutdown the service to minimize its footprint. - cancelBugreport(); } } From 9d6ff2968eeb985d290e835e0860080e438a659f Mon Sep 17 00:00:00 2001 From: Nandana Dutt Date: Wed, 13 Mar 2019 16:42:17 +0000 Subject: [PATCH 2/2] Bugreporting API: Take ownership of fds. BUG: 126434607 FIXES: 127649051 Test: manual Change-Id: I39e8421925c53061b6bc2954dffe3bccb7b3314d --- core/java/android/os/BugreportManager.java | 23 ++++-- .../os/BugreportManagerServiceImpl.java | 78 +++++++++++++++++-- 2 files changed, 90 insertions(+), 11 deletions(-) diff --git a/core/java/android/os/BugreportManager.java b/core/java/android/os/BugreportManager.java index 89b6577c79ba7..bc979b4b9b316 100644 --- 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 { *

The bugreport artifacts will be copied over to the given file descriptors only if the * user consents to sharing with the calling app. * + *

{@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 f4454ae2a1807..653b65c22d7bb 100644 --- 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 { + } + } }