Bug 1224396 - Skia path allocation cleanups. r=msreckovic
authorLee Salzman <lsalzman@mozilla.com>
Wed, 22 Nov 2017 12:19:29 -0500
changeset 393195 db4543ce21ce1e8a1c81b685a16e61f71db2f602
parent 393194 3d1cdd68d47bbc270b178afcbd38f277bbf4dc35
child 393196 ec39af7d2914d83bdb491d0f1536fe710eb9cc72
push id32954
push usershindli@mozilla.com
push dateWed, 22 Nov 2017 21:30:30 +0000
treeherdermozilla-central@960f50c2e0a9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmsreckovic
bugs1224396
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1224396 - Skia path allocation cleanups. r=msreckovic MozReview-Commit-ID: GAf2vC1Fucv
gfx/skia/skia/include/core/SkPathRef.h
gfx/skia/skia/src/core/SkArenaAlloc.cpp
gfx/skia/skia/src/core/SkArenaAlloc.h
--- a/gfx/skia/skia/include/core/SkPathRef.h
+++ b/gfx/skia/skia/include/core/SkPathRef.h
@@ -11,17 +11,17 @@
 
 #include "../private/SkAtomics.h"
 #include "../private/SkTDArray.h"
 #include "SkMatrix.h"
 #include "SkPoint.h"
 #include "SkRRect.h"
 #include "SkRect.h"
 #include "SkRefCnt.h"
