From 1e67f08963cc245660049b6a588483a148955e08 Mon Sep 17 00:00:00 2001 From: Doris Liu Date: Thu, 12 Nov 2015 15:57:45 -0800 Subject: [PATCH] Add error check for float parsing and fix tests Change-Id: Ia4f4863d415536b3796edbcdb83c951b6cff02cf --- core/jni/android_util_PathParser.cpp | 11 +++-- libs/hwui/PathParser.cpp | 74 ++++++++++++++++++++++---------- libs/hwui/PathParser.h | 14 ++++-- libs/hwui/VectorDrawablePath.cpp | 13 +++--- libs/hwui/microbench/PathParserBench.cpp | 3 +- libs/hwui/unit_tests/PathParserTests.cpp | 43 +++++++++++++++---- 6 files changed, 115 insertions(+), 43 deletions(-) diff --git a/core/jni/android_util_PathParser.cpp b/core/jni/android_util_PathParser.cpp index bf6ffc5487d2..245aa0f9020e 100644 --- a/core/jni/android_util_PathParser.cpp +++ b/core/jni/android_util_PathParser.cpp @@ -19,6 +19,7 @@ #include #include +#include #include "core_jni_helpers.h" namespace android { @@ -27,10 +28,14 @@ static bool parseStringForPath(JNIEnv* env, jobject, jlong skPathHandle, jstring jint strLength) { const char* pathString = env->GetStringUTFChars(inputPathStr, NULL); SkPath* skPath = reinterpret_cast(skPathHandle); - bool hasValidData = android::uirenderer::PathParser::parseStringForSkPath(skPath, pathString, - strLength); + + android::uirenderer::PathParser::ParseResult result; + android::uirenderer::PathParser::parseStringForSkPath(skPath, &result, pathString, strLength); env->ReleaseStringUTFChars(inputPathStr, pathString); - return hasValidData; + if (result.failureOccurred) { + ALOGE(result.failureMessage.c_str()); + } + return !result.failureOccurred; } static const JNINativeMethod gMethods[] = { diff --git a/libs/hwui/PathParser.cpp b/libs/hwui/PathParser.cpp index 61b9d2114a62..35230fff279e 100644 --- a/libs/hwui/PathParser.cpp +++ b/libs/hwui/PathParser.cpp @@ -18,6 +18,7 @@ #include "jni.h" +#include #include #include #include @@ -97,14 +98,31 @@ static void extract(int* outEndPosition, bool* outEndWithNegOrDot, const char* s *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* 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* outPoints, PathParser::ParseResult* result, + const char* pathStr, int start, int end) { if (pathStr[start] == 'z' || pathStr[start] == 'Z') { return; @@ -120,7 +138,12 @@ static void getFloats(std::vector* outPoints, const char* pathStr, int st 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 @@ static void getFloats(std::vector* outPoints, const char* pathStr, int st 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 @@ void PathParser::getPathDataFromString(PathData* data, const char* pathStr, size while (end < strLen) { end = nextStart(pathStr, strLen, end); std::vector 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 @@ void PathParser::getPathDataFromString(PathData* data, const char* pathStr, size 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 @@ void PathParser::dump(const PathData& data) { 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 @@ void PathParser::dump(const PathData& data) { 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 d30bb0f1b973..a9c1e60dae2e 100644 --- a/libs/hwui/PathParser.h +++ b/libs/hwui/PathParser.h @@ -23,17 +23,25 @@ #include #include +#include + 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 115435c14641..05ea2da88fdb 100644 --- a/libs/hwui/VectorDrawablePath.cpp +++ b/libs/hwui/VectorDrawablePath.cpp @@ -37,8 +37,11 @@ public: }; 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 @@ void VectorDrawablePath::verbsToPath(SkPath* outPath, const PathData* data) { 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 @@ static void drawArc(SkPath* p, 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* points, size_t start, size_t end) { @@ -305,7 +308,7 @@ void PathResolver::addCommand(SkPath* outPath, char previousCmd, 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 198035edc292..171078db9ef6 100644 --- a/libs/hwui/microbench/PathParserBench.cpp +++ b/libs/hwui/microbench/PathParserBench.cpp @@ -28,9 +28,10 @@ void BM_PathParser_parseStringPath::Run(int iter) { 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 60ea219ac916..c99d7b0a0368 100644 --- a/libs/hwui/unit_tests/PathParserTests.cpp +++ b/libs/hwui/unit_tests/PathParserTests.cpp @@ -164,16 +164,37 @@ const static TestData sTestDataSet[] = { }; +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, createSkPathFromPathData) { 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 -- 2.11.0