From 0308b3971d98e3cebfe8722c86572b628d02f694 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Wed, 15 May 2019 23:38:43 +0900 Subject: [PATCH] NetworkStats: Fix race condition causing system server crashes NetworkStatsService uses an internal boolean to know when it has started for the purpose of preventing access to other internal variables before they are initialized. However that boolean is set to true in systemReady() non-atomically with respect to the initialization of the other variables it guards, which can cause the system server to crash. This patch fixes this concurrency bug by moving setting the internal boolean flag and the variable it guards in one atomic synchronized block. This patch also removes code checking if bandwidth control is enabled, because this is now always true. Bug: 132767673 Test: Compiled. Change-Id: Ia089b5767ce271d669879c975508654d4dd03429 --- .../server/net/NetworkStatsService.java | 62 +++---------------- 1 file changed, 10 insertions(+), 52 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkStatsService.java b/services/core/java/com/android/server/net/NetworkStatsService.java index 2fc78d6f47..5505828063 100644 --- a/services/core/java/com/android/server/net/NetworkStatsService.java +++ b/services/core/java/com/android/server/net/NetworkStatsService.java @@ -373,14 +373,9 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } public void systemReady() { - mSystemReady = true; - - if (!isBandwidthControlEnabled()) { - Slog.w(TAG, "bandwidth controls disabled, unable to track stats"); - return; - } - synchronized (mStatsLock) { + mSystemReady = true; + // create data recorders along with historical rotators mDevRecorder = buildRecorder(PREFIX_DEV, mSettings.getDevConfig(), false); mXtRecorder = buildRecorder(PREFIX_XT, mSettings.getXtConfig(), false); @@ -426,7 +421,14 @@ public class NetworkStatsService extends INetworkStatsService.Stub { // ignored; service lives in system_server } - registerPollAlarmLocked(); + // schedule periodic pall alarm based on {@link NetworkStatsSettings#getPollInterval()}. + final PendingIntent pollIntent = + PendingIntent.getBroadcast(mContext, 0, new Intent(ACTION_NETWORK_STATS_POLL), 0); + + final long currentRealtime = SystemClock.elapsedRealtime(); + mAlarmManager.setInexactRepeating(AlarmManager.ELAPSED_REALTIME, currentRealtime, + mSettings.getPollInterval(), pollIntent); + registerGlobalAlert(); } @@ -486,23 +488,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } } - /** - * Clear any existing {@link #ACTION_NETWORK_STATS_POLL} alarms, and - * reschedule based on current {@link NetworkStatsSettings#getPollInterval()}. - */ - private void registerPollAlarmLocked() { - if (mPollIntent != null) { - mAlarmManager.cancel(mPollIntent); - } - - mPollIntent = PendingIntent.getBroadcast( - mContext, 0, new Intent(ACTION_NETWORK_STATS_POLL), 0); - - final long currentRealtime = SystemClock.elapsedRealtime(); - mAlarmManager.setInexactRepeating(AlarmManager.ELAPSED_REALTIME, currentRealtime, - mSettings.getPollInterval(), mPollIntent); - } - /** * Register for a global alert that is delivered through * {@link INetworkManagementEventObserver} once a threshold amount of data @@ -548,8 +533,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } private INetworkStatsSession openSessionInternal(final int flags, final String callingPackage) { - assertBandwidthControlEnabled(); - final int callingUid = Binder.getCallingUid(); final int usedFlags = isRateLimitedForPoll(callingUid) ? flags & (~NetworkStatsManager.FLAG_POLL_ON_OPEN) @@ -742,7 +725,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { private long getNetworkTotalBytes(NetworkTemplate template, long start, long end) { assertSystemReady(); - assertBandwidthControlEnabled(); // NOTE: if callers want to get non-augmented data, they should go // through the public API @@ -753,7 +735,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { private NetworkStats getNetworkUidBytes(NetworkTemplate template, long start, long end) { assertSystemReady(); - assertBandwidthControlEnabled(); final NetworkStatsCollection uidComplete; synchronized (mStatsLock) { @@ -768,7 +749,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { if (Binder.getCallingUid() != uid) { mContext.enforceCallingOrSelfPermission(ACCESS_NETWORK_STATE, TAG); } - assertBandwidthControlEnabled(); // TODO: switch to data layer stats once kernel exports // for now, read network layer stats and flatten across all ifaces @@ -855,7 +835,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { NetworkState[] networkStates, String activeIface) { checkNetworkStackPermission(mContext); - assertBandwidthControlEnabled(); final long token = Binder.clearCallingIdentity(); try { @@ -868,7 +847,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { @Override public void forceUpdate() { mContext.enforceCallingOrSelfPermission(READ_NETWORK_USAGE_HISTORY, TAG); - assertBandwidthControlEnabled(); final long token = Binder.clearCallingIdentity(); try { @@ -879,8 +857,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } private void advisePersistThreshold(long thresholdBytes) { - assertBandwidthControlEnabled(); - // clamp threshold into safe range mPersistThreshold = MathUtils.constrain(thresholdBytes, 128 * KB_IN_BYTES, 2 * MB_IN_BYTES); if (LOGV) { @@ -1739,24 +1715,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { } } - private void assertBandwidthControlEnabled() { - if (!isBandwidthControlEnabled()) { - throw new IllegalStateException("Bandwidth module disabled"); - } - } - - private boolean isBandwidthControlEnabled() { - final long token = Binder.clearCallingIdentity(); - try { - return mNetworkManager.isBandwidthControlEnabled(); - } catch (RemoteException e) { - // ignored; service lives in system_server - return false; - } finally { - Binder.restoreCallingIdentity(token); - } - } - private class DropBoxNonMonotonicObserver implements NonMonotonicObserver { @Override public void foundNonMonotonic(NetworkStats left, int leftIndex, NetworkStats right,