-#include <stddef.h> // ptrdiff_t
+#include "../private/SkTemplates.h"
 
 class SkRBuffer;
 class SkWBuffer;
 
 /**
  * Holds the path verbs and points. It is versioned by a generation ID. None of its public methods
  * modify the contents. To modify or append to the verbs/points wrap the SkPathRef in an
  * SkPathRef::Editor object. Installing the editor resets the generation ID. It also performs
@@ -428,41 +428,45 @@ private:
     SkPoint* growForVerb(int /*SkPath::Verb*/ verb, SkScalar weight);
 
     /**
      * Ensures that the free space available in the path ref is >= size. The verb and point counts
      * are not changed.
      */
     void makeSpace(size_t size) {
         SkDEBUGCODE(this->validate();)
-        ptrdiff_t growSize = size - fFreeSpace;
-        if (growSize <= 0) {
+        if (size <= fFreeSpace) {
             return;
         }
+        size_t growSize = size - fFreeSpace;
         size_t oldSize = this->currSize();
         // round to next multiple of 8 bytes
         growSize = (growSize + 7) & ~static_cast<size_t>(7);
         // we always at least double the allocation
-        if (static_cast<size_t>(growSize) < oldSize) {
+        if (growSize < oldSize) {
             growSize = oldSize;
         }
         if (growSize < kMinSize) {
             growSize = kMinSize;
         }
-        size_t newSize = oldSize + growSize;
+        constexpr size_t maxSize = std::numeric_limits<size_t>::max();
+        size_t newSize;
+        if (growSize <= maxSize - oldSize) {
+            newSize = oldSize + growSize;
+        } else {
+            SK_ABORT("Path too big.");
+        }
         // Note that realloc could memcpy more than we need. It seems to be a win anyway. TODO:
         // encapsulate this.
         fPoints = reinterpret_cast<SkPoint*>(sk_realloc_throw(fPoints, newSize));
         size_t oldVerbSize = fVerbCnt * sizeof(uint8_t);
-        void* newVerbsDst = reinterpret_cast<void*>(
-                                reinterpret_cast<intptr_t>(fPoints) + newSize - oldVerbSize);
-        void* oldVerbsSrc = reinterpret_cast<void*>(
-                                reinterpret_cast<intptr_t>(fPoints) + oldSize - oldVerbSize);
+        void* newVerbsDst = SkTAddOffset<void>(fPoints, newSize - oldVerbSize);
+        void* oldVerbsSrc = SkTAddOffset<void>(fPoints, oldSize - oldVerbSize);
         memmove(newVerbsDst, oldVerbsSrc, oldVerbSize);
-        fVerbs = reinterpret_cast<uint8_t*>(reinterpret_cast<intptr_t>(fPoints) + newSize);
+        fVerbs = SkTAddOffset<uint8_t>(fPoints, newSize);
         fFreeSpace += growSize;
         SkDEBUGCODE(this->validate();)
     }
 
     /**
      * Private, non-const-ptr version of the public function verbsMemBegin().
      */
     uint8_t* verbsMemWritable() {
--- a/gfx/skia/skia/src/core/SkArenaAlloc.cpp
+++ b/gfx/skia/skia/src/core/SkArenaAlloc.cpp
@@ -3,16 +3,17 @@
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include <algorithm>
 #include <cstddef>
 #include "SkArenaAlloc.h"
+#include "SkTypes.h"
 
 static char* end_chain(char*) { return nullptr; }
 
 char* SkArenaAlloc::SkipPod(char* footerEnd) {
     char* objEnd = footerEnd - (sizeof(Footer) + sizeof(int32_t));
     int32_t skip;
     memmove(&skip, objEnd, sizeof(int32_t));
     return objEnd - skip;
@@ -90,29 +91,41 @@ void SkArenaAlloc::installUint32Footer(F
 }
 
 void SkArenaAlloc::ensureSpace(uint32_t size, uint32_t alignment) {
     constexpr uint32_t headerSize = sizeof(Footer) + sizeof(ptrdiff_t);
     // The chrome c++ library we use does not define std::max_align_t.
     // This must be conservative to add the right amount of extra memory to handle the alignment
     // padding.
     constexpr uint32_t alignof_max_align_t = 8;
-    uint32_t objSizeAndOverhead = size + headerSize + sizeof(Footer);
+    constexpr uint32_t maxSize = std::numeric_limits<uint32_t>::max();
+    constexpr uint32_t overhead = headerSize + sizeof(Footer);
+    SkASSERT_RELEASE(size <= maxSize - overhead);
+    uint32_t objSizeAndOverhead = size + overhead;
     if (alignment > alignof_max_align_t) {
-        objSizeAndOverhead += alignment - 1;
+        uint32_t alignmentOverhead = alignment - 1;
+        SkASSERT_RELEASE(objSizeAndOverhead <= maxSize - alignmentOverhead);
+        objSizeAndOverhead += alignmentOverhead;
     }
 
-    uint32_t allocationSize = std::max(objSizeAndOverhead, fExtraSize * fFib0);
-    fFib0 += fFib1;
-    std::swap(fFib0, fFib1);
+    uint32_t minAllocationSize;
+    if (fExtraSize <= maxSize / fFib0) {
+        minAllocationSize = fExtraSize * fFib0;
+        fFib0 += fFib1;
+        std::swap(fFib0, fFib1);
+    } else {
+        minAllocationSize = maxSize;
+    }
+    uint32_t allocationSize = std::max(objSizeAndOverhead, minAllocationSize);
 
     // Round up to a nice size. If > 32K align to 4K boundary else up to max_align_t. The > 32K
     // heuristic is from the JEMalloc behavior.
     {
         uint32_t mask = allocationSize > (1 << 15) ? (1 << 12) - 1 : 16 - 1;
+        SkASSERT_RELEASE(allocationSize <= maxSize - mask);
         allocationSize = (allocationSize + mask) & ~mask;
     }
 
     char* newBlock = new char[allocationSize];
 
     auto previousDtor = fDtorCursor;
     fCursor = newBlock;
     fDtorCursor = newBlock;
--- a/gfx/skia/skia/src/core/SkArenaAlloc.h
+++ b/gfx/skia/skia/src/core/SkArenaAlloc.h
@@ -152,24 +152,27 @@ private:
 
     char* allocObject(uint32_t size, uint32_t alignment);
 
     char* allocObjectWithFooter(uint32_t sizeIncludingFooter, uint32_t alignment);
 
     template <typename T>
     char* commonArrayAlloc(uint32_t count) {
         char* objStart;
+        SkASSERT_RELEASE(count <= std::numeric_limits<uint32_t>::max() / sizeof(T));
         uint32_t arraySize = SkTo<uint32_t>(count * sizeof(T));
         uint32_t alignment = SkTo<uint32_t>(alignof(T));
 
         if (skstd::is_trivially_destructible<T>::value) {
             objStart = this->allocObject(arraySize, alignment);
             fCursor = objStart + arraySize;
         } else {
-            uint32_t totalSize = arraySize + sizeof(Footer) + sizeof(uint32_t);
+            constexpr uint32_t overhead = sizeof(Footer) + sizeof(uint32_t);
+            SkASSERT_RELEASE(arraySize <= std::numeric_limits<uint32_t>::max() - overhead);
+            uint32_t totalSize = arraySize + overhead;
             objStart = this->allocObjectWithFooter(totalSize, alignment);
 
             // Can never be UB because max value is alignof(T).
             uint32_t padding = SkTo<uint32_t>(objStart - fCursor);
 
             // Advance to end of array to install footer.?
             fCursor = objStart + arraySize;
             this->installUint32Footer(