I'm investigating how to make the IPC transfer a bit more secure on the

src/effects/SkLightingImageFilter.cpp

Index: src/effects/SkLightingImageFilter.cpp
diff --git a/src/effects/SkLightingImageFilter.cpp b/src/effects/SkLightingImageFilter.cpp
index 7a74f73624bc89411c12d79c8e149417f2e50852..9bcb1d8f658f240f61f659cf16d4a78e494f3d34 100644
--- a/src/effects/SkLightingImageFilter.cpp
+++ b/src/effects/SkLightingImageFilter.cpp
@@ -46,6 +46,18 @@ void setUniformNormal3(const GrGLUniformManager& uman, UniformHandle uni, const
 }
 #endif
 
+void getClampedColor(const SkPoint3 color, U8CPU& r8, U8CPU& g8, U8CPU& b8) {

[Stephen White, 2013/08/21 20:49:26]

Personal style, but I'm not a fan of non-const refs. Make it a pointer arg, and
it's easier to see from the call site what gets modified.

In fact, I'd just use SkClampMax on each component in the the SkPackARGB32()
call rather than rolling your own, and do away with this function entirely.

[sugoi, 2013/08/21 21:12:09]

Ok, will do.

[scroggo, 2013/08/21 23:25:27]

That's actually in the Skia style guide, so it's more than your personal
preference ;)

[sugoi1, 2013/08/22 15:41:00]

Done.

+    // Sending negative numbers to SkPackARGB32 would convert them to
+    // unsigned, changing small negative integers into very large positive
+    // unsigned integers. To avoid issues, clamp colors to the [0, 255] range.
+    int r = SkScalarFloorToInt(color.fX);
+    int g = SkScalarFloorToInt(color.fY);
+    int b = SkScalarFloorToInt(color.fZ);
+    r8 = r < 0 ? 0 : (r > 255 ? 255 : r);
+    g8 = g < 0 ? 0 : (g > 255 ? 255 : g);
+    b8 = b < 0 ? 0 : (b > 255 ? 255 : b);
+}
+
 // Shift matrix components to the left, as we advance pixels to the right.
 inline void shiftMatrixLeft(int m[9]) {
     m[0] = m[1];
@@ -64,10 +76,9 @@ public:
         SkScalar colorScale = SkScalarMul(fKD, normal.dot(surfaceTolight));
         colorScale = SkScalarClampMax(colorScale, SK_Scalar1);
         SkPoint3 color(lightColor * colorScale);
-        return SkPackARGB32(255,
-                            SkScalarFloorToInt(color.fX),
-                            SkScalarFloorToInt(color.fY),
-                            SkScalarFloorToInt(color.fZ));
+        U8CPU r(0), g(0), b(0);
+        getClampedColor(color, r, g, b);
+        return SkPackARGB32(255, r, g, b);
     }
 private:
     SkScalar fKD;
@@ -85,10 +96,9 @@ public:
             SkScalarPow(normal.dot(halfDir), fShininess));
         colorScale = SkScalarClampMax(colorScale, SK_Scalar1);
         SkPoint3 color(lightColor * colorScale);
-        return SkPackARGB32(SkScalarFloorToInt(color.maxComponent()),
-                            SkScalarFloorToInt(color.fX),
-                            SkScalarFloorToInt(color.fY),
-                            SkScalarFloorToInt(color.fZ));
+        U8CPU r(0), g(0), b(0);
+        getClampedColor(color, r, g, b);
+        return SkPackARGB32(r > g ? (r > b ? r : b) : (g > b ? g : b), r, g, b);

[Stephen White, 2013/08/21 20:49:26]

Could we not simply clamp the alpha after doing maxComponent()? Kind of ugly to
do it all inline here.

[sugoi, 2013/08/21 21:12:09]

Sure, I'll just add the clamping code here.

[sugoi1, 2013/08/22 15:41:00]

Done.

     }
 private:
     SkScalar fKS;
@@ -661,7 +671,7 @@ public:
      : INHERITED(color),
        fLocation(location),
        fTarget(target),
-       fSpecularExponent(specularExponent)
+       fSpecularExponent(SkScalarPin(specularExponent, kSpecularExponentMin, kSpecularExponentMax))
     {
        fS = target - location;
        fS.normalize();
@@ -745,6 +755,11 @@ protected:
     }
 
 private:
+    // According to the spec, the specular term should be in the range [1, 128] :
+    // http://www.w3.org/TR/SVG/filters.html#feSpecularLightingSpecularExponentAttribute
+    static const SkScalar kSpecularExponentMin = SkFloatToScalar(1.0f);
+    static const SkScalar kSpecularExponentMax = SkFloatToScalar(128.0f);
+
     typedef SkLight INHERITED;
     SkPoint3 fLocation;
     SkPoint3 fTarget;