From 33f38b05fb54994a39ff77c1b8681276c6d03ea3 Mon Sep 17 00:00:00 2001
From: Mike Reed <reed@google.com>
Date: Wed, 14 Feb 2018 13:58:06 -0500
Subject: simplify size check in string

b/72956754

Bug: skia:
Change-Id: I50627d9c7fe84630c496f8829608cde875512da0
Reviewed-on: https://skia-review.googlesource.com/107304
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Herb Derby <herb@google.com>
---
 src/core/SkSafeMath.h |  7 +++++++
 src/core/SkString.cpp | 19 ++++++++++++-------
 tests/StringTest.cpp  | 21 +++++++++++++++++++++
 3 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/src/core/SkSafeMath.h b/src/core/SkSafeMath.h
index 98bd07b2cb..949e9c4554 100644
--- a/src/core/SkSafeMath.h
+++ b/src/core/SkSafeMath.h
@@ -48,6 +48,13 @@ public:
         return add(x, alignment - 1) & ~(alignment - 1);
     }
 
+    template <typename T> T castTo(size_t value) {
+        if (!SkTFitsIn<T>(value)) {
+            fOK = false;
+        }
+        return static_cast<T>(value);
+    }
+
     // These saturate to their results
     static size_t Add(size_t x, size_t y);
     static size_t Mul(size_t x, size_t y);
diff --git a/src/core/SkString.cpp b/src/core/SkString.cpp
index 59c57c9936..0978904a11 100644
--- a/src/core/SkString.cpp
+++ b/src/core/SkString.cpp
@@ -5,8 +5,8 @@
  * found in the LICENSE file.
  */
 
-
 #include "SkAtomics.h"
+#include "SkSafeMath.h"
 #include "SkString.h"
 #include "SkUtils.h"
 #include <stdarg.h>
@@ -224,13 +224,18 @@ sk_sp<SkString::Rec> SkString::Rec::Make(const char text[], size_t len) {
         return sk_sp<SkString::Rec>(const_cast<Rec*>(&gEmptyRec));
     }
 
-    len = trim_size_t_to_u32(len);
-    // add 1 for terminating 0, then align4 so we can have some slop when growing the string
-    const size_t actualLength = SizeOfRec() + SkAlign4(len + 1);
-    SkASSERT_RELEASE(len < actualLength);  // Check for overflow.
+    SkSafeMath safe;
+    // We store a 32bit version of the length
+    uint32_t stringLen = safe.castTo<uint32_t>(len);
+    // Add SizeOfRec() for our overhead and 1 for null-termination
+    size_t allocationSize = safe.add(len, SizeOfRec() + sizeof(char));
+    // Align up to a multiple of 4
+    allocationSize = safe.alignUp(allocationSize, 4);
+
+    SkASSERT_RELEASE(safe.ok());
 
-    void* storage = ::operator new (actualLength);
-    sk_sp<Rec> rec(new (storage) Rec(SkToU32(len), 1));
+    void* storage = ::operator new (allocationSize);
+    sk_sp<Rec> rec(new (storage) Rec(stringLen, 1));
     if (text) {
         memcpy(rec->data(), text, len);
     }
diff --git a/tests/StringTest.cpp b/tests/StringTest.cpp
index 166637fd65..f9f76e9f21 100644
--- a/tests/StringTest.cpp
+++ b/tests/StringTest.cpp
@@ -304,3 +304,24 @@ DEF_TEST(String_Threaded, r) {
         thread.join();
     }
 }
+
+// Ensure that the string allocate doesn't internally overflow any calculations, and accidentally
+// let us create a string with a requested length longer than we can manage.
+DEF_TEST(String_huge, r) {
+    // start testing slightly below max 32
+    size_t size = SK_MaxU32 - 16;
+    // See where we crash, and manually check that its at the right point.
+    //
+    //  To test, change the false to true
+    while (false) {
+        // On a 64bit build, this should crash when size == 1 << 32, since we can't store
+        // that length in the string's header (which has a u32 slot for the length).
+        //
+        // On a 32bit build, this should crash the first time around, since we can't allocate
+        // anywhere near this amount.
+        //
+        SkString str(size);
+        size += 1;
+    }
+}
+
-- 
cgit v1.2.3