Merge "Pass caller information in cancelBugreport."

core/java/android/os/BugreportManager.java

@@ -26,7 +26,6 @@ import android.annotation.SystemApi;
 import android.annotation.SystemService;
 import android.app.ActivityManager;
 import android.content.Context;
-import android.os.Handler;
 import android.util.Log;
 import android.widget.Toast;
 
@@ -189,13 +188,18 @@ public final class BugreportManager {
         }
     }
 
-    /*
+    /**
-     * Cancels a currently running bugreport.
+     * Cancels the currently running bugreport.
+     *
+     * <p>Apps are only able to cancel their own bugreports. App A cannot cancel a bugreport started
+     * by app B.
+     *
+     * @throws SecurityException if trying to cancel another app's bugreport in progress
      */
     @RequiresPermission(android.Manifest.permission.DUMP)
     public void cancelBugreport() {
         try {
-            mBinder.cancelBugreport();
+            mBinder.cancelBugreport(-1 /* callingUid */, mContext.getOpPackageName());
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         }

core/tests/bugreports/src/com/android/os/bugreports/tests/BugreportManagerTest.java

@@ -23,7 +23,10 @@ import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import android.Manifest;
+import android.content.BroadcastReceiver;
 import android.content.Context;
+import android.content.Intent;
+import android.content.IntentFilter;
 import android.os.BugreportManager;
 import android.os.BugreportManager.BugreportCallback;
 import android.os.BugreportParams;
@@ -31,7 +34,9 @@ import android.os.FileUtils;
 import android.os.Handler;
 import android.os.HandlerThread;
 import android.os.ParcelFileDescriptor;
+import android.os.Process;
 import android.os.StrictMode;
+import android.text.TextUtils;
 import android.util.Log;
 
 import androidx.annotation.NonNull;
@@ -53,10 +58,11 @@ import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 import java.io.File;
+import java.io.IOException;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 
-
 /**
  * Tests for BugreportManager API.
  */
@@ -67,8 +73,16 @@ public class BugreportManagerTest {
 
     private static final String TAG = "BugreportManagerTest";
     private static final long BUGREPORT_TIMEOUT_MS = TimeUnit.MINUTES.toMillis(10);
+    private static final long DUMPSTATE_STARTUP_TIMEOUT_MS = TimeUnit.SECONDS.toMillis(10);
     private static final long UIAUTOMATOR_TIMEOUT_MS = TimeUnit.SECONDS.toMillis(10);
 
+    // Sent by Shell when its bugreport finishes (contains final bugreport/screenshot file name
+    // associated with the bugreport).
+    private static final String INTENT_BUGREPORT_FINISHED =
+            "com.android.internal.intent.action.BUGREPORT_FINISHED";
+    private static final String EXTRA_BUGREPORT = "android.intent.extra.BUGREPORT";
+    private static final String EXTRA_SCREENSHOT = "android.intent.extra.SCREENSHOT";
+
     private Handler mHandler;
     private Executor mExecutor;
     private BugreportManager mBrm;
@@ -211,6 +225,48 @@ public class BugreportManagerTest {
         assertFdsAreClosed(mBugreportFd, mScreenshotFd);
     }
 
+    @Test
+    public void cancelBugreport_noReportStarted() throws Exception {
+        // Without the native DumpstateService running, we don't get a SecurityException.
+        mBrm.cancelBugreport();
+    }
+
+    @LargeTest
+    @Test
+    public void cancelBugreport_fromDifferentUid() throws Exception {
+        assertThat(Process.myUid()).isNotEqualTo(Process.SHELL_UID);
+
+        // Start a bugreport through ActivityManager's shell command - this starts a BR from the
+        // shell UID rather than our own.
+        BugreportBroadcastReceiver br = new BugreportBroadcastReceiver();
+        InstrumentationRegistry.getContext()
+                .registerReceiver(br, new IntentFilter(INTENT_BUGREPORT_FINISHED));
+        UiDevice.getInstance(InstrumentationRegistry.getInstrumentation())
+                .executeShellCommand("am bug-report");
+
+        // The command triggers the report through a broadcast, so wait until dumpstate actually
+        // starts up, which may take a bit.
+        waitTillDumpstateRunningOrTimeout();
+
+        try {
+            mBrm.cancelBugreport();
+            fail("Expected cancelBugreport to throw SecurityException when report started by "
+                    + "different UID");
+        } catch (SecurityException expected) {
+        } finally {
+            // Do this in the finally block so that even if this test case fails, we don't break
+            // other test cases unexpectedly due to the still-running shell report.
+            try {
+                // The shell's BR is still running and should complete successfully.
+                br.waitForBugreportFinished();
+            } finally {
+                // The latch may fail for a number of reasons but we still need to unregister the
+                // BroadcastReceiver.
+                InstrumentationRegistry.getContext().unregisterReceiver(br);
+            }
+        }
+    }
+
     @Test
     public void insufficientPermissions_throwsException() throws Exception {
         dropPermissions();
@@ -346,6 +402,28 @@ public class BugreportManagerTest {
                 .adoptShellPermissionIdentity(Manifest.permission.DUMP);
     }
 
+    private static boolean isDumpstateRunning() {
+        String[] output;
+        try {
+            output =
+                    UiDevice.getInstance(InstrumentationRegistry.getInstrumentation())
+                            .executeShellCommand("ps -A -o NAME | grep dumpstate")
+                            .trim()
+                            .split("\n");
+        } catch (IOException e) {
+            Log.w(TAG, "Failed to check if dumpstate is running", e);
+            return false;
+        }
+        for (String line : output) {
+            // Check for an exact match since there may be other things that contain "dumpstate" as
+            // a substring (e.g. the dumpstate HAL).
+            if (TextUtils.equals("dumpstate", line)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     private static void assertFdIsClosed(ParcelFileDescriptor pfd) {
         try {
             int fd = pfd.getFd();
@@ -364,18 +442,25 @@ public class BugreportManagerTest {
         return System.currentTimeMillis();
     }
 
-    private static boolean shouldTimeout(long startTimeMs) {
+    private static void waitTillDumpstateRunningOrTimeout() throws Exception {
-        return now() - startTimeMs >= BUGREPORT_TIMEOUT_MS;
+        long startTimeMs = now();
+        while (!isDumpstateRunning()) {
+            Thread.sleep(500 /* .5s */);
+            if (now() - startTimeMs >= DUMPSTATE_STARTUP_TIMEOUT_MS) {
+                break;
+            }
+            Log.d(TAG, "Waited " + (now() - startTimeMs) + "ms for dumpstate to start");
+        }
     }
 
     private static void waitTillDoneOrTimeout(BugreportCallbackImpl callback) throws Exception {
         long startTimeMs = now();
         while (!callback.isDone()) {
             Thread.sleep(1000 /* 1s */);
-            if (shouldTimeout(startTimeMs)) {
+            if (now() - startTimeMs >= BUGREPORT_TIMEOUT_MS) {
                 break;
             }
-            Log.d(TAG, "Waited " + (now() - startTimeMs) + "ms");
+            Log.d(TAG, "Waited " + (now() - startTimeMs) + "ms for bugreport to finish");
         }
     }
 
@@ -450,6 +535,36 @@ public class BugreportManagerTest {
         assertTrue(device.wait(Until.gone(consentTitleObj), UIAUTOMATOR_TIMEOUT_MS));
     }
 
+    private class BugreportBroadcastReceiver extends BroadcastReceiver {
+        Intent mBugreportFinishedIntent = null;
+        final CountDownLatch mLatch;
+
+        BugreportBroadcastReceiver() {
+            mLatch = new CountDownLatch(1);
+        }
+
+        @Override
+        public void onReceive(Context context, Intent intent) {
+            setBugreportFinishedIntent(intent);
+            mLatch.countDown();
+        }
+
+        private void setBugreportFinishedIntent(Intent intent) {
+            mBugreportFinishedIntent = intent;
+        }
+
+        public Intent getBugreportFinishedIntent() {
+            return mBugreportFinishedIntent;
+        }
+
+        public void waitForBugreportFinished() throws Exception {
+            if (!mLatch.await(BUGREPORT_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
+                throw new Exception("Failed to receive BUGREPORT_FINISHED in "
+                        + BUGREPORT_TIMEOUT_MS + " ms.");
+            }
+        }
+    }
+
     /**
      * A rule to change strict mode vm policy temporarily till test method finished.
      *

services/core/java/com/android/server/os/BugreportManagerServiceImpl.java

@@ -94,9 +94,12 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
 
     @Override
     @RequiresPermission(android.Manifest.permission.DUMP)
-    public void cancelBugreport() {
+    public void cancelBugreport(int callingUidUnused, String callingPackage) {
         mContext.enforceCallingOrSelfPermission(android.Manifest.permission.DUMP,
                 "cancelBugreport");
+        int callingUid = Binder.getCallingUid();
+        mAppOps.checkPackage(callingUid, callingPackage);
+
         synchronized (mLock) {
             IDumpstate ds = getDumpstateBinderServiceLocked();
             if (ds == null) {
@@ -104,7 +107,11 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
                 return;
             }
             try {
-                ds.cancelBugreport();
+                // Note: this may throw SecurityException back out to the caller if they aren't
+                // allowed to cancel the report, in which case we should NOT be setting ctl.stop,
+                // since that would unintentionally kill some other app's bugreport, which we
+                // specifically disallow.
+                ds.cancelBugreport(callingUid, callingPackage);
             } catch (RemoteException e) {
                 Slog.e(TAG, "RemoteException in cancelBugreport", e);
             }
@@ -182,7 +189,7 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
             // lifecycle correctly. If we don't subsequent callers will get
             // BUGREPORT_ERROR_ANOTHER_REPORT_IN_PROGRESS error.
             // Note that listener will be notified by the death recipient below.
-            cancelBugreport();
+            cancelBugreport(callingUid, callingPackage);
         }
     }