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

src/effects/SkLightingImageFilter.cpp

 /*
  * Copyright 2012 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkLightingImageFilter.h"
 #include "SkBitmap.h"
 #include "SkColorPriv.h"
@@ skipping 28 matching lines @@
 void setUniformPoint3(const GrGLUniformManager& uman, UniformHandle uni, const SkPoint3& point) {
     GR_STATIC_ASSERT(sizeof(SkPoint3) == 3 * sizeof(GrGLfloat));
     uman.set3fv(uni, 0, 1, &point.fX);
 }
 
 void setUniformNormal3(const GrGLUniformManager& uman, UniformHandle uni, const SkPoint3& point) {
     setUniformPoint3(uman, uni, SkPoint3(point.fX, point.fY, point.fZ));
 }
 #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];
     m[3] = m[4];
     m[6] = m[7];
     m[1] = m[2];
     m[4] = m[5];
     m[7] = m[8];
 }
 
 class DiffuseLightingType {
 public:
     DiffuseLightingType(SkScalar kd)
         : fKD(kd) {}
     SkPMColor light(const SkPoint3& normal, const SkPoint3& surfaceTolight, const SkPoint3& lightColor) const {
         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;
 };
 
 class SpecularLightingType {
 public:
     SpecularLightingType(SkScalar ks, SkScalar shininess)
         : fKS(ks), fShininess(shininess) {}
     SkPMColor light(const SkPoint3& normal, const SkPoint3& surfaceTolight, const SkPoint3& lightColor) const {
         SkPoint3 halfDir(surfaceTolight);
         halfDir.fZ += SK_Scalar1;        // eye position is always (0, 0, 1)
         halfDir.normalize();
         SkScalar colorScale = SkScalarMul(fKS,
             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;
     SkScalar fShininess;
 };
 
 inline SkScalar sobel(int a, int b, int c, int d, int e, int f, SkScalar scale) {
     return SkScalarMul(SkIntToScalar(-a + b - 2 * c + 2 * d -e + f), scale);
 }
 
@@ skipping 552 matching lines @@
 };
 
 ///////////////////////////////////////////////////////////////////////////////
 
 class SkSpotLight : public SkLight {
 public:
     SkSpotLight(const SkPoint3& location, const SkPoint3& target, SkScalar specularExponent, SkScalar cutoffAngle, SkColor color)
      : INHERITED(color),
        fLocation(location),
        fTarget(target),
-       fSpecularExponent(specularExponent)
+       fSpecularExponent(SkScalarPin(specularExponent, kSpecularExponentMin, kSpecularExponentMax))
     {
        fS = target - location;
        fS.normalize();
        fCosOuterConeAngle = SkScalarCos(SkDegreesToRadians(cutoffAngle));
        const SkScalar antiAliasThreshold = SkFloatToScalar(0.016f);
        fCosInnerConeAngle = fCosOuterConeAngle + antiAliasThreshold;
        fConeScale = SkScalarInvert(antiAliasThreshold);
     }
 
     SkPoint3 surfaceToLight(int x, int y, int z, SkScalar surfaceScale) const {
@@ skipping 63 matching lines @@
 
         const SkSpotLight& o = static_cast<const SkSpotLight&>(other);
         return INHERITED::isEqual(other) &&
                fLocation == o.fLocation &&
                fTarget == o.fTarget &&
                fSpecularExponent == o.fSpecularExponent &&
                fCosOuterConeAngle == o.fCosOuterConeAngle;
     }
 
 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;
     SkScalar fSpecularExponent;
     SkScalar fCosOuterConeAngle;
     SkScalar fCosInnerConeAngle;
     SkScalar fConeScale;
     SkPoint3 fS;
 };
 
@@ skipping 786 matching lines @@
 
 #endif
 
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_START(SkLightingImageFilter)
     SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkDiffuseLightingImageFilter)
     SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkSpecularLightingImageFilter)
     SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkDistantLight)
     SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkPointLight)
     SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkSpotLight)
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_END