From baed5257ad4d64bfd441d9cbacf13d4216b97cad Mon Sep 17 00:00:00 2001
From: Tej Singh <singhtejinder@google.com>
Date: Fri, 1 May 2020 17:30:00 -0700
Subject: [PATCH] Fix statsd NPE on setPullAtomCallback

Suspected root cause: if a process crashes right after calling
setPullAtomCallback, it's possible that oneway binder calls can queue
but do not execute before the process crashes. Then, when the process
crashes, nothing has a strong reference to the IPullAtomCallback, so
it gets deallocated. Then, when the oneway call actually executes, the
callback is null. This is being followed up in b/155793159

Regardless, statsd should handle null input properly.

Test: GTS test in ag/11348719 now passes
Test: atest GtsStatsdHostTestCases
Bug: 153822941
Change-Id: Ic6d415e10eca8d133290de80cb61e1634590ca6a
---
 apex/statsd/framework/java/android/app/StatsManager.java     | 4 ----
 .../java/com/android/server/stats/StatsManagerService.java   | 4 ++++
 cmds/statsd/src/external/StatsPullerManager.cpp              | 5 +++++
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/apex/statsd/framework/java/android/app/StatsManager.java b/apex/statsd/framework/java/android/app/StatsManager.java
index 7fbfc4318949f..d1b7d8dc2c7ae 100644
--- a/apex/statsd/framework/java/android/app/StatsManager.java
+++ b/apex/statsd/framework/java/android/app/StatsManager.java
@@ -28,7 +28,6 @@ import android.os.Binder;
 import android.os.IPullAtomCallback;
 import android.os.IPullAtomResultReceiver;
 import android.os.IStatsManagerService;
-import android.os.IStatsd;
 import android.os.RemoteException;
 import android.os.StatsFrameworkInitializer;
 import android.util.AndroidException;
@@ -56,9 +55,6 @@ public final class StatsManager {
     private static final Object sLock = new Object();
     private final Context mContext;
 
-    @GuardedBy("sLock")
-    private IStatsd mService;
-
     @GuardedBy("sLock")
     private IStatsManagerService mStatsManagerService;
 
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 90764b0bd4267..97846f2397a5c 100644
--- a/apex/statsd/service/java/com/android/server/stats/StatsManagerService.java
+++ b/apex/statsd/service/java/com/android/server/stats/StatsManagerService.java
@@ -172,6 +172,10 @@ public class StatsManagerService extends IStatsManagerService.Stub {
     public void registerPullAtomCallback(int atomTag, long coolDownMillis, long timeoutMillis,
             int[] additiveFields, IPullAtomCallback pullerCallback) {
         enforceRegisterStatsPullAtomPermission();
+        if (pullerCallback == null) {
+            Log.w(TAG, "Puller callback is null for atom " + atomTag);
+            return;
+        }
         int callingUid = Binder.getCallingUid();
         PullerKey key = new PullerKey(callingUid, atomTag);
         PullerValue val =
diff --git a/cmds/statsd/src/external/StatsPullerManager.cpp b/cmds/statsd/src/external/StatsPullerManager.cpp
index cfd5d14b0d3be..0de7e620adab3 100644
--- a/cmds/statsd/src/external/StatsPullerManager.cpp
+++ b/cmds/statsd/src/external/StatsPullerManager.cpp
@@ -354,6 +354,11 @@ void StatsPullerManager::RegisterPullAtomCallback(const int uid, const int32_t a
     std::lock_guard<std::mutex> _l(mLock);
     VLOG("RegisterPullerCallback: adding puller for tag %d", atomTag);
 
+    if (callback == nullptr) {
+        ALOGW("SetPullAtomCallback called with null callback for atom %d.", atomTag);
+        return;
+    }
+
     StatsdStats::getInstance().notePullerCallbackRegistrationChanged(atomTag, /*registered=*/true);
     int64_t actualCoolDownNs = coolDownNs < kMinCoolDownNs ? kMinCoolDownNs : coolDownNs;
     int64_t actualTimeoutNs = timeoutNs > kMaxTimeoutNs ? kMaxTimeoutNs : timeoutNs;