Fix memory leak in FuzzGradients

fuzz/FuzzGradients.cpp

 /*
  * Copyright 2016 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "Fuzz.h"
 #include "SkCanvas.h"
 #include "SkGradientShader.h"
@@ skipping 14 matching lines @@
         !fuzz->next(&transY) ||
         !fuzz->next(&persp0) ||
         !fuzz->next(&persp1) ||
         !fuzz->next(&persp2)) {
         return false;
     }
     m->setAll(scaleX, skewX, transX, skewY, scaleY, transY, persp0, persp1, persp2);
     return true;
 }
 
-bool initGradientParams(Fuzz* fuzz, uint32_t* count, SkColor** colors, SkScalar** pos,
-    SkShader::TileMode* mode) {
+bool initGradientParams(Fuzz* fuzz, uint32_t* count, std::vector<SkColor>* colors,
+    std::vector<SkScalar>* pos, SkShader::TileMode* mode) {
     if (fuzz->remaining() < sizeof(uint32_t)) {
         return false;
     }
     uint32_t t_count;
-    SkColor* t_colors;
-    SkScalar* t_pos;
-
     t_count = fuzz->nextRangeU(0, MAX_COUNT);
     if (t_count == 1) {

[mtklein_C, 2016/10/24 19:26:35]

Why don't we fuzz count == 1?

[kjlubick, 2016/10/24 20:22:30]

The API said >=2, so I followed that rule.  Looking inside the code though, it
does seem that there is code that activates if count == 1.  

I've removed it to see that we properly handle when users don't read the API

         t_count = 2;
     }
 
     if (fuzz->remaining() < (1 + t_count * (sizeof(SkColor) + sizeof(SkScalar)))) {

[mtklein_C, 2016/10/24 19:26:35]

What's the 1 extra byte here for?

It's probably a good idea to cut this code if possible.  It's guarding later
code, but it means you have to keep more state in your head implicitly.  Won't
the calls to next() just fail if this isn't true?

[kjlubick, 2016/10/24 20:22:30]

FWIW, the extra 1 byte is for
static_cast<SkShader::TileMode>(fuzz->nextRangeU(0, 3));

I'll be reworking this, I don't know why it seemed like a good idea at the time.

         return false;
     }
-    t_colors = new SkColor[t_count];
-    t_pos = new SkScalar[t_count];
+
+    SkColor c;

[mtklein_C, 2016/10/24 19:26:35]

It doesn't make things any faster or slower to put these out here, but it does
make the code a little harder to read.  It's easiest to read code that's as
tightly scoped as possible.

Let's do something like

colors->clear();
pos   ->clear();
for (int i = 0; i < count; i++) {
    SkColor  c;
    SkScalar s;
    if (!fuzz->next(&c) || !fuzz->next(&s)) {
        return false;
    }
    colors->push_back(c);
    pos   ->push_back(s);
}
if (count) {
    std::sort(pos->begin(), pos->end());
    (*pos)[0]       = 0;
    (*pos)[count-1] = 1;
}

[kjlubick, 2016/10/24 20:22:30]

Done.

+    SkScalar s;
     for (uint32_t i = 0; i < t_count; i++) {

[mtklein_C, 2016/10/24 19:26:35]

ordinarily we'd write int instead of uint32_t here.  uint32_t gives off a vibe
of, this must be exactly 4 bytes.

[kjlubick, 2016/10/24 20:22:30]

Well, then I get a warning because t_count is uint32_t.  What's the preferred
way to deal with that?

[mtklein_C, 2016/10/24 20:54:38]

This is why we always make things like this ints.  Generally use int, but use
size_t for byte lengths.  uint32_t should only be used if something explicitly
needs to be 4 bytes long (if, e.g you're breaking it up into 4 uint8_t using
shifts).  "unsigned" pretty much is never used.

-        fuzz->next(&t_colors[i]);
-        fuzz->next(&t_pos[i]);
+        fuzz->next(&c);

[mtklein_C, 2016/10/24 19:26:35]

Don't we need to check that calls to next() succeed?  Have you considered
marking next() with SK_WARN_UNUSED_RESULT?

[kjlubick, 2016/10/24 20:22:30]

They only wouldn't succeed if there isn't enough "randomness" in fuzz.  Since
I'm reworking the above guard, I will be checking that now.

SK_WARN_UNUSED_RESULT added.

+        colors->push_back(c);
+        fuzz->next(&s);
+        pos->push_back(s);
     }
 
     if (t_count == 0) {
         *count = 0;
-        *colors = NULL;
-        *pos = NULL;
+        colors->clear();
+        pos->clear();
     } else {
-        std::sort(t_pos, t_pos + t_count);
-        t_pos[0] = 0;
-        t_pos[t_count - 1] = 1;
+        std::sort(pos->begin(), pos->begin() + t_count);

[mtklein_C, 2016/10/24 19:26:35]

pos->begin() + t_count is also known as pos->end().  That's the more idomatic
thing to write here.

[kjlubick, 2016/10/24 20:22:30]

Done.

+        (*pos)[0] = 0;
+        (*pos)[t_count - 1] = 1;
         *count = t_count;
-        *colors = t_colors;
-        *pos = t_pos;
     }
 
     *mode = static_cast<SkShader::TileMode>(fuzz->nextRangeU(0, 3));
     return true;
 }
 
 void fuzzLinearGradient(Fuzz* fuzz) {
         SkScalar a, b, c, d;
         bool useLocalMatrix, useGlobalMatrix;
         if (!fuzz->next(&a)               ||
             !fuzz->next(&b)               ||
             !fuzz->next(&c)               ||
             !fuzz->next(&d)               ||
             !fuzz->next(&useLocalMatrix)  ||
             !fuzz->next(&useGlobalMatrix)) {
             return;
         }
         SkPoint pts[2] = {SkPoint::Make(a,b), SkPoint::Make(c, d)};
 
         uint32_t count;
-        SkColor* colors;
-        SkScalar* pos;
+        std::vector<SkColor> colors;
+        std::vector<SkScalar> pos;
         SkShader::TileMode mode;
         if (!initGradientParams(fuzz, &count, &colors, &pos, &mode)) {
             return;
         }
 
         SkPaint p;
         uint32_t flags;
         if (!fuzz->next(&flags)) {
             return;
         }
 
         SkTLazy<SkMatrix> localMatrix;
         if (useLocalMatrix && !makeMatrix(fuzz, localMatrix.init())) {
             return;
         }
-        p.setShader(SkGradientShader::MakeLinear(pts, colors, pos, count, mode,
-            flags, localMatrix.getMaybeNull()));
+        p.setShader(SkGradientShader::MakeLinear(pts, colors.data(),
+            pos.data(), count, mode, flags, localMatrix.getMaybeNull()));
 
         sk_sp<SkSurface> surface(SkSurface::MakeRasterN32Premul(50, 50));
         if (useGlobalMatrix) {
             SkMatrix gm;
             if (!makeMatrix(fuzz, &gm)) {
                 return;
             }
             SkCanvas* c = surface->getCanvas();
             c->setMatrix(gm);
             c->drawPaint(p);
         } else {
             surface->getCanvas()->drawPaint(p);
         }
 }
 
 void fuzzRadialGradient(Fuzz* fuzz) {
         SkScalar a, b, radius;
         bool useLocalMatrix, useGlobalMatrix;
         if (!fuzz->next(&a)               ||
             !fuzz->next(&b)               ||
             !fuzz->next(&radius)          ||
             !fuzz->next(&useLocalMatrix)  ||
             !fuzz->next(&useGlobalMatrix)) {
             return;
         }
         SkPoint center = SkPoint::Make(a,b);
 
         uint32_t count;
-        SkColor* colors;
-        SkScalar* pos;
+        std::vector<SkColor> colors;
+        std::vector<SkScalar> pos;
         SkShader::TileMode mode;
         if (!initGradientParams(fuzz, &count, &colors, &pos, &mode)) {
             return;
         }
 
         SkPaint p;
         uint32_t flags;
         if (!fuzz->next(&flags)) {
             return;
         }
 
         SkTLazy<SkMatrix> localMatrix;
         if (useLocalMatrix && !makeMatrix(fuzz, localMatrix.init())) {
             return;
         }
-        p.setShader(SkGradientShader::MakeRadial(center, radius, colors, pos,
-            count, mode, flags, localMatrix.getMaybeNull()));
+        p.setShader(SkGradientShader::MakeRadial(center, radius, colors.data(),
+            pos.data(), count, mode, flags, localMatrix.getMaybeNull()));
 
 
         sk_sp<SkSurface> surface(SkSurface::MakeRasterN32Premul(50, 50));
         if (useGlobalMatrix) {
             SkMatrix gm;
             if (!makeMatrix(fuzz, &gm)) {
                 return;
             }
             SkCanvas* c = surface->getCanvas();
             c->setMatrix(gm);
@@ skipping 13 matching lines @@
             !fuzz->next(&d)               ||
             !fuzz->next(&endRadius)       ||
             !fuzz->next(&useLocalMatrix)  ||
             !fuzz->next(&useGlobalMatrix)) {
             return;
         }
         SkPoint start = SkPoint::Make(a, b);
         SkPoint end = SkPoint::Make(c, d);
 
         uint32_t count;
-        SkColor* colors;
-        SkScalar* pos;
+        std::vector<SkColor> colors;
+        std::vector<SkScalar> pos;
         SkShader::TileMode mode;
         if (!initGradientParams(fuzz, &count, &colors, &pos, &mode)) {
             return;
         }
 
         SkPaint p;
         uint32_t flags;
         if (!fuzz->next(&flags)) {
             return;
         }
 
         SkTLazy<SkMatrix> localMatrix;
         if (useLocalMatrix && !makeMatrix(fuzz, localMatrix.init())) {
             return;
         }
-        p.setShader(SkGradientShader::MakeTwoPointConical(start, startRadius, end,
-            endRadius, colors, pos, count, mode, flags, localMatrix.getMaybeNull()));
+        p.setShader(SkGradientShader::MakeTwoPointConical(start, startRadius,
+            end, endRadius, colors.data(), pos.data(), count, mode, flags,
+            localMatrix.getMaybeNull()));
 
         sk_sp<SkSurface> surface(SkSurface::MakeRasterN32Premul(50, 50));
         if (useGlobalMatrix) {
             SkMatrix gm;
             if (!makeMatrix(fuzz, &gm)) {
                 return;
             }
             SkCanvas* c = surface->getCanvas();
             c->setMatrix(gm);
             c->drawPaint(p);
         } else {
             surface->getCanvas()->drawPaint(p);
         }
 }
 
 void fuzzSweepGradient(Fuzz* fuzz) {
         SkScalar cx, cy;
         bool useLocalMatrix, useGlobalMatrix;
         if (!fuzz->next(&cx)              ||
             !fuzz->next(&cy)              ||
             !fuzz->next(&useLocalMatrix)  ||
             !fuzz->next(&useGlobalMatrix)) {
             return;
         }
 
         uint32_t count;
-        SkColor* colors;
-        SkScalar* pos;
+        std::vector<SkColor> colors;
+        std::vector<SkScalar> pos;
         SkShader::TileMode mode;
         if (!initGradientParams(fuzz, &count, &colors, &pos, &mode)) {
             return;
         }
 
         SkPaint p;
         if (useLocalMatrix) {
             SkMatrix m;
             if (!makeMatrix(fuzz, &m)) {
                 return;
             }
             uint32_t flags;
             if (!fuzz->next(&flags)) {
                 return;
             }
-            p.setShader(SkGradientShader::MakeSweep(cx, cy, colors, pos, count, flags, &m));
+            p.setShader(SkGradientShader::MakeSweep(cx, cy, colors.data(),
+                pos.data(), count, flags, &m));
         } else {
-            p.setShader(SkGradientShader::MakeSweep(cx, cy, colors, pos, count));
+            p.setShader(SkGradientShader::MakeSweep(cx, cy, colors.data(),
+                pos.data(), count));
         }
 
 
         sk_sp<SkSurface> surface(SkSurface::MakeRasterN32Premul(50, 50));
         if (useGlobalMatrix) {
             SkMatrix gm;
             if (!makeMatrix(fuzz, &gm)) {
                 return;
             }
             SkCanvas* c = surface->getCanvas();
@@ skipping 21 matching lines @@
             return;
         case 2:
             SkDebugf("TwoPointConicalGradient\n");
             fuzzTwoPointConicalGradient(fuzz);
             return;
     }
     SkDebugf("SweepGradient\n");
     fuzzSweepGradient(fuzz);
     return;
 }