commit c7829eb3161f0de3682ab93f60d79a1abb5267da
author Michael Ludwig <michaelludwig@google.com> Thu Aug 12 10:41:53 2021 -0400
committer SkCQ <skcq-be@skia-corp.google.com.iam.gserviceaccount.com> Thu Aug 12 15:52:09 2021 +0000
tree 8882796ed2db3788eb865c3a900e5605501223db
parent 06273bcb8abcc6385770f06d8d0b1583542b2a26

Guard against large content bounds for layer mapping calculations

Bug: chromium:1232744, chromium:1226344
Change-Id: I1fda9fb01f8c501bb5b410a8c7cd292f53b13142
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/438577
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>

src/core/SkCanvas.cpp

diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index f04a1c5..39fd744 100644
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -657,10 +657,10 @@
 
 // Helper function to compute the center reference point used for scale decomposition under
 // non-linear transformations.
-static bool compute_decomposition_center(const SkMatrix& dstToLocal,
-                                         const skif::ParameterSpace<SkRect>* contentBounds,
-                                         const skif::DeviceSpace<SkIRect>& targetOutput,
-                                         skif::ParameterSpace<SkPoint>* out) {
+static skif::ParameterSpace<SkPoint> compute_decomposition_center(
+        const SkMatrix& dstToLocal,
+        const skif::ParameterSpace<SkRect>* contentBounds,
+        const skif::DeviceSpace<SkIRect>& targetOutput) {
     // Will use the inverse and center of the device bounds if the content bounds aren't provided.
     SkRect rect = contentBounds ? SkRect(*contentBounds) : SkRect::Make(SkIRect(targetOutput));
     SkPoint center = {rect.centerX(), rect.centerY()};
@@ -670,8 +670,7 @@
         dstToLocal.mapPoints(&center, 1);
     }
 
-    *out = skif::ParameterSpace<SkPoint>(center);
-    return true;
+    return skif::ParameterSpace<SkPoint>(center);
 }
 
 // Compute suitable transformations and layer bounds for a new layer that will be used as the source
@@ -686,13 +685,14 @@
         const skif::DeviceSpace<SkIRect>& targetOutput,
         const skif::ParameterSpace<SkRect>* contentBounds = nullptr,
         bool mustCoverDst = true) {
-    skif::ParameterSpace<SkPoint> center;
     SkMatrix dstToLocal;
     if (!localToDst.isFinite() ||
-        !localToDst.invert(&dstToLocal) ||
-        !compute_decomposition_center(dstToLocal, contentBounds, targetOutput, &center)) {
+        !localToDst.invert(&dstToLocal)) {
         return {{}, skif::LayerSpace<SkIRect>(SkIRect::MakeEmpty())};
     }
+
+    skif::ParameterSpace<SkPoint> center =
+            compute_decomposition_center(dstToLocal, contentBounds, targetOutput);
     // *after* possibly getting a representative point from the provided content bounds, it might
     // be necessary to discard the bounds for subsequent layer calculations.
     if (mustCoverDst) {
@@ -743,7 +743,11 @@
         SkMatrix adjust = SkMatrix::MakeRectToRect(SkRect::Make(SkIRect(layerBounds)),
                                                    SkRect::Make(SkIRect(newLayerBounds)),
                                                    SkMatrix::kFill_ScaleToFit);
-        if (!mapping.adjustLayerSpace(adjust)) {
+        // If the adjust matrix isn't invertible, or if multiplying it into the device transform
+        // causes overflows, then subsequent SkDevice coordinate spaces would be invalid, so
+        // just return the empty bounds and skip the layer.
+        if (!mapping.adjustLayerSpace(adjust) ||
+            !mapping.deviceMatrix().isFinite()) {
             layerBounds = skif::LayerSpace<SkIRect>(SkIRect::MakeEmpty());
         } else {
             layerBounds = newLayerBounds;

src/core/SkImageFilterTypes.cpp

diff --git a/src/core/SkImageFilterTypes.cpp b/src/core/SkImageFilterTypes.cpp
index 0ec20a6..88077b8 100644
--- a/src/core/SkImageFilterTypes.cpp
+++ b/src/core/SkImageFilterTypes.cpp
@@ -50,12 +50,13 @@
         // Perspective, which has a non-uniform scaling effect on the filter. Pick a single scale
         // factor that best matches where the filter will be evaluated.
         SkScalar scale = SkMatrixPriv::DifferentialAreaScale(ctm, SkPoint(representativePoint));
-        if (SkScalarIsFinite(scale)) {
+        if (SkScalarIsFinite(scale) && !SkScalarNearlyZero(scale)) {
             // Now take the sqrt to go from an area scale factor to a scaling per X and Y
             // FIXME: It would be nice to be able to choose a non-uniform scale.
             scale = SkScalarSqrt(scale);
         } else {
             // The representative point was behind the W = 0 plane, so don't factor out any scale.
+            // NOTE: This makes remainder and layer the same as the MatrixCapability::Translate case
             scale = 1.f;
         }
 
@@ -63,6 +64,7 @@
         remainder.preScale(SkScalarInvert(scale), SkScalarInvert(scale));
         layer = SkMatrix::Scale(scale, scale);
     }
+
     return Mapping(remainder, layer);
 }
 

src/core/SkMatrix.cpp

diff --git a/src/core/SkMatrix.cpp b/src/core/SkMatrix.cpp
index 80c282c..9a57187 100644
--- a/src/core/SkMatrix.cpp
+++ b/src/core/SkMatrix.cpp
@@ -1864,8 +1864,7 @@
                                           m.getScaleX(), m.getSkewY(), m.getPerspX(),
                                           m.getSkewX(), m.getScaleY(), m.getPerspY());
 
-    SkScalar denom = 1.f / xyw.fZ; // 1/w
+    double denom = 1.0 / xyw.fZ;   // 1/w
     denom = denom * denom * denom; // 1/w^3
-
-    return SkScalarAbs(SkDoubleToScalar(sk_determinant(jacobian.fMat, true)) * denom);
+    return SkScalarAbs(SkDoubleToScalar(sk_determinant(jacobian.fMat, true) * denom));
 }