commit 58382eb575806ce84f98dd61a3b66ffefe38ef57
author Lei Zhang <thestig@chromium.org> Fri Sep 11 00:01:43 2020 +0000
committer Chromium commit bot <commit-bot@chromium.org> Fri Sep 11 00:01:43 2020 +0000
tree 8a946f8827f80c327a44197b2e2bb416e0a2cfdd
parent 874b62712a9aff78584548c6a2e66ffaaa011519

Use CheckedNumerics to calculate pitch in CalculatePitchAndSize().

In CFX_DIBitmap::CalculatePitchAndSize(), instead of manually avoiding
integer overflow when calculating the pitch, use CheckedNumerics instead
by instantiating a FX_SAFE_UINT32.

This raises the limit for the maximum possible pitch value in some
cases, such that the manual integer overflow check for the size value is
the limiting factor for successful calculations.

Bug: chromium:1124660
Change-Id: I83ec7fe8eff69cc4101dfecf4d8360bd9dea39f2
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/73397
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>

core/fxge/dib/cfx_dibitmap.cpp

diff --git a/core/fxge/dib/cfx_dibitmap.cpp b/core/fxge/dib/cfx_dibitmap.cpp
index 951bf7b..eeeb68c 100644
--- a/core/fxge/dib/cfx_dibitmap.cpp
+++ b/core/fxge/dib/cfx_dibitmap.cpp
@@ -859,12 +859,19 @@
   if (!bpp)
     return pdfium::nullopt;
 
-  if ((INT_MAX - 31) / width < bpp)
-    return pdfium::nullopt;
-
   uint32_t actual_pitch = pitch;
-  if (actual_pitch == 0)
-    actual_pitch = static_cast<uint32_t>((width * bpp + 31) / 32 * 4);
+  if (actual_pitch == 0) {
+    FX_SAFE_UINT32 safe_pitch = width;
+    safe_pitch *= bpp;
+    safe_pitch += 31;
+    // Note: This is not the same as /8 due to truncation.
+    safe_pitch /= 32;
+    safe_pitch *= 4;
+    if (!safe_pitch.IsValid())
+      return pdfium::nullopt;
+
+    actual_pitch = safe_pitch.ValueOrDie();
+  }
 
   if ((1 << 30) / actual_pitch < static_cast<uint32_t>(height))
     return pdfium::nullopt;

core/fxge/dib/cfx_dibitmap_unittest.cpp

diff --git a/core/fxge/dib/cfx_dibitmap_unittest.cpp b/core/fxge/dib/cfx_dibitmap_unittest.cpp
index 8563bf9..507fdbe 100644
--- a/core/fxge/dib/cfx_dibitmap_unittest.cpp
+++ b/core/fxge/dib/cfx_dibitmap_unittest.cpp
@@ -68,8 +68,8 @@
       CFX_DIBitmap::CalculatePitchAndSize(4194304, 1024, FXDIB_8bppRgb, 0));
 
   // Overflow cases with provided pitch.
-  EXPECT_FALSE(CFX_DIBitmap::CalculatePitchAndSize(1073741824, 1, FXDIB_Argb,
-                                                   1073741824));
+  EXPECT_FALSE(CFX_DIBitmap::CalculatePitchAndSize(1073741825, 1, FXDIB_Argb,
+                                                   1073741825));
   EXPECT_FALSE(
       CFX_DIBitmap::CalculatePitchAndSize(1048576, 1024, FXDIB_Argb, 4194304));
   EXPECT_FALSE(CFX_DIBitmap::CalculatePitchAndSize(4194304, 1024, FXDIB_8bppRgb,
@@ -79,12 +79,12 @@
 TEST(CFX_DIBitmap, CalculatePitchAndSizeBoundary) {
   // Test boundary condition for pitch overflow.
   Optional<CFX_DIBitmap::PitchAndSize> result =
-      CFX_DIBitmap::CalculatePitchAndSize(268435452, 4, FXDIB_8bppRgb, 0);
+      CFX_DIBitmap::CalculatePitchAndSize(268435456, 4, FXDIB_8bppRgb, 0);
   ASSERT_TRUE(result);
-  EXPECT_EQ(268435452u, result.value().pitch);
-  EXPECT_EQ(1073741808u, result.value().size);
+  EXPECT_EQ(268435456u, result.value().pitch);
+  EXPECT_EQ(1073741824u, result.value().size);
   EXPECT_FALSE(
-      CFX_DIBitmap::CalculatePitchAndSize(268435453, 4, FXDIB_8bppRgb, 0));
+      CFX_DIBitmap::CalculatePitchAndSize(268435457, 4, FXDIB_8bppRgb, 0));
 
   // Test boundary condition for size overflow.
   result = CFX_DIBitmap::CalculatePitchAndSize(17043520, 63, FXDIB_8bppRgb, 0);