commit 73154babe597e8b62739bed93ee7ac642e4173d6
author Florin Malita <fmalita@chromium.org> Thu Aug 09 17:29:14 2018 -0400
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Thu Aug 09 22:05:32 2018 +0000
tree b0e01c4d5404738427aa7ec4ea30901490b56e9b
parent 94c4f41ab8d5ad7226ee796a1ecbb95d6874a75f

[skottie] Avoid UB while parsing 1-based indices

Current impl can underflow int due to unchecked arithmetic.

Add a Parse<size_t> specialization and convert call sites which use
inline arithmetic.  Underflowing unsigned types should be well defined
and caught in later tests.

Bug: oss-fuzz:9798
Change-Id: Iaebe8aad4009e2511fe1d8733d336f5f119bb384
Reviewed-on: https://skia-review.googlesource.com/146648
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Florin Malita <fmalita@chromium.org>

modules/skottie/src/Skottie.cpp

diff --git a/modules/skottie/src/Skottie.cpp b/modules/skottie/src/Skottie.cpp
index 0331cbb..b1bd2c9 100644
--- a/modules/skottie/src/Skottie.cpp
+++ b/modules/skottie/src/Skottie.cpp
@@ -218,8 +218,8 @@
         PolyStarAdapter::Type::kPoly, // "sy": 2
     };
 
-    const auto type = ParseDefault<int>(jstar["sy"], 0) - 1;
-    if (type < 0 || type >= SkTo<int>(SK_ARRAY_COUNT(gTypes))) {
+    const auto type = ParseDefault<size_t>(jstar["sy"], 0) - 1;
+    if (type >= SK_ARRAY_COUNT(gTypes)) {
         LogFail(jstar, "Unknown polystar type");
         return nullptr;
     }
@@ -344,16 +344,16 @@
         SkPaint::kRound_Join,
         SkPaint::kBevel_Join,
     };
-    stroke_node->setStrokeJoin(gJoins[SkTPin<int>(ParseDefault<int>(jstroke["lj"], 1) - 1,
-                                                  0, SK_ARRAY_COUNT(gJoins) - 1)]);
+    stroke_node->setStrokeJoin(gJoins[SkTMin<size_t>(ParseDefault<size_t>(jstroke["lj"], 1) - 1,
+                                                     SK_ARRAY_COUNT(gJoins) - 1)]);
 
     static constexpr SkPaint::Cap gCaps[] = {
         SkPaint::kButt_Cap,
         SkPaint::kRound_Cap,
         SkPaint::kSquare_Cap,
     };
-    stroke_node->setStrokeCap(gCaps[SkTPin<int>(ParseDefault(jstroke["lc"], 1) - 1,
-                                                0, SK_ARRAY_COUNT(gCaps) - 1)]);
+    stroke_node->setStrokeCap(gCaps[SkTMin<size_t>(ParseDefault<size_t>(jstroke["lc"], 1) - 1,
+                                                   SK_ARRAY_COUNT(gCaps) - 1)]);
 
     return stroke_node;
 }
@@ -398,8 +398,8 @@
         sksg::Merge::Mode::kXOR      ,  // "mm": 5
     };
 
-    const auto mode = gModes[SkTPin<int>(ParseDefault<int>(jmerge["mm"], 1) - 1,
-                                         0, SK_ARRAY_COUNT(gModes) - 1)];
+    const auto mode = gModes[SkTMin<size_t>(ParseDefault<size_t>(jmerge["mm"], 1) - 1,
+                                            SK_ARRAY_COUNT(gModes) - 1)];
 
     std::vector<sk_sp<sksg::GeometryNode>> merged;
     merged.push_back(Merge(std::move(geos), mode));
@@ -416,8 +416,8 @@
         kSeparate, // "m": 2
     } gModes[] = { Mode::kMerged, Mode::kSeparate };
 
-    const auto mode = gModes[SkTPin<int>(ParseDefault<int>(jtrim["m"], 1) - 1,
-                                         0, SK_ARRAY_COUNT(gModes) - 1)];
+    const auto mode = gModes[SkTMin<size_t>(ParseDefault<size_t>(jtrim["m"], 1) - 1,
+                                            SK_ARRAY_COUNT(gModes) - 1)];
 
     std::vector<sk_sp<sksg::GeometryNode>> inputs;
     if (mode == Mode::kMerged) {
@@ -1258,9 +1258,9 @@
             sksg::MaskEffect::Mode::kNormal, // tt: 1
             sksg::MaskEffect::Mode::kInvert, // tt: 2
         };
-        const auto matteType = ParseDefault<int>((*jlayer)["tt"], 1) - 1;
+        const auto matteType = ParseDefault<size_t>((*jlayer)["tt"], 1) - 1;
 
-        if (matteType >= 0 && matteType < SkTo<int>(SK_ARRAY_COUNT(gMaskModes))) {
+        if (matteType < SK_ARRAY_COUNT(gMaskModes)) {
             return sksg::MaskEffect::Make(std::move(controller_node),
                                           std::move(layerCtx->fCurrentMatte),
                                           gMaskModes[matteType]);

modules/skottie/src/SkottieJson.cpp

diff --git a/modules/skottie/src/SkottieJson.cpp b/modules/skottie/src/SkottieJson.cpp
index 23221af..568f4df 100644
--- a/modules/skottie/src/SkottieJson.cpp
+++ b/modules/skottie/src/SkottieJson.cpp
@@ -53,18 +53,28 @@
     return false;
 }
 
-template <>
-bool Parse<int>(const Value& v, int* i) {
+template <typename T>
+bool ParseIntegral(const Value& v, T* result) {
     if (const skjson::NumberValue* num = v) {
         const auto dbl = **num;
-        *i = dbl;
-        return *i == dbl;
+        *result = static_cast<T>(dbl);
+        return static_cast<double>(*result) == dbl;
     }
 
     return false;
 }
 
 template <>
+bool Parse<int>(const Value& v, int* i) {
+    return ParseIntegral(v, i);
+}
+
+template <>
+bool Parse<size_t>(const Value& v, size_t* sz) {
+    return ParseIntegral(v, sz);
+}
+
+template <>
 bool Parse<SkString>(const Value& v, SkString* s) {
     if (const skjson::StringValue* sv = v) {
         s->set(sv->begin(), sv->size());