Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 96dc0c11 authored by Hugo Benichi's avatar Hugo Benichi
Browse files

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
parent f15b5b38

services/core/java/com/android/server/net/NetworkStatsService.java

+10 −52
Original line number Diff line number Diff line
@@ -373,14 +373,9 @@ public class NetworkStatsService extends INetworkStatsService.Stub { 
    }



    public void systemReady() {

        synchronized (mStatsLock) {

            mSystemReady = true;



        if (!isBandwidthControlEnabled()) {

            Slog.w(TAG, "bandwidth controls disabled, unable to track stats");

            return;

        }



        synchronized (mStatsLock) {

            // 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<String> {

        @Override

        public void foundNonMonotonic(NetworkStats left, int leftIndex, NetworkStats right,

tests/net/java/com/android/server/net/NetworkStatsServiceTest.java

+0 −21
Original line number Diff line number Diff line
@@ -239,7 +239,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildWifiState()};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
@@ -283,7 +282,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildWifiState()};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
@@ -357,7 +355,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildWifiState()};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
@@ -398,7 +395,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildMobile3gState(IMSI_1)};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));
@@ -433,7 +429,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
                .addValues(TEST_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1536L, 12L, 512L, 4L, 0L)

                .addValues(TEST_IFACE, UID_RED, SET_DEFAULT, 0xF00D, 512L, 4L, 512L, 4L, 0L)

                .addValues(TEST_IFACE, UID_BLUE, SET_DEFAULT, TAG_NONE, 512L, 4L, 0L, 0L, 0L));

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));

        forcePollAndWaitForIdle();
@@ -473,7 +468,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildWifiState()};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
@@ -531,7 +525,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildMobile3gState(IMSI_1)};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));
@@ -558,7 +551,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        expectNetworkStatsUidDetail(new NetworkStats(getElapsedRealtime(), 1)

                .addValues(TEST_IFACE, UID_RED, SET_DEFAULT, TAG_NONE, 1024L, 8L, 1024L, 8L, 0L)

                .addValues(TEST_IFACE, UID_RED, SET_DEFAULT, 0xF00D, 512L, 4L, 512L, 4L, 0L));

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));

        forcePollAndWaitForIdle();
@@ -588,7 +580,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildWifiState()};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
@@ -646,7 +637,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildWifiState()};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
@@ -690,7 +680,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 


        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
@@ -740,7 +729,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildWifiState()};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
@@ -797,7 +785,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildWifiState(true /* isMetered */)};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
@@ -837,7 +824,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
            new NetworkState[] {buildMobile3gState(IMSI_1, true /* isRoaming */)};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));
@@ -875,7 +861,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildMobile3gState(IMSI_1)};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_MOBILE, states, getActiveIface(states));
@@ -916,7 +901,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        NetworkState[] states = new NetworkState[] {buildWifiState()};

        expectNetworkStatsSummary(buildEmptyStats());

        expectNetworkStatsUidDetail(buildEmptyStats());

        expectBandwidthControlCheck();



        mService.forceUpdateIfaces(NETWORKS_WIFI, states, getActiveIface(states));
@@ -1057,7 +1041,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 


    private void expectSystemReady() throws Exception {

        expectNetworkStatsSummary(buildEmptyStats());

        expectBandwidthControlCheck();

    }



    private String getActiveIface(NetworkState... states) throws Exception {
@@ -1125,10 +1108,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { 
        when(mSettings.getUidTagPersistBytes(anyLong())).thenReturn(MB_IN_BYTES);

    }



    private void expectBandwidthControlCheck() throws Exception {

        when(mNetManager.isBandwidthControlEnabled()).thenReturn(true);

    }



    private void assertStatsFilesExist(boolean exist) {

        final File basePath = new File(mStatsDir, "netstats");

        if (exist) {