MediaMetrics: Update Item serialization code

Add for bounds checking errors
Update key checks to omit length, use C primitive types

Test: atest mediametrics_tests, adb shell dumpsys media.metrics
Change-Id: I1e6d5bd7a9611f6d59e15a1dbebc646405e5a018

media/libmediametrics/IMediaAnalyticsService.cpp

@@ -55,13 +55,17 @@ public:
 
         Parcel data;
         data.writeInterfaceToken(IMediaAnalyticsService::getInterfaceDescriptor());
-        item->writeToParcel(&data);
 
-        status_t err = remote()->transact(
+        status_t status = item->writeToParcel(&data);
+        if (status != NO_ERROR) { // assume failure logged in item
+            return status;
+        }
+
+        status = remote()->transact(
                 SUBMIT_ITEM_ONEWAY, data, nullptr /* reply */, IBinder::FLAG_ONEWAY);
-        ALOGW_IF(err != NO_ERROR, "%s: bad response from service for submit, err=%d",
-                __func__, err);
-        return err;
+        ALOGW_IF(status != NO_ERROR, "%s: bad response from service for submit, status=%d",
+                __func__, status);
+        return status;
     }
 };
 
@@ -79,11 +83,14 @@ status_t BnMediaAnalyticsService::onTransact(
         CHECK_INTERFACE(IMediaAnalyticsService, data, reply);
 
         MediaAnalyticsItem * const item = MediaAnalyticsItem::create();
-        if (item->readFromParcel(data) < 0) {
-            return BAD_VALUE;
+        status_t status = item->readFromParcel(data);
+        if (status != NO_ERROR) { // assume failure logged in item
+            return status;
         }
+        // TODO: remove this setPid.
         item->setPid(clientPid);
-        const status_t status __unused = submitInternal(item, true /* release */);
+        status = submitInternal(item, true /* release */);
+        // assume failure logged by submitInternal
         return NO_ERROR;
     } break;
 

media/libmediametrics/MediaAnalyticsItem.cpp

@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-#undef LOG_TAG
 #define LOG_TAG "MediaAnalyticsItem"
 
 #include <inttypes.h>
@@ -23,6 +22,7 @@
 #include <sys/types.h>
 
 #include <mutex>
+#include <set>
 
 #include <binder/Parcel.h>
 #include <utils/Errors.h>
@@ -45,18 +45,6 @@ namespace android {
 // the service is off.
 #define SVC_TRIES               2
 
-// So caller doesn't need to know size of allocated space
-MediaAnalyticsItem *MediaAnalyticsItem::create()
-{
-    return MediaAnalyticsItem::create(kKeyNone);
-}
-
-MediaAnalyticsItem *MediaAnalyticsItem::create(MediaAnalyticsItem::Key key)
-{
-    MediaAnalyticsItem *item = new MediaAnalyticsItem(key);
-    return item;
-}
-
 MediaAnalyticsItem* MediaAnalyticsItem::convert(mediametrics_handle_t handle) {
     MediaAnalyticsItem *item = (android::MediaAnalyticsItem *) handle;
     return item;
@@ -159,64 +147,50 @@ int64_t MediaAnalyticsItem::getPkgVersionCode() const {
     return mPkgVersionCode;
 }
 
-// this key is for the overall record -- "codec", "player", "drm", etc
-MediaAnalyticsItem &MediaAnalyticsItem::setKey(MediaAnalyticsItem::Key key) {
-    mKey = key;
-    return *this;
-}
-
-// number of attributes we have in this record
-int32_t MediaAnalyticsItem::count() const {
-    return mPropCount;
-}
 
 // find the proper entry in the list
-size_t MediaAnalyticsItem::findPropIndex(const char *name, size_t len) const
+size_t MediaAnalyticsItem::findPropIndex(const char *name) const
 {
     size_t i = 0;
     for (; i < mPropCount; i++) {
-        if (mProps[i].isNamed(name, len)) break;
+        if (mProps[i].isNamed(name)) break;
     }
     return i;
 }
 
 MediaAnalyticsItem::Prop *MediaAnalyticsItem::findProp(const char *name) const {
-    size_t len = strlen(name);
-    size_t i = findPropIndex(name, len);
+    const size_t i = findPropIndex(name);
     if (i < mPropCount) {
         return &mProps[i];
     }
-    return NULL;
+    return nullptr;
 }
 
 // consider this "find-or-allocate".
 // caller validates type and uses clearPropValue() accordingly
 MediaAnalyticsItem::Prop *MediaAnalyticsItem::allocateProp(const char *name) {
-    size_t len = strlen(name);
-    size_t i = findPropIndex(name, len);
-    Prop *prop;
-
+    const size_t i = findPropIndex(name);
     if (i < mPropCount) {
-        prop = &mProps[i];
-    } else {
-        if (i == mPropSize) {
-            if (growProps() == false) {
-                ALOGE("failed allocation for new props");
-                return NULL;
-            }
-        }
-        i = mPropCount++;
-        prop = &mProps[i];
-        prop->setName(name, len);
+        return &mProps[i]; // already have it, return
     }
 
+    Prop *prop = allocateProp(); // get a new prop
+    if (prop == nullptr) return nullptr;
+    prop->setName(name);
     return prop;
 }
 
+MediaAnalyticsItem::Prop *MediaAnalyticsItem::allocateProp() {
+    if (mPropCount == mPropSize && growProps() == false) {
+        ALOGE("%s: failed allocation for new properties", __func__);
+        return nullptr;
+    }
+    return &mProps[mPropCount++];
+}
+
 // used within the summarizers; return whether property existed
 bool MediaAnalyticsItem::removeProp(const char *name) {
-    size_t len = strlen(name);
-    size_t i = findPropIndex(name, len);
+    const size_t i = findPropIndex(name);
     if (i < mPropCount) {
         mProps[i].clear();
         if (i != mPropCount-1) {
@@ -231,19 +205,15 @@ bool MediaAnalyticsItem::removeProp(const char *name) {
 
 // remove indicated keys and their values
 // return value is # keys removed
-int32_t MediaAnalyticsItem::filter(int n, MediaAnalyticsItem::Attr attrs[]) {
-    int zapped = 0;
-    if (attrs == NULL || n <= 0) {
-        return -1;
-    }
-    for (ssize_t i = 0 ; i < n ;  i++) {
+size_t MediaAnalyticsItem::filter(size_t n, const char *attrs[]) {
+    size_t zapped = 0;
+    for (size_t i = 0; i < n; ++i) {
         const char *name = attrs[i];
-        size_t len = strlen(name);
-        size_t j = findPropIndex(name, len);
+        size_t j = findPropIndex(name);
         if (j >= mPropCount) {
             // not there
             continue;
-        } else if (j+1 == mPropCount) {
+        } else if (j + 1 == mPropCount) {
             // last one, shorten
             zapped++;
             mProps[j].clear();
@@ -261,35 +231,31 @@ int32_t MediaAnalyticsItem::filter(int n, MediaAnalyticsItem::Attr attrs[]) {
 
 // remove any keys NOT in the provided list
 // return value is # keys removed
-int32_t MediaAnalyticsItem::filterNot(int n, MediaAnalyticsItem::Attr attrs[]) {
-    int zapped = 0;
-    if (attrs == NULL || n <= 0) {
-        return -1;
-    }
-    for (ssize_t i = mPropCount-1 ; i >=0 ;  i--) {
-        Prop *prop = &mProps[i];
-        for (ssize_t j = 0; j < n ; j++) {
-            if (prop->isNamed(attrs[j])) {
-                prop->clear();
-                zapped++;
-                if (i != (ssize_t)(mPropCount-1)) {
-                    *prop = mProps[mPropCount-1];
-                }
-                mProps[mPropCount-1].clear();
-                mPropCount--;
-                break;
-            }
+size_t MediaAnalyticsItem::filterNot(size_t n, const char *attrs[]) {
+    std::set<std::string> check(attrs, attrs + n);
+    size_t zapped = 0;
+    for (size_t j = 0; j < mPropCount;) {
+        if (check.find(mProps[j].getName()) != check.end()) {
+            ++j;
+            continue;
+        }
+        if (j + 1 == mPropCount) {
+            // last one, shorten
+            zapped++;
+            mProps[j].clear();
+            mPropCount--;
+            break;
+        } else {
+            // in the middle, bring last one down and shorten
+            zapped++;
+            mProps[j].clear();
+            mProps[j] = mProps[mPropCount-1];
+            mPropCount--;
         }
     }
     return zapped;
 }
 
-// remove a single key
-// return value is 0 (not found) or 1 (found and removed)
-int32_t MediaAnalyticsItem::filter(MediaAnalyticsItem::Attr name) {
-    return filter(1, &name);
-}
-
 bool MediaAnalyticsItem::growProps(int increment)
 {
     if (increment <= 0) {
@@ -314,98 +280,77 @@ bool MediaAnalyticsItem::growProps(int increment)
 // Parcel / serialize things for binder calls
 //
 
-int32_t MediaAnalyticsItem::readFromParcel(const Parcel& data) {
-    int32_t version = data.readInt32();
+status_t MediaAnalyticsItem::readFromParcel(const Parcel& data) {
+    int32_t version;
+    status_t status = data.readInt32(&version);
+    if (status != NO_ERROR) return status;
 
-    switch(version) {
-        case 0:
-          return readFromParcel0(data);
-          break;
-        default:
-          ALOGE("Unsupported MediaAnalyticsItem Parcel version: %d", version);
-          return -1;
+    switch (version) {
+    case 0:
+      return readFromParcel0(data);
+    default:
+      ALOGE("%s: unsupported parcel version: %d", __func__, version);
+      return INVALID_OPERATION;
     }
 }
 
-int32_t MediaAnalyticsItem::readFromParcel0(const Parcel& data) {
-    // into 'this' object
-    // .. we make a copy of the string to put away.
-    mKey = data.readCString();
-    mPid = data.readInt32();
-    mUid = data.readInt32();
-    mPkgName = data.readCString();
-    mPkgVersionCode = data.readInt64();
-    // We no longer pay attention to user setting of finalized, BUT it's
-    // still part of the wire packet -- so read & discard.
-    mTimestamp = data.readInt64();
-
-    int count = data.readInt32();
+status_t MediaAnalyticsItem::readFromParcel0(const Parcel& data) {
+    const char *s = data.readCString();
+    mKey = s == nullptr ? "" : s;
+    int32_t pid, uid;
+    status_t status = data.readInt32(&pid) ?: data.readInt32(&uid);
+    if (status != NO_ERROR) return status;
+    mPid = (pid_t)pid;
+    mUid = (uid_t)uid;
+    s = data.readCString();
+    mPkgName = s == nullptr ? "" : s;
+    int32_t count;
+    int64_t version, timestamp;
+    status = data.readInt64(&version) ?: data.readInt64(&timestamp) ?: data.readInt32(&count);
+    if (status != NO_ERROR) return status;
+    if (count < 0) return BAD_VALUE;
+    mPkgVersionCode = version;
+    mTimestamp = timestamp;
     for (int i = 0; i < count ; i++) {
-            MediaAnalyticsItem::Attr attr = data.readCString();
-            int32_t ztype = data.readInt32();
-                switch (ztype) {
-                    case MediaAnalyticsItem::kTypeInt32:
-                            setInt32(attr, data.readInt32());
-                            break;
-                    case MediaAnalyticsItem::kTypeInt64:
-                            setInt64(attr, data.readInt64());
-                            break;
-                    case MediaAnalyticsItem::kTypeDouble:
-                            setDouble(attr, data.readDouble());
-                            break;
-                    case MediaAnalyticsItem::kTypeCString:
-                            setCString(attr, data.readCString());
-                            break;
-                    case MediaAnalyticsItem::kTypeRate:
-                            {
-                                int64_t count = data.readInt64();
-                                int64_t duration = data.readInt64();
-                                setRate(attr, count, duration);
-                            }
-                            break;
-                    default:
-                            ALOGE("reading bad item type: %d, idx %d",
-                                  ztype, i);
-                            return -1;
-                }
-    }
-
-    return 0;
-}
-
-int32_t MediaAnalyticsItem::writeToParcel(Parcel *data) {
-
-    if (data == NULL) return -1;
-
-    int32_t version = 0;
-    data->writeInt32(version);
-
-    switch(version) {
-        case 0:
-          return writeToParcel0(data);
-          break;
-        default:
-          ALOGE("Unsupported MediaAnalyticsItem Parcel version: %d", version);
-          return -1;
-    }
-}
-
-int32_t MediaAnalyticsItem::writeToParcel0(Parcel *data) {
-
-    data->writeCString(mKey.c_str());
-    data->writeInt32(mPid);
-    data->writeInt32(mUid);
-    data->writeCString(mPkgName.c_str());
-    data->writeInt64(mPkgVersionCode);
-    data->writeInt64(mTimestamp);
+        Prop *prop = allocateProp();
+        status_t status = prop->readFromParcel(data);
+        if (status != NO_ERROR) return status;
+    }
+    return NO_ERROR;
+}
 
-    // set of items
-    const size_t count = mPropCount;
-    data->writeInt32(count);
-    for (size_t i = 0 ; i < count; i++ ) {
-        mProps[i].writeToParcel(data);
+status_t MediaAnalyticsItem::writeToParcel(Parcel *data) const {
+    if (data == nullptr) return BAD_VALUE;
+
+    const int32_t version = 0;
+    status_t status = data->writeInt32(version);
+    if (status != NO_ERROR) return status;
+
+    switch (version) {
+    case 0:
+      return writeToParcel0(data);
+    default:
+      ALOGE("%s: unsupported parcel version: %d", __func__, version);
+      return INVALID_OPERATION;
+    }
+}
+
+status_t MediaAnalyticsItem::writeToParcel0(Parcel *data) const {
+    status_t status =
+        data->writeCString(mKey.c_str())
+        ?: data->writeInt32(mPid)
+        ?: data->writeInt32(mUid)
+        ?: data->writeCString(mPkgName.c_str())
+        ?: data->writeInt64(mPkgVersionCode)
+        ?: data->writeInt64(mTimestamp);
+    if (status != NO_ERROR) return status;
+
+    data->writeInt32((int32_t)mPropCount);
+    for (size_t i = 0 ; i < mPropCount; ++i) {
+        status = mProps[i].writeToParcel(data);
+        if (status != NO_ERROR) return status;
     }
-    return 0;
+    return NO_ERROR;
 }
 
 const char *MediaAnalyticsItem::toCString() {
@@ -506,7 +451,6 @@ bool MediaAnalyticsItem::selfrecord() {
     }
 }
 
-
 //static
 bool MediaAnalyticsItem::isEnabled() {
     // completely skip logging from certain UIDs. We do this here
@@ -634,199 +578,282 @@ bool MediaAnalyticsItem::merge(MediaAnalyticsItem *incoming) {
     return true;
 }
 
-// a byte array; contents are
-// overall length (uint32) including the length field itself
-// encoding version (uint32)
-// count of properties (uint32)
-// N copies of:
-//     property name as length(int16), bytes
-//         the bytes WILL include the null terminator of the name
-//     type (uint8 -- 1 byte)
-//     size of value field (int16 -- 2 bytes)
-//     value (size based on type)
-//       int32, int64, double -- little endian 4/8/8 bytes respectively
-//       cstring -- N bytes of value [WITH terminator]
+namespace {
 
-enum { kInt32 = 0, kInt64, kDouble, kRate, kCString};
-
-bool MediaAnalyticsItem::dumpAttributes(char **pbuffer, size_t *plength) {
+template <typename T>
+status_t insert(const T& val, char **bufferpptr, char *bufferptrmax)
+{
+    const size_t size = sizeof(val);
+    if (*bufferpptr + size > bufferptrmax) {
+        ALOGE("%s: buffer exceeded with size %zu", __func__, size);
+        return BAD_VALUE;
+    }
+    memcpy(*bufferpptr, &val, size);
+    *bufferpptr += size;
+    return NO_ERROR;
+}
 
-    char *build = NULL;
+template <>
+status_t insert(const char * const& val, char **bufferpptr, char *bufferptrmax)
+{
+    const size_t size = strlen(val) + 1;
+    if (size > UINT16_MAX || *bufferpptr + size > bufferptrmax) {
+        ALOGE("%s: buffer exceeded with size %zu", __func__, size);
+        return BAD_VALUE;
+    }
+    memcpy(*bufferpptr, val, size);
+    *bufferpptr += size;
+    return NO_ERROR;
+}
 
-    if (pbuffer == NULL || plength == NULL)
-        return false;
+template <>
+ __unused
+status_t insert(char * const& val, char **bufferpptr, char *bufferptrmax)
+{
+    return insert((const char *)val, bufferpptr, bufferptrmax);
+}
 
-    // consistency for the caller, who owns whatever comes back in this pointer.
-    *pbuffer = NULL;
+template <typename T>
+status_t extract(T *val, const char **bufferpptr, const char *bufferptrmax)
+{
+    const size_t size = sizeof(*val);
+    if (*bufferpptr + size > bufferptrmax) {
+        ALOGE("%s: buffer exceeded with size %zu", __func__, size);
+        return BAD_VALUE;
+    }
+    memcpy(val, *bufferpptr, size);
+    *bufferpptr += size;
+    return NO_ERROR;
+}
 
-    // first, let's calculate sizes
-    int32_t goal = 0;
-    int32_t version = 0;
+template <>
+status_t extract(char **val, const char **bufferpptr, const char *bufferptrmax)
+{
+    const char *ptr = *bufferpptr;
+    while (*ptr != 0) {
+        if (ptr >= bufferptrmax) {
+            ALOGE("%s: buffer exceeded", __func__);
+        }
+        ++ptr;
+    }
+    const size_t size = (ptr - *bufferpptr) + 1;
+    *val = (char *)malloc(size);
+    memcpy(*val, *bufferpptr, size);
+    *bufferpptr += size;
+    return NO_ERROR;
+}
 
-    goal += sizeof(uint32_t);   // overall length, including the length field
-    goal += sizeof(uint32_t);   // encoding version
-    goal += sizeof(uint32_t);   // # properties
+} // namespace
 
-    int32_t count = mPropCount;
-    for (int i = 0 ; i < count; i++ ) {
-        Prop *prop = &mProps[i];
-        goal += sizeof(uint16_t);           // name length
-        goal += strlen(prop->mName) + 1;    // string + null
-        goal += sizeof(uint8_t);            // type
-        goal += sizeof(uint16_t);           // size of value
-        switch (prop->mType) {
-            case MediaAnalyticsItem::kTypeInt32:
-                    goal += sizeof(uint32_t);
-                    break;
-            case MediaAnalyticsItem::kTypeInt64:
-                    goal += sizeof(uint64_t);
-                    break;
-            case MediaAnalyticsItem::kTypeDouble:
-                    goal += sizeof(double);
-                    break;
-            case MediaAnalyticsItem::kTypeRate:
-                    goal += 2 * sizeof(uint64_t);
-                    break;
-            case MediaAnalyticsItem::kTypeCString:
-                    // length + actual string + null
-                    goal += strlen(prop->u.CStringValue) + 1;
-                    break;
-            default:
-                    ALOGE("found bad Prop type: %d, idx %d, name %s",
-                          prop->mType, i, prop->mName);
-                    return false;
+status_t MediaAnalyticsItem::writeToByteString(char **pbuffer, size_t *plength) const
+{
+    if (pbuffer == nullptr || plength == nullptr)
+        return BAD_VALUE;
+
+    // get size
+    const size_t keySizeZeroTerminated = strlen(mKey.c_str()) + 1;
+    if (keySizeZeroTerminated > UINT16_MAX) {
+        ALOGW("%s: key size %zu too large", __func__, keySizeZeroTerminated);
+        return INVALID_OPERATION;
+    }
+    const uint16_t version = 0;
+    const uint32_t header_len =
+        sizeof(uint32_t)     // overall length
+        + sizeof(header_len) // header length
+        + sizeof(version)    // encoding version
+        + sizeof(uint16_t)   // key length
+        + keySizeZeroTerminated // key, zero terminated
+        + sizeof(int32_t)    // pid
+        + sizeof(int32_t)    // uid
+        + sizeof(int64_t)    // timestamp
+        ;
+
+    uint32_t len = header_len
+        + sizeof(uint32_t) // # properties
+        ;
+    for (size_t i = 0 ; i < mPropCount; ++i) {
+        const size_t size = mProps[i].getByteStringSize();
+        if (size > UINT_MAX - 1) {
+            ALOGW("%s: prop %zu has size %zu", __func__, i, size);
+            return INVALID_OPERATION;
         }
+        len += size;
     }
 
-    // now that we have a size... let's allocate and fill
-    build = (char *)malloc(goal);
-    if (build == NULL)
-        return false;
+    // TODO: consider package information and timestamp.
 
-    memset(build, 0, goal);
+    // now that we have a size... let's allocate and fill
+    char *build = (char *)calloc(1 /* nmemb */, len);
+    if (build == nullptr) return NO_MEMORY;
 
     char *filling = build;
-
-#define _INSERT(val, size) \
-    { memcpy(filling, &(val), (size)); filling += (size);}
-#define _INSERTSTRING(val, size) \
-    { memcpy(filling, (val), (size)); filling += (size);}
-
-    _INSERT(goal, sizeof(int32_t));
-    _INSERT(version, sizeof(int32_t));
-    _INSERT(count, sizeof(int32_t));
-
-    for (int i = 0 ; i < count; i++ ) {
-        Prop *prop = &mProps[i];
-        int16_t attrNameLen = strlen(prop->mName) + 1;
-        _INSERT(attrNameLen, sizeof(int16_t));
-        _INSERTSTRING(prop->mName, attrNameLen);    // termination included
-        int8_t elemtype;
-        int16_t elemsize;
-        switch (prop->mType) {
-            case MediaAnalyticsItem::kTypeInt32:
-                {
-                    elemtype = kInt32;
-                    _INSERT(elemtype, sizeof(int8_t));
-                    elemsize = sizeof(int32_t);
-                    _INSERT(elemsize, sizeof(int16_t));
-
-                    _INSERT(prop->u.int32Value, sizeof(int32_t));
-                    break;
-                }
-            case MediaAnalyticsItem::kTypeInt64:
-                {
-                    elemtype = kInt64;
-                    _INSERT(elemtype, sizeof(int8_t));
-                    elemsize = sizeof(int64_t);
-                    _INSERT(elemsize, sizeof(int16_t));
-
-                    _INSERT(prop->u.int64Value, sizeof(int64_t));
-                    break;
-                }
-            case MediaAnalyticsItem::kTypeDouble:
-                {
-                    elemtype = kDouble;
-                    _INSERT(elemtype, sizeof(int8_t));
-                    elemsize = sizeof(double);
-                    _INSERT(elemsize, sizeof(int16_t));
-
-                    _INSERT(prop->u.doubleValue, sizeof(double));
-                    break;
-                }
-            case MediaAnalyticsItem::kTypeRate:
-                {
-                    elemtype = kRate;
-                    _INSERT(elemtype, sizeof(int8_t));
-                    elemsize = 2 * sizeof(uint64_t);
-                    _INSERT(elemsize, sizeof(int16_t));
-
-                    _INSERT(prop->u.rate.count, sizeof(uint64_t));
-                    _INSERT(prop->u.rate.duration, sizeof(uint64_t));
-                    break;
-                }
-            case MediaAnalyticsItem::kTypeCString:
-                {
-                    elemtype = kCString;
-                    _INSERT(elemtype, sizeof(int8_t));
-                    elemsize = strlen(prop->u.CStringValue) + 1;
-                    _INSERT(elemsize, sizeof(int16_t));
-
-                    _INSERTSTRING(prop->u.CStringValue, elemsize);
-                    break;
-                }
-            default:
-                    // error if can't encode; warning if can't decode
-                    ALOGE("found bad Prop type: %d, idx %d, name %s",
-                          prop->mType, i, prop->mName);
-                    goto badness;
+    char *buildmax = build + len;
+    if (insert(len, &filling, buildmax) != NO_ERROR
+            || insert(header_len, &filling, buildmax) != NO_ERROR
+            || insert(version, &filling, buildmax) != NO_ERROR
+            || insert((uint16_t)keySizeZeroTerminated, &filling, buildmax) != NO_ERROR
+            || insert(mKey.c_str(), &filling, buildmax) != NO_ERROR
+            || insert((int32_t)mPid, &filling, buildmax) != NO_ERROR
+            || insert((int32_t)mUid, &filling, buildmax) != NO_ERROR
+            || insert((int64_t)mTimestamp, &filling, buildmax) != NO_ERROR
+            || insert((uint32_t)mPropCount, &filling, buildmax) != NO_ERROR) {
+        ALOGD("%s:could not write header", __func__);
+        free(build);
+        return INVALID_OPERATION;
+    }
+    for (size_t i = 0 ; i < mPropCount; ++i) {
+        if (mProps[i].writeToByteString(&filling, buildmax) != NO_ERROR) {
+            free(build);
+            ALOGD("%s:could not write prop %zu of %zu", __func__, i, mPropCount);
+            return INVALID_OPERATION;
         }
     }
 
-    if (build + goal != filling) {
+    if (filling != buildmax) {
         ALOGE("problems populating; wrote=%d planned=%d",
-              (int)(filling-build), goal);
-        goto badness;
+              (int)(filling - build), len);
+        free(build);
+        return INVALID_OPERATION;
     }
-
     *pbuffer = build;
-    *plength = goal;
-
-    return true;
+    *plength = len;
+    return NO_ERROR;
+}
 
-  badness:
-    free(build);
-    return false;
+status_t MediaAnalyticsItem::readFromByteString(const char *bufferptr, size_t length)
+{
+    if (bufferptr == nullptr) return BAD_VALUE;
+
+    const char *read = bufferptr;
+    const char *readend = bufferptr + length;
+
+    uint32_t len;
+    uint32_t header_len;
+    int16_t version;
+    int16_t key_len;
+    char *key = nullptr;
+    int32_t pid;
+    int32_t uid;
+    int64_t timestamp;
+    uint32_t propCount;
+    if (extract(&len, &read, readend) != NO_ERROR
+            || extract(&header_len, &read, readend) != NO_ERROR
+            || extract(&version, &read, readend) != NO_ERROR
+            || extract(&key_len, &read, readend) != NO_ERROR
+            || extract(&key, &read, readend) != NO_ERROR
+            || extract(&pid, &read, readend) != NO_ERROR
+            || extract(&uid, &read, readend) != NO_ERROR
+            || extract(&timestamp, &read, readend) != NO_ERROR
+            || len > length
+            || header_len > len) {
+        free(key);
+        ALOGD("%s: invalid header", __func__);
+        return INVALID_OPERATION;
+    }
+    mKey = key;
+    free(key);
+    const size_t pos = read - bufferptr;
+    if (pos > header_len) {
+        ALOGD("%s: invalid header pos:%zu > header_len:%u",
+                __func__, pos, header_len);
+        return INVALID_OPERATION;
+    } else if (pos < header_len) {
+        ALOGD("%s: mismatched header pos:%zu < header_len:%u, advancing",
+                __func__, pos, header_len);
+        read += (header_len - pos);
+    }
+    if (extract(&propCount, &read, readend) != NO_ERROR) {
+        ALOGD("%s: cannot read prop count", __func__);
+        return INVALID_OPERATION;
+    }
+    mPid = pid;
+    mUid = uid;
+    mTimestamp = timestamp;
+    for (size_t i = 0; i < propCount; ++i) {
+        Prop *prop = allocateProp();
+        if (prop->readFromByteString(&read, readend) != NO_ERROR) {
+            ALOGD("%s: cannot read prop %zu", __func__, i);
+            return INVALID_OPERATION;
+        }
+    }
+    return NO_ERROR;
 }
 
-void MediaAnalyticsItem::Prop::writeToParcel(Parcel *data) const
+status_t MediaAnalyticsItem::Prop::writeToParcel(Parcel *data) const
 {
-   data->writeCString(mName);
-   data->writeInt32(mType);
    switch (mType) {
    case kTypeInt32:
-       data->writeInt32(u.int32Value);
-       break;
+       return data->writeCString(mName)
+               ?: data->writeInt32(mType)
+               ?: data->writeInt32(u.int32Value);
    case kTypeInt64:
-       data->writeInt64(u.int64Value);
-       break;
+       return data->writeCString(mName)
+               ?: data->writeInt32(mType)
+               ?: data->writeInt64(u.int64Value);
    case kTypeDouble:
-       data->writeDouble(u.doubleValue);
-       break;
+       return data->writeCString(mName)
+               ?: data->writeInt32(mType)
+               ?: data->writeDouble(u.doubleValue);
    case kTypeRate:
-       data->writeInt64(u.rate.count);
-       data->writeInt64(u.rate.duration);
-       break;
+       return data->writeCString(mName)
+               ?: data->writeInt32(mType)
+               ?: data->writeInt64(u.rate.first)
+               ?: data->writeInt64(u.rate.second);
    case kTypeCString:
-       data->writeCString(u.CStringValue);
-       break;
+       return data->writeCString(mName)
+               ?: data->writeInt32(mType)
+               ?: data->writeCString(u.CStringValue);
    default:
        ALOGE("%s: found bad type: %d, name %s", __func__, mType, mName);
-       break;
+       return BAD_VALUE;
    }
 }
 
-void MediaAnalyticsItem::Prop::toString(char *buffer, size_t length) const {
+status_t MediaAnalyticsItem::Prop::readFromParcel(const Parcel& data)
+{
+    const char *key = data.readCString();
+    if (key == nullptr) return BAD_VALUE;
+    int32_t type;
+    status_t status = data.readInt32(&type);
+    if (status != NO_ERROR) return status;
+    switch (type) {
+    case kTypeInt32:
+        status = data.readInt32(&u.int32Value);
+        break;
+    case kTypeInt64:
+        status = data.readInt64(&u.int64Value);
+        break;
+    case kTypeDouble:
+        status = data.readDouble(&u.doubleValue);
+        break;
+    case kTypeCString: {
+        const char *s = data.readCString();
+        if (s == nullptr) return BAD_VALUE;
+        set(s);
+        break;
+        }
+    case kTypeRate: {
+        std::pair<int64_t, int64_t> rate;
+        status = data.readInt64(&rate.first)
+                ?: data.readInt64(&rate.second);
+        if (status == NO_ERROR) {
+            set(rate);
+        }
+        break;
+        }
+    default:
+        ALOGE("%s: reading bad item type: %d", __func__, mType);
+        return BAD_VALUE;
+    }
+    if (status == NO_ERROR) {
+        setName(key);
+        mType = (Type)type;
+    }
+    return status;
+}
+
+void MediaAnalyticsItem::Prop::toString(char *buffer, size_t length) const
+{
     switch (mType) {
     case kTypeInt32:
         snprintf(buffer, length, "%s=%d:", mName, u.int32Value);
@@ -839,7 +866,7 @@ void MediaAnalyticsItem::Prop::toString(char *buffer, size_t length) const {
         break;
     case MediaAnalyticsItem::kTypeRate:
         snprintf(buffer, length, "%s=%lld/%lld:",
-                mName, (long long)u.rate.count, (long long)u.rate.duration);
+                mName, (long long)u.rate.first, (long long)u.rate.second);
         break;
     case MediaAnalyticsItem::kTypeCString:
         // TODO sanitize string for ':' '='
@@ -852,5 +879,168 @@ void MediaAnalyticsItem::Prop::toString(char *buffer, size_t length) const {
     }
 }
 
-} // namespace android
+size_t MediaAnalyticsItem::Prop::getByteStringSize() const
+{
+    const size_t header =
+        sizeof(uint16_t)      // length
+        + sizeof(uint8_t)     // type
+        + strlen(mName) + 1;  // mName + 0 termination
+    size_t payload = 0;
+    switch (mType) {
+    case MediaAnalyticsItem::kTypeInt32:
+        payload = sizeof(u.int32Value);
+        break;
+    case MediaAnalyticsItem::kTypeInt64:
+        payload = sizeof(u.int64Value);
+        break;
+    case MediaAnalyticsItem::kTypeDouble:
+        payload = sizeof(u.doubleValue);
+        break;
+    case MediaAnalyticsItem::kTypeRate:
+        payload = sizeof(u.rate.first) + sizeof(u.rate.second);
+        break;
+    case MediaAnalyticsItem::kTypeCString:
+        payload = strlen(u.CStringValue) + 1;
+        break;
+    default:
+        ALOGE("%s: found bad prop type: %d, name %s",
+                __func__, mType, mName); // no payload computed
+        break;
+    }
+    return header + payload;
+}
+
+// TODO: fold into a template later.
+status_t MediaAnalyticsItem::writeToByteString(
+        const char *name, int32_t value, char **bufferpptr, char *bufferptrmax)
+{
+    const size_t len = 2 + 1 + strlen(name) + 1 + sizeof(value);
+    if (len > UINT16_MAX) return BAD_VALUE;
+    return insert((uint16_t)len, bufferpptr, bufferptrmax)
+            ?: insert((uint8_t)kTypeInt32, bufferpptr, bufferptrmax)
+            ?: insert(name, bufferpptr, bufferptrmax)
+            ?: insert(value, bufferpptr, bufferptrmax);
+}
+
+status_t MediaAnalyticsItem::writeToByteString(
+        const char *name, int64_t value, char **bufferpptr, char *bufferptrmax)
+{
+    const size_t len = 2 + 1 + strlen(name) + 1 + sizeof(value);
+    if (len > UINT16_MAX) return BAD_VALUE;
+    return insert((uint16_t)len, bufferpptr, bufferptrmax)
+            ?: insert((uint8_t)kTypeInt64, bufferpptr, bufferptrmax)
+            ?: insert(name, bufferpptr, bufferptrmax)
+            ?: insert(value, bufferpptr, bufferptrmax);
+}
 
+status_t MediaAnalyticsItem::writeToByteString(
+        const char *name, double value, char **bufferpptr, char *bufferptrmax)
+{
+    const size_t len = 2 + 1 + strlen(name) + 1 + sizeof(value);
+    if (len > UINT16_MAX) return BAD_VALUE;
+    return insert((uint16_t)len, bufferpptr, bufferptrmax)
+            ?: insert((uint8_t)kTypeDouble, bufferpptr, bufferptrmax)
+            ?: insert(name, bufferpptr, bufferptrmax)
+            ?: insert(value, bufferpptr, bufferptrmax);
+}
+
+status_t MediaAnalyticsItem::writeToByteString(
+        const char *name, const std::pair<int64_t, int64_t> &value, char **bufferpptr, char *bufferptrmax)
+{
+    const size_t len = 2 + 1 + strlen(name) + 1 + 8 + 8;
+    if (len > UINT16_MAX) return BAD_VALUE;
+    return insert((uint16_t)len, bufferpptr, bufferptrmax)
+            ?: insert((uint8_t)kTypeRate, bufferpptr, bufferptrmax)
+            ?: insert(name, bufferpptr, bufferptrmax)
+            ?: insert(value.first, bufferpptr, bufferptrmax)
+            ?: insert(value.second, bufferpptr, bufferptrmax);
+}
+
+status_t MediaAnalyticsItem::writeToByteString(
+        const char *name, char * const &value, char **bufferpptr, char *bufferptrmax)
+{
+    const size_t len = 2 + 1 + strlen(name) + 1 + strlen(value) + 1;
+    if (len > UINT16_MAX) return BAD_VALUE;
+    return insert((uint16_t)len, bufferpptr, bufferptrmax)
+            ?: insert((uint8_t)kTypeCString, bufferpptr, bufferptrmax)
+            ?: insert(name, bufferpptr, bufferptrmax)
+            ?: insert(value, bufferpptr, bufferptrmax);
+}
+
+status_t MediaAnalyticsItem::writeToByteString(
+        const char *name, const none_t &, char **bufferpptr, char *bufferptrmax)
+{
+    const size_t len = 2 + 1 + strlen(name) + 1;
+    if (len > UINT16_MAX) return BAD_VALUE;
+    return insert((uint16_t)len, bufferpptr, bufferptrmax)
+            ?: insert((uint8_t)kTypeCString, bufferpptr, bufferptrmax)
+            ?: insert(name, bufferpptr, bufferptrmax);
+}
+
+status_t MediaAnalyticsItem::Prop::writeToByteString(
+        char **bufferpptr, char *bufferptrmax) const
+{
+    switch (mType) {
+    case kTypeInt32:
+        return MediaAnalyticsItem::writeToByteString(mName, u.int32Value, bufferpptr, bufferptrmax);
+    case kTypeInt64:
+        return MediaAnalyticsItem::writeToByteString(mName, u.int64Value, bufferpptr, bufferptrmax);
+    case kTypeDouble:
+        return MediaAnalyticsItem::writeToByteString(mName, u.doubleValue, bufferpptr, bufferptrmax);
+    case kTypeRate:
+        return MediaAnalyticsItem::writeToByteString(mName, u.rate, bufferpptr, bufferptrmax);
+    case kTypeCString:
+        return MediaAnalyticsItem::writeToByteString(mName, u.CStringValue, bufferpptr, bufferptrmax);
+    case kTypeNone:
+        return MediaAnalyticsItem::writeToByteString(mName, none_t{}, bufferpptr, bufferptrmax);
+    default:
+        ALOGE("%s: found bad prop type: %d, name %s",
+                __func__, mType, mName);  // no payload sent
+        return BAD_VALUE;
+    }
+}
+
+status_t MediaAnalyticsItem::Prop::readFromByteString(
+        const char **bufferpptr, const char *bufferptrmax)
+{
+    uint16_t len;
+    char *name;
+    uint8_t type;
+    status_t status = extract(&len, bufferpptr, bufferptrmax)
+            ?: extract(&type, bufferpptr, bufferptrmax)
+            ?: extract(&name, bufferpptr, bufferptrmax);
+    if (status != NO_ERROR) return status;
+    if (mName != nullptr) {
+        free(mName);
+    }
+    mName = name;
+    if (mType == kTypeCString) {
+        free(u.CStringValue);
+        u.CStringValue = nullptr;
+    }
+    mType = (Type)type;
+    switch (mType) {
+    case kTypeInt32:
+        return extract(&u.int32Value, bufferpptr, bufferptrmax);
+    case kTypeInt64:
+        return extract(&u.int64Value, bufferpptr, bufferptrmax);
+    case kTypeDouble:
+        return extract(&u.doubleValue, bufferpptr, bufferptrmax);
+    case kTypeRate:
+        return extract(&u.rate.first, bufferpptr, bufferptrmax)
+                ?: extract(&u.rate.second, bufferpptr, bufferptrmax);
+    case kTypeCString:
+        status = extract(&u.CStringValue, bufferpptr, bufferptrmax);
+        if (status != NO_ERROR) mType = kTypeNone;
+        return status;
+    case kTypeNone:
+        return NO_ERROR;
+    default:
+        mType = kTypeNone;
+        ALOGE("%s: found bad prop type: %d, name %s",
+                __func__, mType, mName);  // no payload sent
+        return BAD_VALUE;
+    }
+}
+
+} // namespace android

media/libmediametrics/MediaMetrics.cpp

@@ -195,6 +195,6 @@ bool mediametrics_isEnabled() {
 bool mediametrics_getAttributes(mediametrics_handle_t handle, char **buffer, size_t *length) {
     android::MediaAnalyticsItem *item = (android::MediaAnalyticsItem *) handle;
     if (item == NULL) return false;
-    return item->dumpAttributes(buffer, length);
+    return item->writeToByteString(buffer, length) == android::NO_ERROR;
 
 }

media/libmediametrics/include/MediaAnalyticsItem.h

@@ -19,6 +19,7 @@
 
 #include "MediaMetrics.h"
 
+#include <algorithm>
 #include <string>
 #include <sys/types.h>
 
@@ -34,8 +35,44 @@ namespace android {
 class IMediaAnalyticsService;
 class Parcel;
 
-// the class interface
-//
+/*
+ * Media Metrics
+ * Byte String format for communication of MediaAnalyticsItem.
+ *
+ * .... begin of item
+ * .... begin of header
+ * (uint32) length: including the length field itself
+ * (uint32) header length, including header_length and length fields.
+ * (uint16) version: 0
+ * (uint16) key length, including zero termination
+ * (int8)+ key string, including 0 termination
+ * (int32) pid
+ * (int32) uid
+ * (int64) timestamp
+ * .... end of header
+ * .... begin body
+ * (uint32) properties
+ * #properties of the following:
+ *     (uint16) property_length, including property_length field itself
+ *     (uint8) type of property
+ *     (int8)+ key string, including 0 termination
+ *      based on type of property (above), one of:
+ *       (int32)
+ *       (int64)
+ *       (double)
+ *       (int8)+ for cstring, including 0 termination
+ *       (int64, int64) for rate
+ * .... end body
+ * .... end of item
+ */
+
+/**
+ * Media Metrics MediaAnalyticsItem
+ *
+ * A mutable item representing an event or record that will be
+ * logged with the Media Metrics service.
+ *
+ */
 
 class MediaAnalyticsItem {
     friend class MediaMetricsJNI;           // TODO: remove this access
@@ -52,21 +89,9 @@ public:
                 kTypeRate = 5,
             };
 
-    // Key: the record descriminator
-    // values for the record discriminator
-    // values can be "component/component"
-    // basic values: "video", "audio", "drm"
-    // XXX: need to better define the format
-    using Key = std::string;
     static constexpr const char * const kKeyNone = "none";
     static constexpr const char * const kKeyAny = "any";
 
-        // Attr: names for attributes within a record
-        // format "prop1" or "prop/subprop"
-        // XXX: need to better define the format
-        typedef const char *Attr;
-
-
         enum {
             PROTO_V0 = 0,
             PROTO_FIRST = PROTO_V0,
@@ -78,11 +103,36 @@ public:
     template <typename T>
     explicit MediaAnalyticsItem(T key)
         : mKey(key) { }
+    MediaAnalyticsItem() = default;
+
     MediaAnalyticsItem(const MediaAnalyticsItem&) = delete;
     MediaAnalyticsItem &operator=(const MediaAnalyticsItem&) = delete;
 
-        static MediaAnalyticsItem* create(Key key);
-        static MediaAnalyticsItem* create();
+    bool operator==(const MediaAnalyticsItem& other) const {
+        if (mPropCount != other.mPropCount
+            || mPid != other.mPid
+            || mUid != other.mUid
+            || mPkgName != other.mPkgName
+            || mPkgVersionCode != other.mPkgVersionCode
+            || mKey != other.mKey
+            || mTimestamp != other.mTimestamp) return false;
+         for (size_t i = 0; i < mPropCount; ++i) {
+             Prop *p = other.findProp(mProps[i].getName());
+             if (p == nullptr || mProps[i] != *p) return false;
+         }
+         return true;
+    }
+    bool operator!=(const MediaAnalyticsItem& other) const {
+        return !(*this == other);
+    }
+
+    template <typename T>
+    static MediaAnalyticsItem* create(T key) {
+        return new MediaAnalyticsItem(key);
+    }
+    static MediaAnalyticsItem* create() {
+        return new MediaAnalyticsItem();
+    }
 
         static MediaAnalyticsItem* convert(mediametrics_handle_t);
         static mediametrics_handle_t convert(MediaAnalyticsItem *);
@@ -94,13 +144,14 @@ public:
         void clear();
         MediaAnalyticsItem *dup();
 
-        // set the key discriminator for the record.
-        // most often initialized as part of the constructor
-        MediaAnalyticsItem &setKey(MediaAnalyticsItem::Key);
-        const MediaAnalyticsItem::Key& getKey() const { return mKey; }
+    MediaAnalyticsItem &setKey(const char *key) {
+        mKey = key;
+        return *this;
+    }
+    const std::string& getKey() const { return mKey; }
 
-        // # of attributes in the record
-        int32_t count() const;
+    // # of properties in the record
+    size_t count() const { return mPropCount; }
 
     template<typename S, typename T>
     MediaAnalyticsItem &set(S key, T value) {
@@ -109,19 +160,19 @@ public:
     }
 
     // set values appropriately
-    MediaAnalyticsItem &setInt32(Attr key, int32_t value) {
+    MediaAnalyticsItem &setInt32(const char *key, int32_t value) {
         return set(key, value);
     }
-    MediaAnalyticsItem &setInt64(Attr key, int64_t value) {
+    MediaAnalyticsItem &setInt64(const char *key, int64_t value) {
         return set(key, value);
     }
-    MediaAnalyticsItem &setDouble(Attr key, double value) {
+    MediaAnalyticsItem &setDouble(const char *key, double value) {
         return set(key, value);
     }
-    MediaAnalyticsItem &setRate(Attr key, int64_t count, int64_t duration) {
+    MediaAnalyticsItem &setRate(const char *key, int64_t count, int64_t duration) {
         return set(key, std::make_pair(count, duration));
     }
-    MediaAnalyticsItem &setCString(Attr key, const char *value) {
+    MediaAnalyticsItem &setCString(const char *key, const char *value) {
         return set(key, value);
     }
 
@@ -133,16 +184,16 @@ public:
         return *this;
     }
 
-    MediaAnalyticsItem &addInt32(Attr key, int32_t value) {
+    MediaAnalyticsItem &addInt32(const char *key, int32_t value) {
         return add(key, value);
     }
-    MediaAnalyticsItem &addInt64(Attr key, int64_t value) {
+    MediaAnalyticsItem &addInt64(const char *key, int64_t value) {
         return add(key, value);
     }
-    MediaAnalyticsItem &addDouble(Attr key, double value) {
+    MediaAnalyticsItem &addDouble(const char *key, double value) {
         return add(key, value);
     }
-    MediaAnalyticsItem &addRate(Attr key, int64_t count, int64_t duration) {
+    MediaAnalyticsItem &addRate(const char *key, int64_t count, int64_t duration) {
         return add(key, std::make_pair(count, duration));
     }
 
@@ -155,16 +206,16 @@ public:
         return prop != nullptr && prop->get(value);
     }
 
-    bool getInt32(Attr key, int32_t *value) const {
+    bool getInt32(const char *key, int32_t *value) const {
         return get(key, value);
     }
-    bool getInt64(Attr key, int64_t *value) const {
+    bool getInt64(const char *key, int64_t *value) const {
         return get(key, value);
     }
-    bool getDouble(Attr key, double *value) const {
+    bool getDouble(const char *key, double *value) const {
         return get(key, value);
     }
-    bool getRate(Attr key, int64_t *count, int64_t *duration, double *rate) const {
+    bool getRate(const char *key, int64_t *count, int64_t *duration, double *rate) const {
         std::pair<int64_t, int64_t> value;
         if (!get(key, &value)) return false;
         if (count != nullptr) *count = value.first;
@@ -179,24 +230,29 @@ public:
         return true;
     }
     // Caller owns the returned string
-    bool getCString(Attr key, char **value) const {
-        return get(key, value);
+    bool getCString(const char *key, char **value) const {
+        const char *cs;
+        if (get(key, &cs)) {
+            *value = cs != nullptr ? strdup(cs) : nullptr;
+            return true;
+        }
+        return false;
     }
-    bool getString(Attr key, std::string *value) const {
+    bool getString(const char *key, std::string *value) const {
         return get(key, value);
     }
 
         // Deliver the item to MediaMetrics
         bool selfrecord();
 
-        // remove indicated attributes and their values
-        // filterNot() could also be called keepOnly()
-        // return value is # attributes removed
-        // XXX: perhaps 'remove' instead of 'filter'
-        // XXX: filterNot would become 'keep'
-        int32_t filter(int count, Attr attrs[]);
-        int32_t filterNot(int count, Attr attrs[]);
-        int32_t filter(Attr attr);
+    // remove indicated attributes and their values
+    // filterNot() could also be called keepOnly()
+    // return value is # attributes removed
+    // XXX: perhaps 'remove' instead of 'filter'
+    // XXX: filterNot would become 'keep'
+    size_t filter(size_t count, const char *attrs[]);
+    size_t filterNot(size_t count, const char *attrs[]);
+    size_t filter(const char *attr) { return filter(1, &attr); }
 
         // below here are used on server side or to talk to server
         // clients need not worry about these.
@@ -218,12 +274,26 @@ public:
         MediaAnalyticsItem &setPkgVersionCode(int64_t);
         int64_t getPkgVersionCode() const;
 
-        // our serialization code for binder calls
-        int32_t writeToParcel(Parcel *);
-        int32_t readFromParcel(const Parcel&);
-
-        // supports the stable interface
-        bool dumpAttributes(char **pbuffer, size_t *plength);
+    // our serialization code for binder calls
+    status_t writeToParcel(Parcel *) const;
+    status_t readFromParcel(const Parcel&);
+
+    status_t writeToByteString(char **bufferptr, size_t *length) const;
+    status_t readFromByteString(const char *bufferptr, size_t length);
+
+    static status_t writeToByteString(
+            const char *name, int32_t value, char **bufferpptr, char *bufferptrmax);
+    static status_t writeToByteString(
+            const char *name, int64_t value, char **bufferpptr, char *bufferptrmax);
+    static status_t writeToByteString(
+            const char *name, double value, char **bufferpptr, char *bufferptrmax);
+    static status_t writeToByteString(
+            const char *name, const std::pair<int64_t, int64_t> &value, char **bufferpptr, char *bufferptrmax);
+    static status_t writeToByteString(
+            const char *name, char * const &value, char **bufferpptr, char *bufferptrmax);
+    struct none_t {}; // for kTypeNone
+    static status_t writeToByteString(
+            const char *name, const none_t &, char **bufferpptr, char *bufferptrmax);
 
         std::string toString() const;
         std::string toString(int version) const;
@@ -233,11 +303,6 @@ public:
         // are we collecting analytics data
         static bool isEnabled();
 
-    private:
-        // handle Parcel version 0
-        int32_t writeToParcel0(Parcel *);
-        int32_t readFromParcel0(const Parcel&);
-
     protected:
 
         // merge fields from arg into this
@@ -246,18 +311,32 @@ public:
         // caller continues to own 'incoming'
         bool merge(MediaAnalyticsItem *incoming);
 
+private:
+    // handle Parcel version 0
+    int32_t writeToParcel0(Parcel *) const;
+    int32_t readFromParcel0(const Parcel&);
+
     // enabled 1, disabled 0
     static constexpr const char * const EnabledProperty = "media.metrics.enabled";
     static constexpr const char * const EnabledPropertyPersist = "persist.media.metrics.enabled";
     static const int EnabledProperty_default = 1;
 
-    private:
-
     // let's reuse a binder connection
     static sp<IMediaAnalyticsService> sAnalyticsService;
     static sp<IMediaAnalyticsService> getInstance();
     static void dropInstance();
 
+    // checks equality even with nullptr.
+    static bool stringEquals(const char *a, const char *b) {
+        if (a == nullptr) {
+            return b == nullptr;
+        } else {
+            return b != nullptr && strcmp(a, b) == 0;
+        }
+    }
+
+public:
+
     class Prop {
     friend class MediaMetricsJNI;           // TODO: remove this access
     public:
@@ -271,7 +350,6 @@ public:
             } else {
                 mName = nullptr;
             }
-            mNameLen = other.mNameLen;
             mType = other.mType;
             switch (mType) {
             case kTypeInt32:
@@ -287,7 +365,7 @@ public:
                 u.CStringValue = strdup(other.u.CStringValue);
                 break;
             case kTypeRate:
-                u.rate = {other.u.rate.count, other.u.rate.duration};
+                u.rate = other.u.rate;
                 break;
             case kTypeNone:
                 break;
@@ -297,11 +375,32 @@ public:
             }
             return *this;
         }
+        bool operator==(const Prop& other) const {
+            if (!stringEquals(mName, other.mName)
+                    || mType != other.mType) return false;
+            switch (mType) {
+            case kTypeInt32:
+                return u.int32Value == other.u.int32Value;
+            case kTypeInt64:
+                return u.int64Value == other.u.int64Value;
+            case kTypeDouble:
+                return u.doubleValue == other.u.doubleValue;
+            case kTypeCString:
+                return stringEquals(u.CStringValue, other.u.CStringValue);
+            case kTypeRate:
+                return u.rate == other.u.rate;
+            case kTypeNone:
+            default:
+                return true;
+            }
+        }
+        bool operator!=(const Prop& other) const {
+            return !(*this == other);
+        }
 
         void clear() {
             free(mName);
             mName = nullptr;
-            mNameLen = 0;
             clearValue();
         }
         void clearValue() {
@@ -322,29 +421,19 @@ public:
 
         void swap(Prop& other) {
             std::swap(mName, other.mName);
-            std::swap(mNameLen, other.mNameLen);
             std::swap(mType, other.mType);
             std::swap(u, other.u);
         }
 
-        void setName(const char *name, size_t len) {
+        void setName(const char *name) {
             free(mName);
             if (name != nullptr) {
-                mName = (char *)malloc(len + 1);
-                mNameLen = len;
-                strncpy(mName, name, len);
-                mName[len] = 0;
+                mName = strdup(name);
             } else {
                 mName = nullptr;
-                mNameLen = 0;
             }
         }
 
-        bool isNamed(const char *name, size_t len) const {
-            return len == mNameLen && memcmp(name, mName, len) == 0;
-        }
-
-        // TODO: remove duplicate but different definition
         bool isNamed(const char *name) const {
             return strcmp(name, mName) == 0;
         }
@@ -369,9 +458,9 @@ public:
            return true;
         }
         template <>
-        bool get(char** value) const {
+        bool get(const char** value) const {
             if (mType != kTypeCString) return false;
-            if (value != nullptr) *value = strdup(u.CStringValue);
+            if (value != nullptr) *value = u.CStringValue;
             return true;
         }
         template <>
@@ -384,8 +473,7 @@ public:
         bool get(std::pair<int64_t, int64_t> *value) const {
            if (mType != kTypeRate) return false;
            if (value != nullptr) {
-               value->first = u.rate.count;
-               value->second = u.rate.duration;
+               *value = u.rate;
            }
            return true;
         }
@@ -416,7 +504,13 @@ public:
             if (value == nullptr) {
                 u.CStringValue = nullptr;
             } else {
-                u.CStringValue = strdup(value);
+                size_t len = strlen(value);
+                if (len > UINT16_MAX - 1) {
+                    len = UINT16_MAX - 1;
+                }
+                u.CStringValue = (char *)malloc(len + 1);
+                strncpy(u.CStringValue, value, len);
+                u.CStringValue[len] = 0;
             }
         }
         template <>
@@ -456,33 +550,79 @@ public:
         template <>
         void add(const std::pair<int64_t, int64_t>& value) {
             if (mType == kTypeRate) {
-                u.rate.count += value.first;
-                u.rate.duration += value.second;
+                u.rate.first += value.first;
+                u.rate.second += value.second;
             } else {
                 mType = kTypeRate;
-                u.rate = {value.first, value.second};
+                u.rate = value;
             }
         }
 
-        void writeToParcel(Parcel *data) const;
+        status_t writeToParcel(Parcel *data) const;
+        status_t readFromParcel(const Parcel& data);
         void toString(char *buffer, size_t length) const;
+        size_t getByteStringSize() const;
+        status_t writeToByteString(char **bufferpptr, char *bufferptrmax) const;
+        status_t readFromByteString(const char **bufferpptr, const char *bufferptrmax);
 
-    // TODO: make private
+    // TODO: make private (and consider converting to std::variant)
     // private:
         char *mName = nullptr;
-        size_t mNameLen = 0;    // the strlen(), doesn't include the null
         Type mType = kTypeNone;
-        union {
+        union u__ {
+            u__() { zero(); }
+            u__(u__ &&other) {
+                *this = std::move(other);
+            }
+            u__& operator=(u__ &&other) {
+                memcpy(this, &other, sizeof(*this));
+                other.zero();
+                return *this;
+            }
+            void zero() { memset(this, 0, sizeof(*this)); }
+
             int32_t int32Value;
             int64_t int64Value;
             double doubleValue;
             char *CStringValue;
-            struct { int64_t count, duration; } rate;
+            std::pair<int64_t, int64_t> rate;
         } u;
     };
 
-    size_t findPropIndex(const char *name, size_t len) const;
+    class iterator {
+    public:
+       iterator(size_t pos, const MediaAnalyticsItem &_item)
+           : i(std::min(pos, _item.count()))
+           , item(_item) { }
+       iterator &operator++() {
+           i = std::min(i + 1, item.count());
+           return *this;
+       }
+       bool operator!=(iterator &other) const {
+           return i != other.i;
+       }
+       Prop &operator*() const {
+           return item.mProps[i];
+       }
+
+    private:
+      size_t i;
+      const MediaAnalyticsItem &item;
+    };
+
+    iterator begin() const {
+        return iterator(0, *this);
+    }
+    iterator end() const {
+        return iterator(SIZE_MAX, *this);
+    }
+
+private:
+
+    // TODO: make prop management class
+    size_t findPropIndex(const char *name) const;
     Prop *findProp(const char *name) const;
+    Prop *allocateProp();
 
         enum {
             kGrowProps = 10
@@ -490,6 +630,7 @@ public:
         bool growProps(int increment = kGrowProps);
         Prop *allocateProp(const char *name);
         bool removeProp(const char *name);
+    Prop *allocateProp(const std::string& name) { return allocateProp(name.c_str()); }
 
         size_t mPropCount = 0;
         size_t mPropSize = 0;
@@ -499,7 +640,7 @@ public:
     uid_t         mUid = -1;
     std::string   mPkgName;
     int64_t       mPkgVersionCode = 0;
-    Key           mKey{kKeyNone};
+    std::string   mKey{kKeyNone};
     nsecs_t       mTimestamp = 0;
 };
 

media/libmediaplayerservice/nuplayer/NuPlayerDriver.cpp

@@ -641,7 +641,7 @@ void NuPlayerDriver::logMetrics(const char *where) {
             mAnalyticsItem->setUid(mClientUid);
         }
     } else {
-        ALOGV("nothing to record (only %d fields)", mAnalyticsItem->count());
+        ALOGV("nothing to record (only %zu fields)", mAnalyticsItem->count());
     }
 }
 

services/mediaanalytics/tests/mediametrics_tests.cpp

@@ -160,6 +160,27 @@ TEST(mediametrics_tests, superbig_item_removal) {
   }
 }
 
+TEST(mediametrics_tests, superbig_item_removal2) {
+  MediaAnalyticsItem item("TheOne");
+  constexpr size_t count = 10000;
+
+  for (size_t i = 0; i < count; ++i) {
+    item.setInt32(std::to_string(i).c_str(), i);
+  }
+  static const char *attrs[] = { "1", };
+  item.filterNot(1, attrs);
+
+  for (size_t i = 0; i < count; ++i) {
+    int32_t i32;
+    if (i == 1) { // check to see that there is only one
+        ASSERT_TRUE(item.getInt32(std::to_string(i).c_str(), &i32));
+        ASSERT_EQ((int32_t)i, i32);
+    } else {
+        ASSERT_FALSE(item.getInt32(std::to_string(i).c_str(), &i32));
+    }
+  }
+}
+
 TEST(mediametrics_tests, item_transmutation) {
   MediaAnalyticsItem item("Alchemist's Stone");
 
@@ -175,3 +196,88 @@ TEST(mediametrics_tests, item_transmutation) {
   ASSERT_TRUE(item.getInt32("convert", &i32));   // check it is i32 and 2 (123 is discarded).
   ASSERT_EQ(2, i32);
 }
+
+TEST(mediametrics_tests, item_binderization) {
+  MediaAnalyticsItem item;
+  item.setInt32("i32", 1)
+      .setInt64("i64", 2)
+      .setDouble("double", 3.1)
+      .setCString("string", "abc")
+      .setRate("rate", 11, 12);
+
+  Parcel p;
+  item.writeToParcel(&p);
+
+  p.setDataPosition(0); // rewind for reading
+  MediaAnalyticsItem item2;
+  item2.readFromParcel(p);
+
+  ASSERT_EQ(item, item2);
+}
+
+TEST(mediametrics_tests, item_byteserialization) {
+  MediaAnalyticsItem item;
+  item.setInt32("i32", 1)
+      .setInt64("i64", 2)
+      .setDouble("double", 3.1)
+      .setCString("string", "abc")
+      .setRate("rate", 11, 12);
+
+  char *data;
+  size_t length;
+  ASSERT_EQ(0, item.writeToByteString(&data, &length));
+  ASSERT_GT(length, (size_t)0);
+
+  MediaAnalyticsItem item2;
+  item2.readFromByteString(data, length);
+
+  printf("item: %s\n", item.toString().c_str());
+  printf("item2: %s\n", item2.toString().c_str());
+  ASSERT_EQ(item, item2);
+
+  free(data);
+}
+
+TEST(mediametrics_tests, item_iteration) {
+  MediaAnalyticsItem item;
+  item.setInt32("i32", 1)
+      .setInt64("i64", 2)
+      .setDouble("double", 3.125)
+      .setCString("string", "abc")
+      .setRate("rate", 11, 12);
+
+  int mask = 0;
+  for (auto &prop : item) {
+      const char *name = prop.getName();
+      if (!strcmp(name, "i32")) {
+          int32_t i32;
+          ASSERT_TRUE(prop.get(&i32));
+          ASSERT_EQ(1, i32);
+          mask |= 1;
+      } else if (!strcmp(name, "i64")) {
+          int64_t i64;
+          ASSERT_TRUE(prop.get(&i64));
+          ASSERT_EQ(2, i64);
+          mask |= 2;
+      } else if (!strcmp(name, "double")) {
+          double d;
+          ASSERT_TRUE(prop.get(&d));
+          ASSERT_EQ(3.125, d);
+          mask |= 4;
+      } else if (!strcmp(name, "string")) {
+          const char *s;
+          ASSERT_TRUE(prop.get(&s));
+          ASSERT_EQ(0, strcmp(s, "abc"));
+          mask |= 8;
+      } else if (!strcmp(name, "rate")) {
+          std::pair<int64_t, int64_t> r;
+          ASSERT_TRUE(prop.get(&r));
+          ASSERT_EQ(11, r.first);
+          ASSERT_EQ(12, r.second);
+          mask |= 16;
+      } else {
+          FAIL();
+      }
+  }
+  ASSERT_EQ(31, mask);
+}