Add error check for float parsing and fix tests
Change-Id: Ia4f4863d415536b3796edbcdb83c951b6cff02cf
diff --git a/libs/hwui/PathParser.cpp b/libs/hwui/PathParser.cpp
index 61b9d21..35230ff 100644
--- a/libs/hwui/PathParser.cpp
+++ b/libs/hwui/PathParser.cpp
@@ -18,6 +18,7 @@
#include "jni.h"
+#include <errno.h>
#include <utils/Log.h>
#include <sstream>
#include <stdlib.h>
@@ -97,14 +98,31 @@
*outEndPosition = currentIndex;
}
+static float parseFloat(PathParser::ParseResult* result, const char* startPtr, size_t expectedLength) {
+ char* endPtr = NULL;
+ float currentValue = strtof(startPtr, &endPtr);
+ if ((currentValue == HUGE_VALF || currentValue == -HUGE_VALF) && errno == ERANGE) {
+ result->failureOccurred = true;
+ result->failureMessage = "Float out of range: ";
+ result->failureMessage.append(startPtr, expectedLength);
+ }
+ if (currentValue == 0 && endPtr == startPtr) {
+ // No conversion is done.
+ result->failureOccurred = true;
+ result->failureMessage = "Float format error when parsing: ";
+ result->failureMessage.append(startPtr, expectedLength);
+ }
+ return currentValue;
+}
+
/**
-* Parse the floats in the string.
-* This is an optimized version of parseFloat(s.split(",|\\s"));
-*
-* @param s the string containing a command and list of floats
-* @return array of floats
-*/
-static void getFloats(std::vector<float>* outPoints, const char* pathStr, int start, int end) {
+ * Parse the floats in the string.
+ *
+ * @param s the string containing a command and list of floats
+ * @return true on success
+ */
+static void getFloats(std::vector<float>* outPoints, PathParser::ParseResult* result,
+ const char* pathStr, int start, int end) {
if (pathStr[start] == 'z' || pathStr[start] == 'Z') {
return;
@@ -120,7 +138,12 @@
extract(&endPosition, &endWithNegOrDot, pathStr, startPosition, end);
if (startPosition < endPosition) {
- outPoints->push_back(strtof(&pathStr[startPosition], NULL));
+ float currentValue = parseFloat(result, &pathStr[startPosition],
+ end - startPosition);
+ if (result->failureOccurred) {
+ return;
+ }
+ outPoints->push_back(currentValue);
}
if (endWithNegOrDot) {
@@ -130,10 +153,14 @@
startPosition = endPosition + 1;
}
}
+ return;
}
-void PathParser::getPathDataFromString(PathData* data, const char* pathStr, size_t strLen) {
+void PathParser::getPathDataFromString(PathData* data, ParseResult* result,
+ const char* pathStr, size_t strLen) {
if (pathStr == NULL) {
+ result->failureOccurred = true;
+ result->failureMessage = "Path string cannot be NULL.";
return;
}
@@ -143,7 +170,10 @@
while (end < strLen) {
end = nextStart(pathStr, strLen, end);
std::vector<float> points;
- getFloats(&points, pathStr, start, end);
+ getFloats(&points, result, pathStr, start, end);
+ if (result->failureOccurred) {
+ return;
+ }
data->verbs.push_back(pathStr[start]);
data->verbSizes.push_back(points.size());
data->points.insert(data->points.end(), points.begin(), points.end());
@@ -151,16 +181,11 @@
end++;
}
- if ((end - start) == 1 && pathStr[start] != '\0') {
+ if ((end - start) == 1 && start < strLen) {
data->verbs.push_back(pathStr[start]);
data->verbSizes.push_back(0);
}
-
- int i = 0;
- while(pathStr[i] != '\0') {
- i++;
- }
-
+ return;
}
void PathParser::dump(const PathData& data) {
@@ -169,6 +194,7 @@
for (size_t i = 0; i < data.verbs.size(); i++) {
std::ostringstream os;
os << data.verbs[i];
+ os << ", verb size: " << data.verbSizes[i];
for (size_t j = 0; j < data.verbSizes[i]; j++) {
os << " " << data.points[start + j];
}
@@ -183,16 +209,20 @@
ALOGD("points are : %s", os.str().c_str());
}
-bool PathParser::parseStringForSkPath(SkPath* skPath, const char* pathStr, size_t strLen) {
+void PathParser::parseStringForSkPath(SkPath* skPath, ParseResult* result, const char* pathStr, size_t strLen) {
PathData pathData;
- getPathDataFromString(&pathData, pathStr, strLen);
-
+ getPathDataFromString(&pathData, result, pathStr, strLen);
+ if (result->failureOccurred) {
+ return;
+ }
// Check if there is valid data coming out of parsing the string.
if (pathData.verbs.size() == 0) {
- return false;
+ result->failureOccurred = true;
+ result->failureMessage = "No verbs found in the string for pathData";
+ return;
}
VectorDrawablePath::verbsToPath(skPath, &pathData);
- return true;
+ return;
}
}; // namespace uirenderer
diff --git a/libs/hwui/PathParser.h b/libs/hwui/PathParser.h
index d30bb0f..a9c1e60 100644
--- a/libs/hwui/PathParser.h
+++ b/libs/hwui/PathParser.h
@@ -23,17 +23,25 @@
#include <android/log.h>
#include <cutils/compiler.h>
+#include <string>
+
namespace android {
namespace uirenderer {
+
class PathParser {
public:
+ struct ANDROID_API ParseResult {
+ bool failureOccurred = false;
+ std::string failureMessage;
+ };
/**
* Parse the string literal and create a Skia Path. Return true on success.
*/
- ANDROID_API static bool parseStringForSkPath(SkPath* outPath, const char* pathStr,
- size_t strLength);
- static void getPathDataFromString(PathData* outData, const char* pathStr, size_t strLength);
+ ANDROID_API static void parseStringForSkPath(SkPath* outPath, ParseResult* result,
+ const char* pathStr, size_t strLength);
+ static void getPathDataFromString(PathData* outData, ParseResult* result,
+ const char* pathStr, size_t strLength);
static void dump(const PathData& data);
};
diff --git a/libs/hwui/VectorDrawablePath.cpp b/libs/hwui/VectorDrawablePath.cpp
index 115435c..05ea2da 100644
--- a/libs/hwui/VectorDrawablePath.cpp
+++ b/libs/hwui/VectorDrawablePath.cpp
@@ -37,8 +37,11 @@
};
VectorDrawablePath::VectorDrawablePath(const char* pathStr, size_t strLength) {
- PathParser::getPathDataFromString(&mData, pathStr, strLength);
- verbsToPath(&mSkPath, &mData);
+ PathParser::ParseResult result;
+ PathParser::getPathDataFromString(&mData, &result, pathStr, strLength);
+ if (!result.failureOccurred) {
+ verbsToPath(&mSkPath, &mData);
+ }
}
VectorDrawablePath::VectorDrawablePath(const PathData& data) {
@@ -80,7 +83,7 @@
for (unsigned int i = 0; i < data->verbs.size(); i++) {
size_t verbSize = data->verbSizes[i];
resolver.addCommand(outPath, previousCommand, data->verbs[i], &data->points, start,
- start + verbSize - 1u);
+ start + verbSize);
previousCommand = data->verbs[i];
start += verbSize;
}
@@ -254,7 +257,7 @@
arcToBezier(p, cx, cy, a, b, x0, y0, thetaD, eta0, sweep);
}
-
+// Use the given verb, and points in the range [start, end) to insert a command into the SkPath.
void PathResolver::addCommand(SkPath* outPath, char previousCmd,
char cmd, const std::vector<float>* points, size_t start, size_t end) {
@@ -305,7 +308,7 @@
break;
}
- for (unsigned int k = start; k <= end; k += incr) {
+ for (unsigned int k = start; k < end; k += incr) {
switch (cmd) {
case 'm': // moveto - Start a new sub-path (relative)
currentX += points->at(k + 0);
diff --git a/libs/hwui/microbench/PathParserBench.cpp b/libs/hwui/microbench/PathParserBench.cpp
index 198035e..171078d 100644
--- a/libs/hwui/microbench/PathParserBench.cpp
+++ b/libs/hwui/microbench/PathParserBench.cpp
@@ -28,9 +28,10 @@
const char* pathString = "M 1 1 m 2 2, l 3 3 L 3 3 H 4 h4 V5 v5, Q6 6 6 6 q 6 6 6 6t 7 7 T 7 7 C 8 8 8 8 8 8 c 8 8 8 8 8 8 S 9 9 9 9 s 9 9 9 9 A 10 10 0 1 1 10 10 a 10 10 0 1 1 10 10";
SkPath skPath;
size_t length = strlen(pathString);
+ PathParser::ParseResult result;
StartBenchmarkTiming();
for (int i = 0; i < iter; i++) {
- PathParser::parseStringForSkPath(&skPath, pathString, length);
+ PathParser::parseStringForSkPath(&skPath, &result, pathString, length);
}
StopBenchmarkTiming();
}
diff --git a/libs/hwui/unit_tests/PathParserTests.cpp b/libs/hwui/unit_tests/PathParserTests.cpp
index 60ea219..c99d7b0 100644
--- a/libs/hwui/unit_tests/PathParserTests.cpp
+++ b/libs/hwui/unit_tests/PathParserTests.cpp
@@ -164,16 +164,37 @@
};
+struct StringPath {
+ const char* stringPath;
+ bool isValid;
+};
+
+const StringPath sStringPaths[] = {
+ {"3e...3", false},
+ {"L.M.F.A.O", false},
+ {"m 1 1", true},
+ {"z", true},
+ {"1-2e34567", false}
+};
+
TEST(PathParser, parseStringForData) {
for (TestData testData: sTestDataSet) {
+ PathParser::ParseResult result;
// Test generated path data against the given data.
PathData pathData;
size_t length = strlen(testData.pathString);
- PathParser::getPathDataFromString(&pathData, testData.pathString, length);
- PathParser::dump(pathData);
+ PathParser::getPathDataFromString(&pathData, &result, testData.pathString, length);
EXPECT_EQ(testData.pathData, pathData);
}
+ for (StringPath stringPath : sStringPaths) {
+ PathParser::ParseResult result;
+ PathData pathData;
+ SkPath skPath;
+ PathParser::getPathDataFromString(&pathData, &result,
+ stringPath.stringPath, strlen(stringPath.stringPath));
+ EXPECT_EQ(stringPath.isValid, !result.failureOccurred);
+ }
}
TEST(PathParser, createSkPathFromPathData) {
@@ -188,21 +209,25 @@
TEST(PathParser, parseStringForSkPath) {
for (TestData testData: sTestDataSet) {
+ PathParser::ParseResult result;
size_t length = strlen(testData.pathString);
// Check the return value as well as the SkPath generated.
SkPath actualPath;
- bool hasValidData = PathParser::parseStringForSkPath(&actualPath, testData.pathString,
- length);
+ PathParser::parseStringForSkPath(&actualPath, &result, testData.pathString, length);
+ bool hasValidData = !result.failureOccurred;
EXPECT_EQ(hasValidData, testData.pathData.verbs.size() > 0);
SkPath expectedPath;
testData.skPathLamda(&expectedPath);
EXPECT_EQ(expectedPath, actualPath);
}
- SkPath path;
- EXPECT_FALSE(PathParser::parseStringForSkPath(&path, "l", 1));
- EXPECT_FALSE(PathParser::parseStringForSkPath(&path, "1 1", 3));
- EXPECT_FALSE(PathParser::parseStringForSkPath(&path, "LMFAO", 5));
- EXPECT_TRUE(PathParser::parseStringForSkPath(&path, "m1 1", 4));
+
+ for (StringPath stringPath : sStringPaths) {
+ PathParser::ParseResult result;
+ SkPath skPath;
+ PathParser::parseStringForSkPath(&skPath, &result, stringPath.stringPath,
+ strlen(stringPath.stringPath));
+ EXPECT_EQ(stringPath.isValid, !result.failureOccurred);
+ }
}
}; // namespace uirenderer