Fix bug with parsing vectors
Also move to StringUtils, add unit test and use std::from_chars.
Bug: 234833109
Test: statsd_testdrive 684 while watching YouTube videos
Test: atest mediametrics_tests#parseVector
Change-Id: I0eff0bcb7fb9542fdab0c08b98f7789decedb7d2
diff --git a/services/mediametrics/StringUtils.cpp b/services/mediametrics/StringUtils.cpp
index d1c7a18..5766f1c 100644
--- a/services/mediametrics/StringUtils.cpp
+++ b/services/mediametrics/StringUtils.cpp
@@ -15,11 +15,13 @@
*/
//#define LOG_NDEBUG 0
-#define LOG_TAG "MediaMetricsService::stringutils"
+#define LOG_TAG "mediametrics::stringutils"
#include <utils/Log.h>
#include "StringUtils.h"
+#include <charconv>
+
#include "AudioTypes.h"
namespace android::mediametrics::stringutils {
@@ -54,6 +56,26 @@
}
}
+bool parseVector(const std::string &str, std::vector<int32_t> *vector) {
+ std::vector<int32_t> values;
+ const char *p = str.c_str();
+ const char *last = p + str.size();
+ while (p != last) {
+ if (*p == ',' || *p == '{' || *p == '}') {
+ p++;
+ }
+ int32_t value = -1;
+ auto [ptr, error] = std::from_chars(p, last, value);
+ if (error == std::errc::invalid_argument || error == std::errc::result_out_of_range) {
+ return false;
+ }
+ p = ptr;
+ values.push_back(value);
+ }
+ *vector = std::move(values);
+ return true;
+}
+
std::vector<std::pair<std::string, std::string>> getDeviceAddressPairs(const std::string& devices)
{
std::vector<std::pair<std::string, std::string>> result;
diff --git a/services/mediametrics/include/mediametricsservice/StatsdLog.h b/services/mediametrics/include/mediametricsservice/StatsdLog.h
index e207bac..5d5009e 100644
--- a/services/mediametrics/include/mediametricsservice/StatsdLog.h
+++ b/services/mediametrics/include/mediametricsservice/StatsdLog.h
@@ -16,11 +16,13 @@
#pragma once
-#include <audio_utils/SimpleLog.h>
#include <map>
#include <mutex>
#include <sstream>
+#include <android-base/thread_annotations.h>
+#include <audio_utils/SimpleLog.h>
+
namespace android::mediametrics {
class StatsdLog {
@@ -61,9 +63,9 @@
}
private:
+ mutable std::mutex mLock;
SimpleLog mSimpleLog; // internally locked
std::map<int /* atom */, size_t /* count */> mCountMap GUARDED_BY(mLock); // sorted
- mutable std::mutex mLock;
};
} // namespace android::mediametrics
diff --git a/services/mediametrics/include/mediametricsservice/StringUtils.h b/services/mediametrics/include/mediametricsservice/StringUtils.h
index 78c25ff..ed2cf2e 100644
--- a/services/mediametrics/include/mediametricsservice/StringUtils.h
+++ b/services/mediametrics/include/mediametricsservice/StringUtils.h
@@ -72,6 +72,12 @@
std::vector<std::string> split(const std::string& flags, const char *delim);
/**
+ * Parses a vector of integers using ',' '{' and '}' as delimeters. Leaves
+ * vector unmodified if the parsing fails.
+ */
+bool parseVector(const std::string &str, std::vector<int32_t> *vector);
+
+/**
* Parse the devices string and return a vector of device address pairs.
*
* A failure to parse returns early with the contents that were able to be parsed.
diff --git a/services/mediametrics/include/mediametricsservice/iface_statsd.h b/services/mediametrics/include/mediametricsservice/iface_statsd.h
index 5bc293b..34d71ba 100644
--- a/services/mediametrics/include/mediametricsservice/iface_statsd.h
+++ b/services/mediametrics/include/mediametricsservice/iface_statsd.h
@@ -15,7 +15,9 @@
*/
#include <memory>
+
#include <stats_event.h>
+#include <StatsdLog.h>
namespace android {
namespace mediametrics {
diff --git a/services/mediametrics/statsd_codec.cpp b/services/mediametrics/statsd_codec.cpp
index a0b8f16..ad4cfce 100644
--- a/services/mediametrics/statsd_codec.cpp
+++ b/services/mediametrics/statsd_codec.cpp
@@ -23,6 +23,7 @@
#include <pthread.h>
#include <pwd.h>
#include <stdint.h>
+#include <string>
#include <string.h>
#include <sys/stat.h>
#include <sys/time.h>
@@ -32,11 +33,12 @@
#include <stats_media_metrics.h>
#include <stats_event.h>
-#include "cleaner.h"
-#include "MediaMetricsService.h"
-#include "ValidateId.h"
-#include "frameworks/proto_logging/stats/message/mediametrics_message.pb.h"
-#include "iface_statsd.h"
+#include <frameworks/proto_logging/stats/message/mediametrics_message.pb.h>
+#include <mediametricsservice/cleaner.h>
+#include <mediametricsservice/iface_statsd.h>
+#include <mediametricsservice/MediaMetricsService.h>
+#include <mediametricsservice/StringUtils.h>
+#include <mediametricsservice/ValidateId.h>
namespace android {
@@ -169,27 +171,8 @@
}
static void parseVector(const std::string &str, std::vector<int32_t> *vector) {
- char valueStr[12] = {};
- int i = 0;
- for (char const * p = str.c_str(); *p != 0; ++p) {
- if (*p == ',' || *p == '{' || *p == '}') {
- valueStr[i] = 0;
- int64_t value = strtol(valueStr, nullptr, 10);
- if (value >= std::numeric_limits<int32_t>::max() ||
- value <= std::numeric_limits<int32_t>::min() ||
- value == 0) {
- ALOGE("failed to parse integer vector at '%s' from '%s'", p, str.c_str());
- return;
- }
- vector->push_back(int32_t(value));
- i = valueStr[0] = 0;
- } else {
- valueStr[i++] = *p;
- if (i == sizeof(valueStr) - 1) { // -1 because we need space for a null terminator
- ALOGE("failed to parse integer vector at '%s' from '%s'", p, str.c_str());
- return;
- }
- }
+ if (!mediametrics::stringutils::parseVector(str, vector)) {
+ ALOGE("failed to parse integer vector from '%s'", str.c_str());
}
}
diff --git a/services/mediametrics/tests/mediametrics_tests.cpp b/services/mediametrics/tests/mediametrics_tests.cpp
index bc7b47b..4a6aee4 100644
--- a/services/mediametrics/tests/mediametrics_tests.cpp
+++ b/services/mediametrics/tests/mediametrics_tests.cpp
@@ -17,9 +17,10 @@
#define LOG_TAG "mediametrics_tests"
#include <utils/Log.h>
-
#include <stdio.h>
+#include <string>
#include <unordered_set>
+#include <vector>
#include <gtest/gtest.h>
#include <media/MediaMetricsItem.h>
@@ -30,6 +31,7 @@
#include <system/audio.h>
using namespace android;
+using android::mediametrics::stringutils::parseVector;
static size_t countNewlines(const char *s) {
size_t count = 0;
@@ -57,6 +59,35 @@
ASSERT_EQ(false, android::mediametrics::startsWith(s, std::string("est")));
}
+TEST(mediametrics_tests, parseVector) {
+ {
+ std::vector<int32_t> values;
+ EXPECT_EQ(true, parseVector("0{4,300,0,-112343,350}9", &values));
+ EXPECT_EQ(values, std::vector<int32_t>({0, 4, 300, 0, -112343, 350, 9}));
+ }
+ {
+ std::vector<int32_t> values;
+ EXPECT_EQ(true, parseVector("53", &values));
+ EXPECT_EQ(values, std::vector<int32_t>({53}));
+ }
+ {
+ std::vector<int32_t> values;
+ EXPECT_EQ(false, parseVector("5{3,6*3}3", &values));
+ EXPECT_EQ(values, std::vector<int32_t>({}));
+ }
+ {
+ std::vector<int32_t> values = {1}; // should still be this when parsing fails
+ std::vector<int32_t> expected = {1};
+ EXPECT_EQ(false, parseVector("51342abcd,1232", &values));
+ EXPECT_EQ(values, std::vector<int32_t>({1}));
+ }
+ {
+ std::vector<int32_t> values = {2}; // should still be this when parsing fails
+ EXPECT_EQ(false, parseVector("12345678901234,12345678901234", &values));
+ EXPECT_EQ(values, std::vector<int32_t>({2}));
+ }
+}
+
TEST(mediametrics_tests, defer) {
bool check = false;
{