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

Commit 9ec159ac authored by Tej Singh's avatar Tej Singh
Browse files

Fix Race Condition 

Currently, it is possible for two threads in statsd to concurrently
access/modify memory in ConditionTrackers since they do not have locks.
This happens when one thread is processing LogEvents (lock on
StatsLogProcessor mutex), while the other thread receives uidmap updates
and locks on the mutex in the MetricProducer. This Cl changes uidmap
updates to also go through the mutex in StatsLogProcessor.

Test: bit statsd_test:*
Test: atest CtsStatsdHostTestCases
Test: local test (ag/9725088) that forced the race condition now passes
Bug: 144373785
Change-Id: I04ae2f7ed025f5ce8bc4fdeb7f10717e20d76282
parent e9513c5e

cmds/statsd/src/StatsLogProcessor.cpp

+26 −5
Original line number Diff line number Diff line
@@ -328,11 +328,6 @@ void StatsLogProcessor::OnConfigUpdatedLocked( 
                               mAnomalyAlarmMonitor, mPeriodicAlarmMonitor);

    if (newMetricsManager->isConfigValid()) {

        mUidMap->OnConfigUpdated(key);

        if (newMetricsManager->shouldAddUidMapListener()) {

            // We have to add listener after the MetricsManager is constructed because it's

            // not safe to create wp or sp from this pointer inside its constructor.

            mUidMap->addListener(newMetricsManager.get());

        }

        newMetricsManager->refreshTtl(timestampNs);

        mMetricsManagers[key] = newMetricsManager;

        VLOG("StatsdConfig valid");
@@ -753,6 +748,32 @@ int64_t StatsLogProcessor::getLastReportTimeNs(const ConfigKey& key) { 
    }

}



void StatsLogProcessor::notifyAppUpgrade(const int64_t& eventTimeNs, const string& apk,

                                         const int uid, const int64_t version) {

    std::lock_guard<std::mutex> lock(mMetricsMutex);

    ALOGW("Received app upgrade");

    for (auto it : mMetricsManagers) {

        it.second->notifyAppUpgrade(eventTimeNs, apk, uid, version);

    }

}



void StatsLogProcessor::notifyAppRemoved(const int64_t& eventTimeNs, const string& apk,

                                         const int uid) {

    std::lock_guard<std::mutex> lock(mMetricsMutex);

    ALOGW("Received app removed");

    for (auto it : mMetricsManagers) {

        it.second->notifyAppRemoved(eventTimeNs, apk, uid);

    }

}



void StatsLogProcessor::onUidMapReceived(const int64_t& eventTimeNs) {

    std::lock_guard<std::mutex> lock(mMetricsMutex);

    ALOGW("Received uid map");

    for (auto it : mMetricsManagers) {

        it.second->onUidMapReceived(eventTimeNs);

    }

}



