From 47537a1c58833ceb0e48c2c4a33d109e362fae2f Mon Sep 17 00:00:00 2001 From: Jeffrey Huang Date: Mon, 6 Jan 2020 15:35:34 -0800 Subject: [PATCH] Update activeConfigsChangedBroadcast avoid using intentsender in #sendActiveConfigsChangedBroadcast and #removeActiveConfigsChangedBroadcast. Bug: 146074295 Test: Ran GTS Tests Change-Id: I9313299ea0bc89f092b1c62fbfc34e06a127eaa9 --- .../android/os/IStatsCompanionService.aidl | 6 -- .../aidl/android/os/IStatsManagerService.aidl | 7 +++ apex/statsd/aidl/android/os/IStatsd.aidl | 4 +- .../android/server/stats/StatsCompanion.java | 17 +++++- .../server/stats/StatsCompanionService.java | 17 ------ .../server/stats/StatsManagerService.java | 56 +++++++++++++++++-- cmds/statsd/src/StatsService.cpp | 48 ++++++++-------- cmds/statsd/src/StatsService.h | 6 +- cmds/statsd/src/config/ConfigManager.cpp | 26 ++++++++- cmds/statsd/src/config/ConfigManager.h | 9 ++- core/java/android/app/StatsManager.java | 10 ++-- 11 files changed, 133 insertions(+), 73 deletions(-) diff --git a/apex/statsd/aidl/android/os/IStatsCompanionService.aidl b/apex/statsd/aidl/android/os/IStatsCompanionService.aidl index ec3a226ab5420..32413e2f1703b 100644 --- a/apex/statsd/aidl/android/os/IStatsCompanionService.aidl +++ b/apex/statsd/aidl/android/os/IStatsCompanionService.aidl @@ -66,12 +66,6 @@ interface IStatsCompanionService { /** Pull the specified data. Results will be sent to statsd when complete. */ StatsLogEventWrapper[] pullData(int pullCode); - /** - * Send a broadcast to the specified PendingIntent's as IBinder notifying it that the list - * of active configs has changed. - */ - oneway void sendActiveConfigsChangedBroadcast(in IBinder intentSender, in long[] configIds); - /** * Requests StatsCompanionService to send a broadcast using the given intentSender * (which should cast to an IIntentSender), along with the other information specified. diff --git a/apex/statsd/aidl/android/os/IStatsManagerService.aidl b/apex/statsd/aidl/android/os/IStatsManagerService.aidl index 151cdbd8770cb..291140cb875c3 100644 --- a/apex/statsd/aidl/android/os/IStatsManagerService.aidl +++ b/apex/statsd/aidl/android/os/IStatsManagerService.aidl @@ -52,6 +52,13 @@ interface IStatsManagerService { */ long[] setActiveConfigsChangedOperation(in PendingIntent pendingIntent, in String packageName); + /** + * Removes the active configs changed operation for the specified package name. + * + * Requires Manifest.permission.DUMP and Manifest.permission.PACKAGE_USAGE_STATS. + */ + void removeActiveConfigsChangedOperation(in String packageName); + /** * Set the PendingIntent to be used when broadcasting subscriber * information to the given subscriberId within the given config. diff --git a/apex/statsd/aidl/android/os/IStatsd.aidl b/apex/statsd/aidl/android/os/IStatsd.aidl index 986d483301a42..323d56fa5484c 100644 --- a/apex/statsd/aidl/android/os/IStatsd.aidl +++ b/apex/statsd/aidl/android/os/IStatsd.aidl @@ -133,14 +133,14 @@ interface IStatsd { * * Requires Manifest.permission.DUMP and Manifest.permission.PACKAGE_USAGE_STATS. */ - long[] setActiveConfigsChangedOperation(in IBinder intentSender, in String packageName); + long[] setActiveConfigsChangedOperation(in IPendingIntentRef pendingIntentRef, int callingUid); /** * Removes the active configs changed operation for the specified package name. * * Requires Manifest.permission.DUMP and Manifest.permission.PACKAGE_USAGE_STATS. */ - void removeActiveConfigsChangedOperation(in String packageName); + void removeActiveConfigsChangedOperation(int callingUid); /** * Removes the configuration with the matching config key. No-op if this config key does not diff --git a/apex/statsd/service/java/com/android/server/stats/StatsCompanion.java b/apex/statsd/service/java/com/android/server/stats/StatsCompanion.java index 6a0bf629cf5c9..77ccfb6d7a299 100644 --- a/apex/statsd/service/java/com/android/server/stats/StatsCompanion.java +++ b/apex/statsd/service/java/com/android/server/stats/StatsCompanion.java @@ -17,6 +17,7 @@ package com.android.server.stats; import android.app.PendingIntent; +import android.app.StatsManager; import android.content.Context; import android.content.Intent; import android.os.Binder; @@ -27,6 +28,8 @@ import android.util.Slog; import com.android.server.SystemService; +import java.util.Arrays; + /** * @hide */ @@ -97,6 +100,8 @@ public class StatsCompanion { */ private static final String EXTRA_LAST_REPORT_TIME = "android.app.extra.LAST_REPORT_TIME"; private static final int CODE_DATA_BROADCAST = 1; + private static final int CODE_ACTIVE_CONFIGS_BROADCAST = 1; + private final PendingIntent mPendingIntent; private final Context mContext; @@ -120,7 +125,17 @@ public class StatsCompanion { @Override public void sendActiveConfigsChangedBroadcast(long[] configIds) { - // no-op + enforceStatsCompanionPermission(mContext); + Intent intent = new Intent(); + intent.putExtra(StatsManager.EXTRA_STATS_ACTIVE_CONFIG_KEYS, configIds); + try { + mPendingIntent.send(mContext, CODE_ACTIVE_CONFIGS_BROADCAST, intent, null, null); + if (DEBUG) { + Slog.d(TAG, "Sent broadcast with config ids " + Arrays.toString(configIds)); + } + } catch (PendingIntent.CanceledException e) { + Slog.w(TAG, "Unable to send active configs changed broadcast using PendingIntent"); + } } @Override diff --git a/apex/statsd/service/java/com/android/server/stats/StatsCompanionService.java b/apex/statsd/service/java/com/android/server/stats/StatsCompanionService.java index d160f22d583f8..befce218dc563 100644 --- a/apex/statsd/service/java/com/android/server/stats/StatsCompanionService.java +++ b/apex/statsd/service/java/com/android/server/stats/StatsCompanionService.java @@ -203,7 +203,6 @@ public class StatsCompanionService extends IStatsCompanionService.Stub { private static final int INSTALLER_FIELD_ID = 5; public static final int CODE_SUBSCRIBER_BROADCAST = 1; - public static final int CODE_ACTIVE_CONFIGS_BROADCAST = 1; public static final int DEATH_THRESHOLD = 10; /** * Which native processes to snapshot memory for. @@ -443,22 +442,6 @@ public class StatsCompanionService extends IStatsCompanionService.Stub { KernelCpuThreadReaderSettingsObserver.getSettingsModifiedReader(mContext); } - @Override - public void sendActiveConfigsChangedBroadcast(IBinder intentSenderBinder, long[] configIds) { - StatsCompanion.enforceStatsCompanionPermission(mContext); - IntentSender intentSender = new IntentSender(intentSenderBinder); - Intent intent = new Intent(); - intent.putExtra(StatsManager.EXTRA_STATS_ACTIVE_CONFIG_KEYS, configIds); - try { - intentSender.sendIntent(mContext, CODE_ACTIVE_CONFIGS_BROADCAST, intent, null, null); - if (DEBUG) { - Slog.d(TAG, "Sent broadcast with config ids " + Arrays.toString(configIds)); - } - } catch (IntentSender.SendIntentException e) { - Slog.w(TAG, "Unable to send active configs changed broadcast using IntentSender"); - } - } - @Override public void sendSubscriberBroadcast(IBinder intentSenderBinder, long configUid, long configKey, long subscriptionId, long subscriptionRuleId, String[] cookies, diff --git a/apex/statsd/service/java/com/android/server/stats/StatsManagerService.java b/apex/statsd/service/java/com/android/server/stats/StatsManagerService.java index 46fe597a7d53a..ef2b761a940e1 100644 --- a/apex/statsd/service/java/com/android/server/stats/StatsManagerService.java +++ b/apex/statsd/service/java/com/android/server/stats/StatsManagerService.java @@ -37,6 +37,7 @@ import java.util.Objects; /** * Service for {@link android.app.StatsManager}. + * * @hide */ public class StatsManagerService extends IStatsManagerService.Stub { @@ -57,6 +58,9 @@ public class StatsManagerService extends IStatsManagerService.Stub { @GuardedBy("mLock") private ArrayMap mDataFetchPirMap = new ArrayMap<>(); + @GuardedBy("mLock") + private ArrayMap mActiveConfigsPirMap = + new ArrayMap<>(); public StatsManagerService(Context context) { super(); @@ -143,11 +147,45 @@ public class StatsManagerService extends IStatsManagerService.Stub { @Override public long[] setActiveConfigsChangedOperation(PendingIntent pendingIntent, String packageName) { - // no-op - if (DEBUG) { - Slog.d(TAG, "setActiveConfigsChangedOperation"); + enforceDumpAndUsageStatsPermission(packageName); + int callingUid = Binder.getCallingUid(); + final long token = Binder.clearCallingIdentity(); + PendingIntentRef pir = new PendingIntentRef(pendingIntent, mContext); + // We add the PIR to a map so we can reregister if statsd is unavailable. + synchronized (mLock) { + mActiveConfigsPirMap.put(callingUid, pir); + } + try { + IStatsd statsd = getStatsdNonblocking(); + if (statsd != null) { + return statsd.setActiveConfigsChangedOperation(pir, callingUid); + } + } catch (RemoteException e) { + Slog.e(TAG, "Failed to setActiveConfigsChangedOperation with statsd"); + } finally { + Binder.restoreCallingIdentity(token); + } + return new long[] {}; + } + + @Override + public void removeActiveConfigsChangedOperation(String packageName) { + enforceDumpAndUsageStatsPermission(packageName); + int callingUid = Binder.getCallingUid(); + final long token = Binder.clearCallingIdentity(); + synchronized (mLock) { + mActiveConfigsPirMap.remove(callingUid); + } + try { + IStatsd statsd = getStatsdNonblocking(); + if (statsd != null) { + statsd.removeActiveConfigsChangedOperation(callingUid); + } + } catch (RemoteException e) { + Slog.e(TAG, "Failed to removeActiveConfigsChangedOperation with statsd"); + } finally { + Binder.restoreCallingIdentity(token); } - return new long[]{}; } @Override @@ -248,8 +286,10 @@ public class StatsManagerService extends IStatsManagerService.Stub { return; } ArrayMap dataFetchCopy; + ArrayMap activeConfigsChangedCopy; synchronized (mLock) { dataFetchCopy = new ArrayMap<>(mDataFetchPirMap); + activeConfigsChangedCopy = new ArrayMap<>(mActiveConfigsPirMap); } for (Map.Entry entry : dataFetchCopy.entrySet()) { ConfigKey key = entry.getKey(); @@ -259,5 +299,13 @@ public class StatsManagerService extends IStatsManagerService.Stub { Slog.e(TAG, "Failed to setDataFetchOperation from pirMap"); } } + for (Map.Entry entry + : activeConfigsChangedCopy.entrySet()) { + try { + statsd.setActiveConfigsChangedOperation(entry.getValue(), entry.getKey()); + } catch (RemoteException e) { + Slog.e(TAG, "Failed to setActiveConfigsChangedOperation from pirMap"); + } + } } } diff --git a/cmds/statsd/src/StatsService.cpp b/cmds/statsd/src/StatsService.cpp index 7fecf46c4ca5d..3a29b7271bbf6 100644 --- a/cmds/statsd/src/StatsService.cpp +++ b/cmds/statsd/src/StatsService.cpp @@ -185,18 +185,17 @@ StatsService::StatsService(const sp& handlerLooper, shared_ptr& activeConfigs) { - auto receiver = mConfigManager->GetActiveConfigsChangedReceiver(uid); - sp sc = getStatsCompanionService(); - if (sc == nullptr) { - VLOG("Could not access statsCompanion"); - return false; - } else if (receiver == nullptr) { + sp receiver = + mConfigManager->GetActiveConfigsChangedReceiver(uid); + if (receiver == nullptr) { VLOG("Could not find receiver for uid %d", uid); return false; - } else { - sc->sendActiveConfigsChangedBroadcast(receiver, activeConfigs); + } else if (receiver->sendActiveConfigsChangedBroadcast(activeConfigs).isOk()) { VLOG("StatsService::active configs broadcast succeeded for uid %d" , uid); return true; + } else { + VLOG("StatsService::active configs broadcast failed for uid %d" , uid); + return false; } }); @@ -630,15 +629,15 @@ status_t StatsService::cmd_trigger_active_config_broadcast(int out, VectorGetActiveConfigsChangedReceiver(uid); - sp sc = getStatsCompanionService(); - if (sc == nullptr) { - VLOG("Could not access statsCompanion"); - } else if (receiver == nullptr) { + sp receiver = mConfigManager->GetActiveConfigsChangedReceiver(uid); + if (receiver == nullptr) { VLOG("Could not find receiver for uid %d", uid); - } else { - sc->sendActiveConfigsChangedBroadcast(receiver, configIds); + return UNKNOWN_ERROR; + } else if (receiver->sendActiveConfigsChangedBroadcast(configIds).isOk()) { VLOG("StatsService::trigger active configs changed broadcast succeeded for uid %d" , uid); + } else { + VLOG("StatsService::trigger active configs changed broadcast failed for uid %d", uid); + return UNKNOWN_ERROR; } return NO_ERROR; } @@ -1209,27 +1208,24 @@ Status StatsService::setDataFetchOperation(int64_t key, return Status::ok(); } -Status StatsService::setActiveConfigsChangedOperation(const sp& intentSender, - const String16& packageName, +Status StatsService::setActiveConfigsChangedOperation(const sp& pir, + const int32_t callingUid, vector* output) { - ENFORCE_DUMP_AND_USAGE_STATS(packageName); + ENFORCE_UID(AID_SYSTEM); - IPCThreadState* ipc = IPCThreadState::self(); - int uid = ipc->getCallingUid(); - mConfigManager->SetActiveConfigsChangedReceiver(uid, intentSender); + mConfigManager->SetActiveConfigsChangedReceiver(callingUid, pir); if (output != nullptr) { - mProcessor->GetActiveConfigs(uid, *output); + mProcessor->GetActiveConfigs(callingUid, *output); } else { ALOGW("StatsService::setActiveConfigsChanged output was nullptr"); } return Status::ok(); } -Status StatsService::removeActiveConfigsChangedOperation(const String16& packageName) { - ENFORCE_DUMP_AND_USAGE_STATS(packageName); +Status StatsService::removeActiveConfigsChangedOperation(const int32_t callingUid) { + ENFORCE_UID(AID_SYSTEM); - IPCThreadState* ipc = IPCThreadState::self(); - mConfigManager->RemoveActiveConfigsChangedReceiver(ipc->getCallingUid()); + mConfigManager->RemoveActiveConfigsChangedReceiver(callingUid); return Status::ok(); } diff --git a/cmds/statsd/src/StatsService.h b/cmds/statsd/src/StatsService.h index 9912d2fb2d387..0e89a2b129e02 100644 --- a/cmds/statsd/src/StatsService.h +++ b/cmds/statsd/src/StatsService.h @@ -134,14 +134,14 @@ public: /** * Binder call to let clients register the active configs changed operation. */ - virtual Status setActiveConfigsChangedOperation(const sp& intentSender, - const String16& packageName, + virtual Status setActiveConfigsChangedOperation(const sp& pir, + const int32_t callingUid, vector* output) override; /** * Binder call to remove the active configs changed operation for the specified package.. */ - virtual Status removeActiveConfigsChangedOperation(const String16& packageName) override; + virtual Status removeActiveConfigsChangedOperation(const int32_t callingUid) override; /** * Binder call to allow clients to remove the specified configuration. */ diff --git a/cmds/statsd/src/config/ConfigManager.cpp b/cmds/statsd/src/config/ConfigManager.cpp index 7bfb991ab1acf..55d73c1ae3420 100644 --- a/cmds/statsd/src/config/ConfigManager.cpp +++ b/cmds/statsd/src/config/ConfigManager.cpp @@ -63,6 +63,24 @@ class ConfigReceiverDeathRecipient : public android::IBinder::DeathRecipient { } }; +class ActiveConfigChangedReceiverDeathRecipient : public android::IBinder::DeathRecipient { + public: + ActiveConfigChangedReceiverDeathRecipient(sp configManager, const int uid): + mConfigManager(configManager), + mUid(uid) {} + ~ActiveConfigChangedReceiverDeathRecipient() override = default; + private: + sp mConfigManager; + int mUid; + + void binderDied(const android::wp& who) override { + if (IInterface::asBinder(mConfigManager->GetActiveConfigsChangedReceiver(mUid)) + == who.promote()) { + mConfigManager->RemoveActiveConfigsChangedReceiver(mUid); + } + } +}; + ConfigManager::ConfigManager() { } @@ -148,9 +166,11 @@ void ConfigManager::RemoveConfigReceiver(const ConfigKey& key) { } void ConfigManager::SetActiveConfigsChangedReceiver(const int uid, - const sp& intentSender) { + const sp& pir) { lock_guard lock(mMutex); - mActiveConfigsChangedReceivers[uid] = intentSender; + mActiveConfigsChangedReceivers[uid] = pir; + IInterface::asBinder(pir)->linkToDeath( + new ActiveConfigChangedReceiverDeathRecipient(this, uid)); } void ConfigManager::RemoveActiveConfigsChangedReceiver(const int uid) { @@ -296,7 +316,7 @@ const sp ConfigManager::GetConfigReceiver(const ConfigKey& ke } } -const sp ConfigManager::GetActiveConfigsChangedReceiver(const int uid) const { +const sp ConfigManager::GetActiveConfigsChangedReceiver(const int uid) const { lock_guard lock(mMutex); auto it = mActiveConfigsChangedReceivers.find(uid); diff --git a/cmds/statsd/src/config/ConfigManager.h b/cmds/statsd/src/config/ConfigManager.h index 1aeb355b1f0a9..88e864a2520b0 100644 --- a/cmds/statsd/src/config/ConfigManager.h +++ b/cmds/statsd/src/config/ConfigManager.h @@ -16,7 +16,6 @@ #pragma once -#include "binder/IBinder.h" #include "config/ConfigKey.h" #include "config/ConfigListener.h" @@ -86,13 +85,13 @@ public: * Sets the broadcast receiver that is notified whenever the list of active configs * changes for this uid. */ - void SetActiveConfigsChangedReceiver(const int uid, const sp& intentSender); + void SetActiveConfigsChangedReceiver(const int uid, const sp& pir); /** * Returns the broadcast receiver for active configs changed for this uid. */ - const sp GetActiveConfigsChangedReceiver(const int uid) const; + const sp GetActiveConfigsChangedReceiver(const int uid) const; /** * Erase any active configs changed broadcast receiver associated with this uid. @@ -148,9 +147,9 @@ private: /** * Each uid can be subscribed by up to one receiver to notify that the list of active configs - * for this uid has changed. The receiver is specified as IBinder from PendingIntent. + * for this uid has changed. The receiver is specified as IPendingIntentRef. */ - std::map> mActiveConfigsChangedReceivers; + std::map> mActiveConfigsChangedReceivers; /** * The ConfigListeners that will be told about changes. diff --git a/core/java/android/app/StatsManager.java b/core/java/android/app/StatsManager.java index a458a55962d88..51b2d40894245 100644 --- a/core/java/android/app/StatsManager.java +++ b/core/java/android/app/StatsManager.java @@ -345,20 +345,18 @@ public final class StatsManager { throws StatsUnavailableException { synchronized (sLock) { try { - IStatsd service = getIStatsdLocked(); + IStatsManagerService service = getIStatsManagerServiceLocked(); if (pendingIntent == null) { service.removeActiveConfigsChangedOperation(mContext.getOpPackageName()); return new long[0]; } else { - // Extracts IIntentSender from the PendingIntent and turns it into an IBinder. - IBinder intentSender = pendingIntent.getTarget().asBinder(); - return service.setActiveConfigsChangedOperation(intentSender, + return service.setActiveConfigsChangedOperation(pendingIntent, mContext.getOpPackageName()); } } catch (RemoteException e) { - Slog.e(TAG, - "Failed to connect to statsd when registering active configs listener."); + Slog.e(TAG, "Failed to connect to statsmanager " + + "when registering active configs listener."); throw new StatsUnavailableException("could not connect", e); } catch (SecurityException e) { throw new StatsUnavailableException(e.getMessage(), e);