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

Commit 9bb7e3b8 authored by Ray Essick's avatar Ray Essick
Browse files

Prevent Race condition in media.metrics 

Change scope of locking to avoid a window of vulnerability on queue
manipulation and item merging. Also cleans up a string manipulation
error exposed by the poc for the queue bug.

Bug: 68015343
Test: repeated running of PoC without crash
Change-Id: Iafa82936e6ec2e25793187e0fa17c2a23085f58f
parent c56f27b2

media/libmediametrics/MediaAnalyticsItem.cpp

+55 −33
Original line number Diff line number Diff line
@@ -279,8 +279,10 @@ MediaAnalyticsItem::Prop *MediaAnalyticsItem::allocateProp(const char *name) { 
        prop = &mProps[i];

    } else {

        if (i == mPropSize) {

            growProps();

            // XXX: verify success

            if (growProps() == false) {

                ALOGE("failed allocation for new props");

                return NULL;

            }

        }

        i = mPropCount++;

        prop = &mProps[i];
@@ -312,41 +314,54 @@ bool MediaAnalyticsItem::removeProp(const char *name) { 
// set the values

void MediaAnalyticsItem::setInt32(MediaAnalyticsItem::Attr name, int32_t value) {

    Prop *prop = allocateProp(name);

    if (prop != NULL) {

        prop->mType = kTypeInt32;

        prop->u.int32Value = value;

    }

}



void MediaAnalyticsItem::setInt64(MediaAnalyticsItem::Attr name, int64_t value) {

    Prop *prop = allocateProp(name);

    if (prop != NULL) {

        prop->mType = kTypeInt64;

        prop->u.int64Value = value;

    }

}



void MediaAnalyticsItem::setDouble(MediaAnalyticsItem::Attr name, double value) {

    Prop *prop = allocateProp(name);

    if (prop != NULL) {

        prop->mType = kTypeDouble;

        prop->u.doubleValue = value;

    }

}



void MediaAnalyticsItem::setCString(MediaAnalyticsItem::Attr name, const char *value) {



    Prop *prop = allocateProp(name);

    // any old value will be gone

    if (prop != NULL) {

        prop->mType = kTypeCString;

        prop->u.CStringValue = strdup(value);

    }

}



void MediaAnalyticsItem::setRate(MediaAnalyticsItem::Attr name, int64_t count, int64_t duration) {

    Prop *prop = allocateProp(name);

    if (prop != NULL) {

        prop->mType = kTypeRate;

        prop->u.rate.count = count;

        prop->u.rate.duration = duration;

    }

}





// find/add/set fused into a single operation

void MediaAnalyticsItem::addInt32(MediaAnalyticsItem::Attr name, int32_t value) {

    Prop *prop = allocateProp(name);

    if (prop == NULL) {

        return;

    }

    switch (prop->mType) {

        case kTypeInt32:

            prop->u.int32Value += value;
@@ -361,6 +376,9 @@ void MediaAnalyticsItem::addInt32(MediaAnalyticsItem::Attr name, int32_t value) 


void MediaAnalyticsItem::addInt64(MediaAnalyticsItem::Attr name, int64_t value) {

    Prop *prop = allocateProp(name);

    if (prop == NULL) {

        return;

    }

    switch (prop->mType) {

        case kTypeInt64:

            prop->u.int64Value += value;
@@ -375,6 +393,9 @@ void MediaAnalyticsItem::addInt64(MediaAnalyticsItem::Attr name, int64_t value) 


void MediaAnalyticsItem::addRate(MediaAnalyticsItem::Attr name, int64_t count, int64_t duration) {

    Prop *prop = allocateProp(name);

    if (prop == NULL) {

        return;

    }

    switch (prop->mType) {

        case kTypeRate:

            prop->u.rate.count += count;
@@ -391,6 +412,9 @@ void MediaAnalyticsItem::addRate(MediaAnalyticsItem::Attr name, int64_t count, i 


void MediaAnalyticsItem::addDouble(MediaAnalyticsItem::Attr name, double value) {

    Prop *prop = allocateProp(name);

    if (prop == NULL) {

        return;

    }

    switch (prop->mType) {

        case kTypeDouble:

            prop->u.doubleValue += value;
@@ -585,7 +609,7 @@ void MediaAnalyticsItem::copyProp(Prop *dst, const Prop *src) 
    }

}



void MediaAnalyticsItem::growProps(int increment)

bool MediaAnalyticsItem::growProps(int increment)

{

    if (increment <= 0) {

        increment = kGrowProps;
@@ -599,6 +623,10 @@ void MediaAnalyticsItem::growProps(int increment) 
        }

        mProps = ni;

        mPropSize = nsize;

        return true;

    } else {

        ALOGW("MediaAnalyticsItem::growProps fails");

        return false;

    }

}
@@ -963,32 +991,26 @@ bool MediaAnalyticsItem::merge(MediaAnalyticsItem *incoming) { 
    int nattr = incoming->mPropCount;

    for (int i = 0 ; i < nattr; i++ ) {

        Prop *iprop = &incoming->mProps[i];

        Prop *oprop = findProp(iprop->mName);

        const char *p = iprop->mName;

        size_t len = strlen(p);

        char semantic = p[len-1];



        // should ignore a zero length name...

        if (len == 0) {

            continue;

        }



        Prop *oprop = findProp(iprop->mName);



        if (oprop == NULL) {

            // no oprop, so we insert the new one

            oprop = allocateProp(p);

            if (oprop != NULL) {

                copyProp(oprop, iprop);

            } else {

            // merge iprop into oprop

            switch (semantic) {

                case '<':       // first  aka keep old)

                    /* nop */

                    break;



                default:        // default is 'last'

                case '>':       // last (aka keep new)

                    copyProp(oprop, iprop);

                    break;



                case '+':       /* sum */

                    // XXX validate numeric types, sum in place

                    break;



                ALOGW("dropped property '%s'", iprop->mName);

            }

        } else {

            copyProp(oprop, iprop);

        }

    }

media/libmediametrics/include/MediaAnalyticsItem.h

+1 −1
Original line number Diff line number Diff line
@@ -243,7 +243,7 @@ class MediaAnalyticsItem { 
        enum {

            kGrowProps = 10

        };

        void growProps(int increment = kGrowProps);

        bool growProps(int increment = kGrowProps);

        size_t findPropIndex(const char *name, size_t len);

        Prop *findProp(const char *name);

        Prop *allocateProp(const char *name);

services/mediaanalytics/MediaAnalyticsService.cpp

+5 −6
Original line number Diff line number Diff line
@@ -299,6 +299,8 @@ MediaAnalyticsItem::SessionID_t MediaAnalyticsService::submit(MediaAnalyticsItem 


    bool finalizing = item->getFinalized();



    Mutex::Autolock _l(mLock);



    // if finalizing, we'll remove it

    MediaAnalyticsItem *oitem = findItem(mOpen, item, finalizing | forcenew);

    if (oitem != NULL) {
@@ -609,10 +611,9 @@ String8 MediaAnalyticsService::dumpQueue(List<MediaAnalyticsItem *> *theList, ns 
// XXX: rewrite this to manage persistence, etc.



// insert appropriately into queue

// caller should hold mLock

void MediaAnalyticsService::saveItem(List<MediaAnalyticsItem *> *l, MediaAnalyticsItem * item, int front) {



    Mutex::Autolock _l(mLock);



    // adding at back of queue (fifo order)

    if (front)  {

        l->push_front(item);
@@ -682,6 +683,7 @@ static bool compatibleItems(MediaAnalyticsItem * oitem, MediaAnalyticsItem * nit 
}



// find the incomplete record that this will overlay

// caller should hold mLock

MediaAnalyticsItem *MediaAnalyticsService::findItem(List<MediaAnalyticsItem*> *theList, MediaAnalyticsItem *nitem, bool removeit) {

    if (nitem == NULL) {

        return NULL;
@@ -689,8 +691,6 @@ MediaAnalyticsItem *MediaAnalyticsService::findItem(List<MediaAnalyticsItem*> *t 


    MediaAnalyticsItem *item = NULL;



    Mutex::Autolock _l(mLock);



    for (List<MediaAnalyticsItem *>::iterator it = theList->begin();

        it != theList->end(); it++) {

        MediaAnalyticsItem *tmp = (*it);
@@ -711,10 +711,9 @@ MediaAnalyticsItem *MediaAnalyticsService::findItem(List<MediaAnalyticsItem*> *t 




// delete the indicated record

// caller should hold mLock

void MediaAnalyticsService::deleteItem(List<MediaAnalyticsItem *> *l, MediaAnalyticsItem *item) {



    Mutex::Autolock _l(mLock);



    for (List<MediaAnalyticsItem *>::iterator it = l->begin();

        it != l->end(); it++) {

        if ((*it)->getSessionID() != item->getSessionID())