Fix memory leak in FuzzGradients

fuzz/FuzzGradients.cpp

Index: fuzz/FuzzGradients.cpp
diff --git a/fuzz/FuzzGradients.cpp b/fuzz/FuzzGradients.cpp
index 2478bca283203101e6c5631f05b069f6cf2d68f2..76da7c75f72de1e665ec4313d145af637e9aa363 100644
--- a/fuzz/FuzzGradients.cpp
+++ b/fuzz/FuzzGradients.cpp
@@ -32,15 +32,12 @@ bool makeMatrix(Fuzz* fuzz, SkMatrix* m) {
     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;
@@ -49,24 +46,25 @@ bool initGradientParams(Fuzz* fuzz, uint32_t* count, SkColor** colors, SkScalar*
     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));
@@ -87,8 +85,8 @@ void fuzzLinearGradient(Fuzz* fuzz) {
         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;
@@ -104,8 +102,8 @@ void fuzzLinearGradient(Fuzz* fuzz) {
         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) {
@@ -134,8 +132,8 @@ void fuzzRadialGradient(Fuzz* fuzz) {
         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;
@@ -151,8 +149,8 @@ void fuzzRadialGradient(Fuzz* fuzz) {
         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));
@@ -186,8 +184,8 @@ void fuzzTwoPointConicalGradient(Fuzz* fuzz) {
         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;
@@ -203,8 +201,9 @@ void fuzzTwoPointConicalGradient(Fuzz* fuzz) {
         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) {
@@ -231,8 +230,8 @@ void fuzzSweepGradient(Fuzz* fuzz) {
         }
 
         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;
@@ -248,9 +247,11 @@ void fuzzSweepGradient(Fuzz* fuzz) {
             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));
         }