Revert sync app op chain changes
These changes cause some noted app ops to be swallowed due to one-way
app ops.
Fixes: 187721493
Test: atest AppOpsLoggingTest
Change-Id: I3b761b65b2e06138fc1d130bf80587f8885bb1d5
diff --git a/core/java/android/app/AppOpsManager.java b/core/java/android/app/AppOpsManager.java
index 4ddb546..bb7cdfa 100644
--- a/core/java/android/app/AppOpsManager.java
+++ b/core/java/android/app/AppOpsManager.java
@@ -2849,14 +2849,14 @@
private static final ThreadLocal<Integer> sBinderThreadCallingUid = new ThreadLocal<>();
/**
- * If a thread is currently executing a two-way binder transaction, this stores the
- * ops that were noted blaming any app (the caller, the caller of the caller, etc).
+ * If a thread is currently executing a two-way binder transaction, this stores the op-codes of
+ * the app-ops that were noted during this transaction.
*
* @see #getNotedOpCollectionMode
* @see #collectNotedOpSync
*/
- private static final ThreadLocal<ArrayMap<String, ArrayMap<String, long[]>>>
- sAppOpsNotedInThisBinderTransaction = new ThreadLocal<>();
+ private static final ThreadLocal<ArrayMap<String, long[]>> sAppOpsNotedInThisBinderTransaction =
+ new ThreadLocal<>();
/** Whether noting for an appop should be collected */
private static final @ShouldCollectNoteOp byte[] sAppOpsToNote = new byte[_NUM_OP];
@@ -9052,6 +9052,66 @@
}
/**
+ * State of a temporarily paused noted app-ops collection.
+ *
+ * @see #pauseNotedAppOpsCollection()
+ *
+ * @hide
+ */
+ public static class PausedNotedAppOpsCollection {
+ final int mUid;
+ final @Nullable ArrayMap<String, long[]> mCollectedNotedAppOps;
+
+ PausedNotedAppOpsCollection(int uid, @Nullable ArrayMap<String,
+ long[]> collectedNotedAppOps) {
+ mUid = uid;
+ mCollectedNotedAppOps = collectedNotedAppOps;
+ }
+ }
+
+ /**
+ * Temporarily suspend collection of noted app-ops when binder-thread calls into the other
+ * process. During such a call there might be call-backs coming back on the same thread which
+ * should not be accounted to the current collection.
+ *
+ * @return a state needed to resume the collection
+ *
+ * @hide
+ */
+ public static @Nullable PausedNotedAppOpsCollection pauseNotedAppOpsCollection() {
+ Integer previousUid = sBinderThreadCallingUid.get();
+ if (previousUid != null) {
+ ArrayMap<String, long[]> previousCollectedNotedAppOps =
+ sAppOpsNotedInThisBinderTransaction.get();
+
+ sBinderThreadCallingUid.remove();
+ sAppOpsNotedInThisBinderTransaction.remove();
+
+ return new PausedNotedAppOpsCollection(previousUid, previousCollectedNotedAppOps);
+ }
+
+ return null;
+ }
+
+ /**
+ * Resume a collection paused via {@link #pauseNotedAppOpsCollection}.
+ *
+ * @param prevCollection The state of the previous collection
+ *
+ * @hide
+ */
+ public static void resumeNotedAppOpsCollection(
+ @Nullable PausedNotedAppOpsCollection prevCollection) {
+ if (prevCollection != null) {
+ sBinderThreadCallingUid.set(prevCollection.mUid);
+
+ if (prevCollection.mCollectedNotedAppOps != null) {
+ sAppOpsNotedInThisBinderTransaction.set(prevCollection.mCollectedNotedAppOps);
+ }
+ }
+ }
+
+ /**
* Finish collection of noted appops on this thread.
*
* <p>Called at the end of a two way binder transaction.
@@ -9091,47 +9151,26 @@
*/
@TestApi
public static void collectNotedOpSync(@NonNull SyncNotedAppOp syncOp) {
- collectNotedOpSync(sOpStrToOp.get(syncOp.getOp()), syncOp.getAttributionTag(),
- syncOp.getPackageName());
- }
-
- /**
- * Collect a noted op when inside of a two-way binder call.
- *
- * <p> Delivered to caller via {@link #prefixParcelWithAppOpsIfNeeded}
- *
- * @param code the op code to note for
- * @param attributionTag the attribution tag to note for
- * @param packageName the package to note for
- */
- private static void collectNotedOpSync(int code, @Nullable String attributionTag,
- @NonNull String packageName) {
// If this is inside of a two-way binder call:
// We are inside of a two-way binder call. Delivered to caller via
// {@link #prefixParcelWithAppOpsIfNeeded}
- ArrayMap<String, ArrayMap<String, long[]>> appOpsNoted =
- sAppOpsNotedInThisBinderTransaction.get();
+ int op = sOpStrToOp.get(syncOp.getOp());
+ ArrayMap<String, long[]> appOpsNoted = sAppOpsNotedInThisBinderTransaction.get();
if (appOpsNoted == null) {
appOpsNoted = new ArrayMap<>(1);
sAppOpsNotedInThisBinderTransaction.set(appOpsNoted);
}
- ArrayMap<String, long[]> packageAppOpsNotedForAttribution = appOpsNoted.get(packageName);
- if (packageAppOpsNotedForAttribution == null) {
- packageAppOpsNotedForAttribution = new ArrayMap<>(1);
- appOpsNoted.put(packageName, packageAppOpsNotedForAttribution);
- }
-
- long[] appOpsNotedForAttribution = packageAppOpsNotedForAttribution.get(attributionTag);
+ long[] appOpsNotedForAttribution = appOpsNoted.get(syncOp.getAttributionTag());
if (appOpsNotedForAttribution == null) {
appOpsNotedForAttribution = new long[2];
- packageAppOpsNotedForAttribution.put(attributionTag, appOpsNotedForAttribution);
+ appOpsNoted.put(syncOp.getAttributionTag(), appOpsNotedForAttribution);
}
- if (code < 64) {
- appOpsNotedForAttribution[0] |= 1L << code;
+ if (op < 64) {
+ appOpsNotedForAttribution[0] |= 1L << op;
} else {
- appOpsNotedForAttribution[1] |= 1L << (code - 64);
+ appOpsNotedForAttribution[1] |= 1L << (op - 64);
}
}
@@ -9185,7 +9224,9 @@
}
}
- if (isListeningForOpNotedInBinderTransaction()) {
+ Integer binderUid = sBinderThreadCallingUid.get();
+
+ if (binderUid != null && binderUid == uid) {
return COLLECT_SYNC;
} else {
return COLLECT_ASYNC;
@@ -9204,32 +9245,20 @@
*/
// TODO (b/186872903) Refactor how sync noted ops are propagated.
public static void prefixParcelWithAppOpsIfNeeded(@NonNull Parcel p) {
- if (!isListeningForOpNotedInBinderTransaction()) {
- return;
- }
- final ArrayMap<String, ArrayMap<String, long[]>> notedAppOps =
- sAppOpsNotedInThisBinderTransaction.get();
+ ArrayMap<String, long[]> notedAppOps = sAppOpsNotedInThisBinderTransaction.get();
if (notedAppOps == null) {
return;
}
p.writeInt(Parcel.EX_HAS_NOTED_APPOPS_REPLY_HEADER);
- final int packageCount = notedAppOps.size();
- p.writeInt(packageCount);
+ int numAttributionWithNotesAppOps = notedAppOps.size();
+ p.writeInt(numAttributionWithNotesAppOps);
- for (int i = 0; i < packageCount; i++) {
+ for (int i = 0; i < numAttributionWithNotesAppOps; i++) {
p.writeString(notedAppOps.keyAt(i));
-
- final ArrayMap<String, long[]> notedTagAppOps = notedAppOps.valueAt(i);
- final int tagCount = notedTagAppOps.size();
- p.writeInt(tagCount);
-
- for (int j = 0; j < tagCount; j++) {
- p.writeString(notedTagAppOps.keyAt(j));
- p.writeLong(notedTagAppOps.valueAt(j)[0]);
- p.writeLong(notedTagAppOps.valueAt(j)[1]);
- }
+ p.writeLong(notedAppOps.valueAt(i)[0]);
+ p.writeLong(notedAppOps.valueAt(i)[1]);
}
}
@@ -9244,54 +9273,36 @@
* @hide
*/
public static void readAndLogNotedAppops(@NonNull Parcel p) {
- final int packageCount = p.readInt();
- if (packageCount <= 0) {
- return;
- }
+ int numAttributionsWithNotedAppOps = p.readInt();
- final String myPackageName = ActivityThread.currentPackageName();
+ for (int i = 0; i < numAttributionsWithNotedAppOps; i++) {
+ String attributionTag = p.readString();
+ long[] rawNotedAppOps = new long[2];
+ rawNotedAppOps[0] = p.readLong();
+ rawNotedAppOps[1] = p.readLong();
- synchronized (sLock) {
- for (int i = 0; i < packageCount; i++) {
- final String packageName = p.readString();
+ if (rawNotedAppOps[0] != 0 || rawNotedAppOps[1] != 0) {
+ BitSet notedAppOps = BitSet.valueOf(rawNotedAppOps);
- final int tagCount = p.readInt();
- for (int j = 0; j < tagCount; j++) {
- final String attributionTag = p.readString();
- final long[] rawNotedAppOps = new long[2];
- rawNotedAppOps[0] = p.readLong();
- rawNotedAppOps[1] = p.readLong();
-
- if (rawNotedAppOps[0] == 0 && rawNotedAppOps[1] == 0) {
- continue;
- }
-
- final BitSet notedAppOps = BitSet.valueOf(rawNotedAppOps);
+ synchronized (sLock) {
for (int code = notedAppOps.nextSetBit(0); code != -1;
code = notedAppOps.nextSetBit(code + 1)) {
- if (Objects.equals(myPackageName, packageName)) {
- if (sOnOpNotedCallback != null) {
- sOnOpNotedCallback.onNoted(new SyncNotedAppOp(code,
- attributionTag, packageName));
- } else {
- String message = getFormattedStackTrace();
- sUnforwardedOps.add(new AsyncNotedAppOp(code, Process.myUid(),
- attributionTag, message, System.currentTimeMillis()));
- if (sUnforwardedOps.size() > MAX_UNFORWARDED_OPS) {
- sUnforwardedOps.remove(0);
- }
+ if (sOnOpNotedCallback != null) {
+ sOnOpNotedCallback.onNoted(new SyncNotedAppOp(code, attributionTag));
+ } else {
+ String message = getFormattedStackTrace();
+ sUnforwardedOps.add(
+ new AsyncNotedAppOp(code, Process.myUid(), attributionTag,
+ message, System.currentTimeMillis()));
+ if (sUnforwardedOps.size() > MAX_UNFORWARDED_OPS) {
+ sUnforwardedOps.remove(0);
}
- } else if (isListeningForOpNotedInBinderTransaction()) {
- collectNotedOpSync(code, attributionTag, packageName);
}
}
- for (int code = notedAppOps.nextSetBit(0); code != -1;
- code = notedAppOps.nextSetBit(code + 1)) {
- if (Objects.equals(myPackageName, packageName)) {
- sMessageCollector.onNoted(new SyncNotedAppOp(code,
- attributionTag, packageName));
- }
- }
+ }
+ for (int code = notedAppOps.nextSetBit(0); code != -1;
+ code = notedAppOps.nextSetBit(code + 1)) {
+ sMessageCollector.onNoted(new SyncNotedAppOp(code, attributionTag));
}
}
}
@@ -9398,15 +9409,7 @@
* @hide
*/
public static boolean isListeningForOpNoted() {
- return sOnOpNotedCallback != null || isListeningForOpNotedInBinderTransaction()
- || isCollectingStackTraces();
- }
-
- /**
- * @return whether we are in a binder transaction and collecting appops.
- */
- private static boolean isListeningForOpNotedInBinderTransaction() {
- return sBinderThreadCallingUid.get() != null;
+ return sOnOpNotedCallback != null || isCollectingStackTraces();
}
/**
diff --git a/core/java/android/app/SyncNotedAppOp.java b/core/java/android/app/SyncNotedAppOp.java
index 32d889e..7c0c08a 100644
--- a/core/java/android/app/SyncNotedAppOp.java
+++ b/core/java/android/app/SyncNotedAppOp.java
@@ -21,7 +21,6 @@
import android.annotation.Nullable;
import android.annotation.TestApi;
import android.os.Parcelable;
-import android.os.Process;
import com.android.internal.annotations.Immutable;
import com.android.internal.util.DataClass;
@@ -29,6 +28,8 @@
/**
* Description of an app-op that was noted for the current process.
*
+ * Note: package name is currently unused in the system.
+ *
* <p>This is either delivered after a
* {@link AppOpsManager.OnOpNotedCallback#onNoted(SyncNotedAppOp) two way binder call} or
* when the app
diff --git a/core/java/android/os/BinderProxy.java b/core/java/android/os/BinderProxy.java
index d44b016..3d466a0 100644
--- a/core/java/android/os/BinderProxy.java
+++ b/core/java/android/os/BinderProxy.java
@@ -560,6 +560,9 @@
}
}
+ final AppOpsManager.PausedNotedAppOpsCollection prevCollection =
+ AppOpsManager.pauseNotedAppOpsCollection();
+
if ((flags & FLAG_ONEWAY) == 0 && AppOpsManager.isListeningForOpNoted()) {
flags |= FLAG_COLLECT_NOTED_APP_OPS;
}
@@ -567,6 +570,8 @@
try {
return transactNative(code, data, reply, flags);
} finally {
+ AppOpsManager.resumeNotedAppOpsCollection(prevCollection);
+
if (transactListener != null) {
transactListener.onTransactEnded(session);
}
diff --git a/services/core/java/com/android/server/appop/AppOpsService.java b/services/core/java/com/android/server/appop/AppOpsService.java
index a9905dc..6d7966f 100644
--- a/services/core/java/com/android/server/appop/AppOpsService.java
+++ b/services/core/java/com/android/server/appop/AppOpsService.java
@@ -3438,7 +3438,7 @@
+ " package " + packageName + "flags: " +
AppOpsManager.flagsToString(flags));
return new SyncNotedAppOp(AppOpsManager.MODE_ERRORED, code, attributionTag,
- packageName + " flags: " + AppOpsManager.flagsToString(flags));
+ packageName);
}
final Op op = getOpLocked(ops, code, uid, true);
final AttributedOp attributedOp = op.getOrCreateAttribution(op, attributionTag);