void StatsLogProcessor::noteOnDiskData(const ConfigKey& key) {

    std::lock_guard<std::mutex> lock(mMetricsMutex);

    mOnDiskDataConfigs.insert(key);

cmds/statsd/src/StatsLogProcessor.h

+11 −1
Original line number Diff line number Diff line
@@ -32,7 +32,7 @@ namespace os { 
namespace statsd {





class StatsLogProcessor : public ConfigListener {

class StatsLogProcessor : public ConfigListener, public virtual PackageInfoListener {

public:

    StatsLogProcessor(const sp<UidMap>& uidMap, const sp<StatsPullerManager>& pullerManager,

                      const sp<AlarmMonitor>& anomalyAlarmMonitor,
@@ -91,6 +91,16 @@ public: 
    /* Sets the active status/ttl for all configs and metrics to the status in ActiveConfigList. */

    void SetConfigsActiveState(const ActiveConfigList& activeConfigList, int64_t currentTimeNs);



    /* Notify all MetricsManagers of app upgrades */

    void notifyAppUpgrade(const int64_t& eventTimeNs, const string& apk, const int uid,

                          const int64_t version) override;



    /* Notify all MetricsManagers of app removals */

    void notifyAppRemoved(const int64_t& eventTimeNs, const string& apk, const int uid) override;



    /* Notify all MetricsManagers of uid map snapshots received */

    void onUidMapReceived(const int64_t& eventTimeNs) override;



    // Reset all configs.

    void resetConfigs();

cmds/statsd/src/StatsService.cpp

+1 −0
Original line number Diff line number Diff line
@@ -200,6 +200,7 @@ StatsService::StatsService(const sp<Looper>& handlerLooper, shared_ptr<LogEventQ 
                }

            });



    mUidMap->setListener(mProcessor);

    mConfigManager->AddListener(mProcessor);



    init_system_properties();

cmds/statsd/src/metrics/MetricProducer.h

+4 −9
Original line number Diff line number Diff line
@@ -87,7 +87,7 @@ struct Activation { 
// writing the report to dropbox. MetricProducers should respond to package changes as required in

// PackageInfoListener, but if none of the metrics are slicing by package name, then the update can

// be a no-op.

class MetricProducer : public virtual PackageInfoListener, public virtual StateListener {

class MetricProducer : public virtual android::RefBase, public virtual StateListener {

public:

    MetricProducer(const int64_t& metricId, const ConfigKey& key, const int64_t timeBaseNs,

                   const int conditionIndex, const sp<ConditionWizard>& wizard,
@@ -109,8 +109,8 @@ public: 
     * the flush again when the end timestamp is forced to be now, and then after flushing, update

     * the start timestamp to be now.

     */

    void notifyAppUpgrade(const int64_t& eventTimeNs, const string& apk, const int uid,

                          const int64_t version) override {

    virtual void notifyAppUpgrade(const int64_t& eventTimeNs, const string& apk, const int uid,

                          const int64_t version) {

        std::lock_guard<std::mutex> lock(mMutex);



        if (eventTimeNs > getCurrentBucketEndTimeNs()) {
@@ -123,16 +123,11 @@ public: 
        // is a partial bucket and can merge it with the previous bucket.

    };



    void notifyAppRemoved(const int64_t& eventTimeNs, const string& apk, const int uid) override{

    void notifyAppRemoved(const int64_t& eventTimeNs, const string& apk, const int uid) {

        // Force buckets to split on removal also.

        notifyAppUpgrade(eventTimeNs, apk, uid, 0);

    };



    void onUidMapReceived(const int64_t& eventTimeNs) override{

            // Purposefully don't flush partial buckets on a new snapshot.

            // This occurs if a new user is added/removed or statsd crashes.

    };



    // Consume the parsed stats log entry that already matched the "what" of the metric.

    void onMatchedLogEvent(const size_t matcherIndex, const LogEvent& event) {

        std::lock_guard<std::mutex> lock(mMutex);

cmds/statsd/src/metrics/MetricsManager.cpp

+11 −0
Original line number Diff line number Diff line
@@ -182,6 +182,10 @@ bool MetricsManager::isConfigValid() const { 


void MetricsManager::notifyAppUpgrade(const int64_t& eventTimeNs, const string& apk, const int uid,

                                      const int64_t version) {

    // Inform all metric producers.

    for (auto it : mAllMetricProducers) {

        it->notifyAppUpgrade(eventTimeNs, apk, uid, version);

    }

    // check if we care this package

    if (std::find(mAllowedPkg.begin(), mAllowedPkg.end(), apk) == mAllowedPkg.end()) {

        return;
@@ -193,6 +197,10 @@ void MetricsManager::notifyAppUpgrade(const int64_t& eventTimeNs, const string& 


void MetricsManager::notifyAppRemoved(const int64_t& eventTimeNs, const string& apk,

                                      const int uid) {

    // Inform all metric producers.

    for (auto it : mAllMetricProducers) {

        it->notifyAppRemoved(eventTimeNs, apk, uid);

    }

    // check if we care this package

    if (std::find(mAllowedPkg.begin(), mAllowedPkg.end(), apk) == mAllowedPkg.end()) {

        return;
@@ -203,6 +211,9 @@ void MetricsManager::notifyAppRemoved(const int64_t& eventTimeNs, const string& 
}



void MetricsManager::onUidMapReceived(const int64_t& eventTimeNs) {

    // Purposefully don't inform metric producers on a new snapshot

    // because we don't need to flush partial buckets.

    // This occurs if a new user is added/removed or statsd crashes.

    if (mAllowedPkg.size() == 0) {

        return;

    }