Bug 1457288 - Part 3: Change distance field sizes and indexes to be unsigned ints, avoiding undefined behavior with potential int overflows. r=mats
authorBrad Werth <bwerth@mozilla.com>
Mon, 30 Apr 2018 12:25:13 -0700
changeset 472601 2a306bd6aa1c5fee9fe34f199cb7fee4d2dbc562
parent 472600 fbae755d80b0c94a4d7f7db4a14f928bd4a8cbf8
child 472602 eb885e55357454de7ddcca8864a8caea11b6c2d2
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1457288
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1457288 - Part 3: Change distance field sizes and indexes to be unsigned ints, avoiding undefined behavior with potential int overflows. r=mats
layout/generic/nsFloatManager.cpp
--- a/layout/generic/nsFloatManager.cpp
+++ b/layout/generic/nsFloatManager.cpp
@@ -817,41 +817,39 @@ nsFloatManager::EllipseShapeInfo::Ellips
     LayoutDeviceIntSize(usedMargin5X / 5, usedMargin5X / 5);
 
   // Since our distance field is computed with a 5x5 neighborhood, but only
   // looks in the negative block and negative inline directions, it is
   // effectively a 3x3 neighborhood. We need to expand our distance field
   // outwards by a further 2 pixels in both axes (on the minimum block edge
   // and the minimum inline edge). We call this edge area the expanded region.
 
-  // Our expansion amounts need to be non-negative for our math to work, but
-  // we don't want to deal with casting them from unsigned ints.
-  static const int32_t iExpand = 2;
-  static const int32_t bExpand = 2;
+  static const uint32_t iExpand = 2;
+  static const uint32_t bExpand = 2;
 
   // Clamp the size of our distance field sizes to prevent multiplication
   // overflow.
-  static const int32_t DF_SIDE_MAX =
+  static const uint32_t DF_SIDE_MAX =
     floor(sqrt((double)(std::numeric_limits<int32_t>::max())));
-  const int32_t iSize = std::min(bounds.width + iExpand, DF_SIDE_MAX);
-  const int32_t bSize = std::min(bounds.height + bExpand, DF_SIDE_MAX);
+  const uint32_t iSize = std::min(bounds.width + iExpand, DF_SIDE_MAX);
+  const uint32_t bSize = std::min(bounds.height + bExpand, DF_SIDE_MAX);
   auto df = MakeUniqueFallible<dfType[]>(iSize * bSize);
   if (!df) {
     // Without a distance field, we can't reason about the float area.
     return;
   }
 
   // Single pass setting distance field, in positive block direction, three
   // cases:
   // 1) Expanded region pixel: set to MAX_MARGIN_5X.
   // 2) Pixel within the ellipse: set to 0.
   // 3) Other pixel: set to minimum neighborhood distance value, computed
   //                 with 5-7-11 chamfer.
 
