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
diff --git a/media/libmediametrics/MediaAnalyticsItem.cpp b/media/libmediametrics/MediaAnalyticsItem.cpp
index f968c09..f7df2b4 100644
--- a/media/libmediametrics/MediaAnalyticsItem.cpp
+++ b/media/libmediametrics/MediaAnalyticsItem.cpp
@@ -279,8 +279,10 @@
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 @@
// set the values
void MediaAnalyticsItem::setInt32(MediaAnalyticsItem::Attr name, int32_t value) {
Prop *prop = allocateProp(name);
- prop->mType = kTypeInt32;
- prop->u.int32Value = value;
+ if (prop != NULL) {
+ prop->mType = kTypeInt32;
+ prop->u.int32Value = value;
+ }
}
void MediaAnalyticsItem::setInt64(MediaAnalyticsItem::Attr name, int64_t value) {
Prop *prop = allocateProp(name);
- prop->mType = kTypeInt64;
- prop->u.int64Value = value;
+ if (prop != NULL) {
+ prop->mType = kTypeInt64;
+ prop->u.int64Value = value;
+ }
}
void MediaAnalyticsItem::setDouble(MediaAnalyticsItem::Attr name, double value) {
Prop *prop = allocateProp(name);
- prop->mType = kTypeDouble;
- prop->u.doubleValue = value;
+ 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
- prop->mType = kTypeCString;
- prop->u.CStringValue = strdup(value);
+ 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);
- prop->mType = kTypeRate;
- prop->u.rate.count = count;
- prop->u.rate.duration = duration;
+ 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::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::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::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::growProps(int increment)
+bool MediaAnalyticsItem::growProps(int increment)
{
if (increment <= 0) {
increment = kGrowProps;
@@ -599,6 +623,10 @@
}
mProps = ni;
mPropSize = nsize;
+ return true;
+ } else {
+ ALOGW("MediaAnalyticsItem::growProps fails");
+ return false;
}
}
@@ -963,32 +991,26 @@
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);
- 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;
-
+ if (oprop != NULL) {
+ copyProp(oprop, iprop);
+ } else {
+ ALOGW("dropped property '%s'", iprop->mName);
}
+ } else {
+ copyProp(oprop, iprop);
}
}
diff --git a/media/libmediametrics/include/MediaAnalyticsItem.h b/media/libmediametrics/include/MediaAnalyticsItem.h
index dd7452f..5f9b916 100644
--- a/media/libmediametrics/include/MediaAnalyticsItem.h
+++ b/media/libmediametrics/include/MediaAnalyticsItem.h
@@ -243,7 +243,7 @@
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);
diff --git a/services/mediaanalytics/MediaAnalyticsService.cpp b/services/mediaanalytics/MediaAnalyticsService.cpp
index c7f9270..f08be50 100644
--- a/services/mediaanalytics/MediaAnalyticsService.cpp
+++ b/services/mediaanalytics/MediaAnalyticsService.cpp
@@ -299,6 +299,8 @@
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 @@
// 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 @@
}
// 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 *item = NULL;
- Mutex::Autolock _l(mLock);
-
for (List<MediaAnalyticsItem *>::iterator it = theList->begin();
it != theList->end(); it++) {
MediaAnalyticsItem *tmp = (*it);
@@ -711,10 +711,9 @@
// 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())