-  for (int32_t b = 0; b < bSize; ++b) {
+  for (uint32_t b = 0; b < bSize; ++b) {
     bool bIsInExpandedRegion(b < bExpand);
     nscoord bInAppUnits = (b - bExpand) * aAppUnitsPerDevPixel;
     bool bIsMoreThanEllipseBEnd(bInAppUnits > mRadii.height);
 
     // Find the i intercept of the ellipse edge for this block row, and
     // adjust it to compensate for the expansion of the inline dimension.
     // If we're in the expanded region, or if we're using a b that's more
     // than the bStart of the ellipse, the intercept is nscoord_MIN.
@@ -859,27 +857,27 @@ nsFloatManager::EllipseShapeInfo::Ellips
                                 bIsMoreThanEllipseBEnd) ? nscoord_MIN :
       iExpand + NSAppUnitsToIntPixels(
         XInterceptAtY(bInAppUnits, mRadii.width, mRadii.height),
         aAppUnitsPerDevPixel);
 
     // Set iMax in preparation for this block row.
     int32_t iMax = iIntercept;
 
-    for (int32_t i = 0; i < iSize; ++i) {
-      const int32_t index = i + b * iSize;
-      MOZ_ASSERT(index >= 0 && index < (iSize * bSize),
+    for (uint32_t i = 0; i < iSize; ++i) {
+      const uint32_t index = i + b * iSize;
+      MOZ_ASSERT(index < (iSize * bSize),
                  "Our distance field index should be in-bounds.");
 
       // Handle our three cases, in order.
       if (i < iExpand ||
           bIsInExpandedRegion) {
         // Case 1: Expanded reqion pixel.
         df[index] = MAX_MARGIN_5X;
-      } else if (i <= iIntercept) {
+      } else if ((int32_t)i <= iIntercept) {
         // Case 2: Pixel within the ellipse.
         df[index] = 0;
       } else {
         // Case 3: Other pixel.
 
         // Backward-looking neighborhood distance from target pixel X
         // with chamfer 5-7-11 looks like:
         //
@@ -888,31 +886,31 @@ nsFloatManager::EllipseShapeInfo::Ellips
         // +--+--+--+
         // |11| 7| 5|
         // +--+--+--+
         // |  | 5| X|
         // +--+--+--+
         //
         // X should be set to the minimum of the values of all of the numbered
         // neighbors summed with the value in that chamfer cell.
-        MOZ_ASSERT(index - iSize - 2 >= 0 &&
-                   index - (iSize * 2) - 1 >= 0,
+        MOZ_ASSERT(index - iSize - 2 < (iSize * bSize) &&
+                   index - (iSize * 2) - 1 < (iSize * bSize),
                    "Our distance field most extreme indices should be "
                    "in-bounds.");
 
         df[index] = std::min<dfType>(df[index - 1] + 5,
                     std::min<dfType>(df[index - iSize] + 5,
                     std::min<dfType>(df[index - iSize - 1] + 7,
                     std::min<dfType>(df[index - iSize - 2] + 11,
                                      df[index - (iSize * 2) - 1] + 11))));
 
         // Check the df value and see if it's less than or equal to the
         // usedMargin5X value.
         if (df[index] <= usedMargin5X) {
-          MOZ_ASSERT(iMax < i);
+          MOZ_ASSERT(iMax < (int32_t)i);
           iMax = i;
         }
       }
     }
 
     NS_WARNING_ASSERTION(bIsInExpandedRegion || iMax > nscoord_MIN,
                          "Once past the expanded region, we should always "
                          "find a pixel within the shape-margin distance for "
@@ -1488,51 +1486,54 @@ nsFloatManager::ImageShapeInfo::ImageSha
   const nsRect& aMarginRect,
   WritingMode aWM,
   const nsSize& aContainerSize)
 {
   MOZ_ASSERT(aShapeImageThreshold >=0.0 && aShapeImageThreshold <=1.0,
              "The computed value of shape-image-threshold is wrong!");
 
   const uint8_t threshold = NSToIntFloor(aShapeImageThreshold * 255);
-  const int32_t w = aImageSize.width;
-  const int32_t h = aImageSize.height;
+
+  MOZ_ASSERT(aImageSize.width >= 0 && aImageSize.height >= 0,
+             "Image size must be non-negative for our math to work.");
+  const uint32_t w = aImageSize.width;
+  const uint32_t h = aImageSize.height;
 
   if (aShapeMargin <= 0) {
     // Without a positive aShapeMargin, all we have to do is a
     // direct threshold comparison of the alpha pixels.
     // https://drafts.csswg.org/css-shapes-1/#valdef-shape-image-threshold-number
 
     // Scan the pixels in a double loop. For horizontal writing modes, we do
     // this row by row, from top to bottom. For vertical writing modes, we do
     // column by column, from left to right. We define the two loops
     // generically, then figure out the rows and cols within the inner loop.
-    const int32_t bSize = aWM.IsVertical() ? w : h;
-    const int32_t iSize = aWM.IsVertical() ? h : w;
-    for (int32_t b = 0; b < bSize; ++b) {
+    const uint32_t bSize = aWM.IsVertical() ? w : h;
+    const uint32_t iSize = aWM.IsVertical() ? h : w;
+    for (uint32_t b = 0; b < bSize; ++b) {
       // iMin and max store the start and end of the float area for the row
       // or column represented by this iteration of the outer loop.
       int32_t iMin = -1;
       int32_t iMax = -1;
 
-      for (int32_t i = 0; i < iSize; ++i) {
-        const int32_t col = aWM.IsVertical() ? b : i;
-        const int32_t row = aWM.IsVertical() ? i : b;
-        const int32_t index = col + row * aStride;
+      for (uint32_t i = 0; i < iSize; ++i) {
+        const uint32_t col = aWM.IsVertical() ? b : i;
+        const uint32_t row = aWM.IsVertical() ? i : b;
+        const uint32_t index = col + row * aStride;
 
         // Determine if the alpha pixel at this row and column has a value
         // greater than the threshold. If it does, update our iMin and iMax
         // values to track the edges of the float area for this row or column.
         // https://drafts.csswg.org/css-shapes-1/#valdef-shape-image-threshold-number
         const uint8_t alpha = aAlphaPixels[index];
         if (alpha > threshold) {
           if (iMin == -1) {
             iMin = i;
           }
-          MOZ_ASSERT(iMax < i);
+          MOZ_ASSERT(iMax < (int32_t)i);
           iMax = i;
         }
       }
 
       // At the end of a row or column; did we find something?
       if (iMin != -1) {
         // We need to supply an offset of the content rect top left, since
         // our col and row have been calculated from the content rect,
@@ -1570,99 +1571,110 @@ nsFloatManager::ImageShapeInfo::ImageSha
                                                 aAppUnitsPerDevPixel);
 
     // Allocate our distance field.  The distance field has to cover
     // the entire aMarginRect, since aShapeMargin could bleed into it,
     // beyond the content rect covered by aAlphaPixels. To make this work,
     // we calculate a dfOffset value which is the top left of the content
     // rect relative to the margin rect.
     nsPoint offsetPoint = aContentRect.TopLeft() - aMarginRect.TopLeft();
+    MOZ_ASSERT(offsetPoint.x >= 0 && offsetPoint.y >= 0,
+               "aContentRect should be within aMarginRect, which we need "
+               "for our math to make sense.");
     LayoutDeviceIntPoint dfOffset =
       LayoutDevicePixel::FromAppUnitsRounded(offsetPoint,
                                              aAppUnitsPerDevPixel);
 
     // Since our distance field is computed with a 5x5 neighborhood,
     // we need to expand our distance field by a further 4 pixels in
     // both axes, 2 on the leading edge and 2 on the trailing edge.
     // We call this edge area the "expanded region".
 
-    // Our expansion amounts need to be the same, and non-negative for our
-    // math to work, but we don't want to deal with casting them from
-    // unsigned ints.
-    static int32_t kExpansionPerSide = 2;
+    // Our expansion amounts need to be the same for our math to work.
+    static uint32_t kExpansionPerSide = 2;
     // Since dfOffset will be used in comparisons against expanded region
     // pixel values, it's convenient to add expansion amounts to dfOffset in
     // both axes, to simplify comparison math later.
     dfOffset.x += kExpansionPerSide;
     dfOffset.y += kExpansionPerSide;
 
     // In all these calculations, we purposely ignore aStride, because
     // we don't have to replicate the packing that we received in
     // aAlphaPixels. When we need to convert from df coordinates to
     // alpha coordinates, we do that with math based on row and col.
     const LayoutDeviceIntSize marginRectDevPixels =
       LayoutDevicePixel::FromAppUnitsRounded(aMarginRect.Size(),
                                              aAppUnitsPerDevPixel);
+
     // Clamp the size of our distance field sizes to prevent multiplication
     // overflow.
-    static const int32_t DF_SIDE_MAX =
+    static const uint32_t DF_SIDE_MAX =
       floor(sqrt((double)(std::numeric_limits<int32_t>::max())));
-    const int32_t wEx = std::min(marginRectDevPixels.width +
-                                 (kExpansionPerSide * 2), DF_SIDE_MAX);
-    const int32_t hEx = std::min(marginRectDevPixels.height +
-                                 (kExpansionPerSide * 2), DF_SIDE_MAX);
+
+    // Clamp the margin plus 2X the expansion values between expansion + 1
+    // and DF_SIDE_MAX. This ensures that the distance field allocation
+    // doesn't overflow during multiplication, and the reverse iteration
+    // doesn't underflow.
+    const uint32_t wEx = std::max(std::min(marginRectDevPixels.width +
+                                           (kExpansionPerSide * 2),
+                                           DF_SIDE_MAX),
+                                  kExpansionPerSide + 1);
+    const uint32_t hEx = std::max(std::min(marginRectDevPixels.height +
+                                           (kExpansionPerSide * 2),
+                                           DF_SIDE_MAX),
+                                  kExpansionPerSide + 1);
 
     // Since the margin-box size is CSS controlled, and large values will
     // generate large wEx and hEx values, we do a falliable allocation for
     // the distance field. If allocation fails, we early exit and layout will
     // be wrong, but we'll avoid aborting from OOM.
     auto df = MakeUniqueFallible<dfType[]>(wEx * hEx);
     if (!df) {
       // Without a distance field, we can't reason about the float area.
       return;
     }
 
-    const int32_t bSize = aWM.IsVertical() ? wEx : hEx;
-    const int32_t iSize = aWM.IsVertical() ? hEx : wEx;
+    const uint32_t bSize = aWM.IsVertical() ? wEx : hEx;
+    const uint32_t iSize = aWM.IsVertical() ? hEx : wEx;
 
     // First pass setting distance field, starting at top-left, three cases:
     // 1) Expanded region pixel: set to MAX_MARGIN_5X.
     // 2) Image pixel with alpha greater than threshold: set to 0.
     // 3) Other pixel: set to minimum backward-looking neighborhood
     //                 distance value, computed with 5-7-11 chamfer.
 
     // Scan the pixels in a double loop. For horizontal writing modes, we do
     // this row by row, from top to bottom. For vertical writing modes, we do
     // column by column, from left to right. We define the two loops
     // generically, then figure out the rows and cols within the inner loop.
-    for (int32_t b = 0; b < bSize; ++b) {
-      for (int32_t i = 0; i < iSize; ++i) {
-        const int32_t col = aWM.IsVertical() ? b : i;
-        const int32_t row = aWM.IsVertical() ? i : b;
-        const int32_t index = col + row * wEx;
-        MOZ_ASSERT(index >= 0 && index < (wEx * hEx),
+    for (uint32_t b = 0; b < bSize; ++b) {
+      for (uint32_t i = 0; i < iSize; ++i) {
+        const uint32_t col = aWM.IsVertical() ? b : i;
+        const uint32_t row = aWM.IsVertical() ? i : b;
+        const uint32_t index = col + row * wEx;
+        MOZ_ASSERT(index < (wEx * hEx),
                    "Our distance field index should be in-bounds.");
 
         // Handle our three cases, in order.
         if (col < kExpansionPerSide ||
             col >= wEx - kExpansionPerSide ||
             row < kExpansionPerSide ||
             row >= hEx - kExpansionPerSide) {
           // Case 1: Expanded pixel.
           df[index] = MAX_MARGIN_5X;
-        } else if (col >= dfOffset.x &&
-                   col < (dfOffset.x + w) &&
-                   row >= dfOffset.y &&
-                   row < (dfOffset.y + h) &&
+        } else if (col >= (uint32_t)dfOffset.x &&
+                   col < (uint32_t)(dfOffset.x + w) &&
+                   row >= (uint32_t)dfOffset.y &&
+                   row < (uint32_t)(dfOffset.y + h) &&
                    aAlphaPixels[col - dfOffset.x +
                                 (row - dfOffset.y) * aStride] > threshold) {
           // Case 2: Image pixel that is opaque.
-          DebugOnly<int32_t> alphaIndex = col - dfOffset.x +
-                                          (row - dfOffset.y) * aStride;
-          MOZ_ASSERT(alphaIndex >= 0 && alphaIndex < (aStride * h),
+          DebugOnly<uint32_t> alphaIndex = col - dfOffset.x +
+                                           (row - dfOffset.y) * aStride;
+          MOZ_ASSERT(alphaIndex < (aStride * h),
             "Our aAlphaPixels index should be in-bounds.");
 
           df[index] = 0;
         } else {
           // Case 3: Other pixel.
 
           // Backward-looking neighborhood distance from target pixel X
           // with chamfer 5-7-11 looks like:
@@ -1673,18 +1685,18 @@ nsFloatManager::ImageShapeInfo::ImageSha
           // |11| 7| 5| 7|11|
           // +--+--+--+--+--+
           // |  | 5| X|  |  |
           // +--+--+--+--+--+
           //
           // X should be set to the minimum of MAX_MARGIN_5X and the
           // values of all of the numbered neighbors summed with the
           // value in that chamfer cell.
-          MOZ_ASSERT(index - (wEx * 2) - 1 >= 0 &&
-                     index - wEx - 2 >= 0,
+          MOZ_ASSERT(index - (wEx * 2) - 1 < (iSize * bSize) &&
+                     index - wEx - 2 < (iSize * bSize),
                      "Our distance field most extreme indices should be "
                      "in-bounds.");
 
           df[index] = std::min<dfType>(MAX_MARGIN_5X,
                       std::min<dfType>(df[index - (wEx * 2) - 1] + 11,
                       std::min<dfType>(df[index - (wEx * 2) + 1] + 11,
                       std::min<dfType>(df[index - wEx - 2] + 11,
                       std::min<dfType>(df[index - wEx - 1] + 7,
@@ -1710,32 +1722,32 @@ nsFloatManager::ImageShapeInfo::ImageSha
     // with a 5-7-11 chamfer. We also check each df value against the
     // usedMargin5X threshold, and use that to set the iMin and iMax values
     // for the interval we'll create for that block axis value (b).
 
     // At the end of each row (or column in vertical writing modes),
     // if any of the other pixels had a value less than usedMargin5X,
     // we create an interval. Note: "bSize - kExpansionPerSide - 1" is the
     // index of the final row of pixels before the trailing expanded region.
-    for (int32_t b = bSize - kExpansionPerSide - 1;
+    for (uint32_t b = bSize - kExpansionPerSide - 1;
          b >= kExpansionPerSide; --b) {
       // iMin tracks the first df pixel and iMax the last df pixel whose
       // df[] value is less than usedMargin5X. Set iMin and iMax in
       // preparation for this row or column.
       int32_t iMin = iSize;
       int32_t iMax = -1;
 
       // Note: "iSize - kExpansionPerSide - 1" is the index of the final row
       // of pixels before the trailing expanded region.
-      for (int32_t i = iSize - kExpansionPerSide - 1;
+      for (uint32_t i = iSize - kExpansionPerSide - 1;
            i >= kExpansionPerSide; --i) {
-        const int32_t col = aWM.IsVertical() ? b : i;
-        const int32_t row = aWM.IsVertical() ? i : b;
-        const int32_t index = col + row * wEx;
-        MOZ_ASSERT(index >= 0 && index < (wEx * hEx),
+        const uint32_t col = aWM.IsVertical() ? b : i;
+        const uint32_t row = aWM.IsVertical() ? i : b;
+        const uint32_t index = col + row * wEx;
+        MOZ_ASSERT(index < (wEx * hEx),
                    "Our distance field index should be in-bounds.");
 
         // Only apply the chamfer calculation if the df value is not
         // already 0, since the chamfer can only reduce the value.
         if (df[index]) {
           // Forward-looking neighborhood distance from target pixel X
           // with chamfer 5-7-11 looks like:
           //
@@ -1767,17 +1779,17 @@ nsFloatManager::ImageShapeInfo::ImageSha
         }
 
         // Finally, we can check the df value and see if it's less than
         // or equal to the usedMargin5X value.
         if (df[index] <= usedMargin5X) {
           if (iMax == -1) {
             iMax = i;
           }
-          MOZ_ASSERT(iMin > i);
+          MOZ_ASSERT(iMin > (int32_t)i);
           iMin = i;
         }
       }
 
       if (iMax != -1) {
         // Our interval values, iMin, iMax, and b are all calculated from
         // the expanded region, which is based on the margin rect. To create
         // our interval, we have to subtract kExpansionPerSide from (iMin,