Tidy JBig2_Image.cppchromium/3511

Add checked/unchecked GetLine(y) methods and use them.
Introduce BIT_INDEX_TO_ALIGNED_BYTE() to de-mystify some shifting.
Move local declarations to spot of use.
Remove spurious Fill(), as we initialize to 0s.
Initialize members in header where possible.
Add unit tests.

Change-Id: I41ccb91b57320dbc790fd0f680f6d98571280343
Reviewed-on: https://pdfium-review.googlesource.com/39370
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>

3 files changed, 251 insertions, 64 deletions

diff --git a/core/fxcodec/jbig2/JBig2_Image.cpp b/core/fxcodec/jbig2/JBig2_Image.cpp
index 6c61c782f8..f66780aa16 100644
--- a/core/fxcodec/jbig2/JBig2_Image.cpp
+++ b/core/fxcodec/jbig2/JBig2_Image.cpp
@@ -16,6 +16,7 @@
 #include "core/fxcrt/fx_memory.h"
 #include "core/fxcrt/fx_safe_types.h"
 #include "third_party/base/ptr_util.h"
+#include "third_party/base/stl_util.h"
 
 #define JBIG2_GETDWORD(buf)                  \
   ((static_cast<uint32_t>((buf)[0]) << 24) | \
@@ -29,6 +30,8 @@
    (buf)[2] = static_cast<uint8_t>((val) >> 8),  \
    (buf)[3] = static_cast<uint8_t>((val) >> 0))
 
+#define BIT_INDEX_TO_ALIGNED_BYTE(x) (((x) >> 5) << 2)
+
 namespace {
 
 const int kMaxImagePixels = INT_MAX - 31;
@@ -36,8 +39,7 @@ const int kMaxImageBytes = kMaxImagePixels / 8;
 
 }  // namespace
 
-CJBig2_Image::CJBig2_Image(int32_t w, int32_t h)
-    : m_pData(nullptr), m_nWidth(0), m_nHeight(0), m_nStride(0) {
+CJBig2_Image::CJBig2_Image(int32_t w, int32_t h) {
   if (w <= 0 || h <= 0 || w > kMaxImagePixels)
     return;
 
@@ -52,9 +54,15 @@ CJBig2_Image::CJBig2_Image(int32_t w, int32_t h)
       FX_Alloc2D(uint8_t, m_nStride, m_nHeight)));
 }
 
-CJBig2_Image::CJBig2_Image(int32_t w, int32_t h, int32_t stride, uint8_t* pBuf)
-    : m_pData(nullptr), m_nWidth(0), m_nHeight(0), m_nStride(0) {
-  if (w < 0 || h < 0 || stride < 0 || stride > kMaxImageBytes)
+CJBig2_Image::CJBig2_Image(int32_t w,
+                           int32_t h,
+                           int32_t stride,
+                           uint8_t* pBuf) {
+  if (w < 0 || h < 0)
+    return;
+
+  // Stride must be word-aligned.
+  if (stride < 0 || stride > kMaxImageBytes || stride % 4 != 0)
     return;
 
   int32_t stride_pixels = 8 * stride;
@@ -68,8 +76,7 @@ CJBig2_Image::CJBig2_Image(int32_t w, int32_t h, int32_t stride, uint8_t* pBuf)
 }
 
 CJBig2_Image::CJBig2_Image(const CJBig2_Image& other)
-    : m_pData(nullptr),
-      m_nWidth(other.m_nWidth),
+    : m_nWidth(other.m_nWidth),
       m_nHeight(other.m_nHeight),
       m_nStride(other.m_nStride) {
   if (other.m_pData) {
@@ -117,6 +124,10 @@ void CJBig2_Image::SetPixel(int32_t x, int32_t y, int v) {
     data()[m] &= ~n;
 }
 
+uint8_t* CJBig2_Image::GetLine(int32_t y) const {
+  return (y >= 0 && y < m_nHeight) ? GetLineUnsafe(y) : nullptr;
+}
+
 void CJBig2_Image::CopyLine(int32_t hTo, int32_t hFrom) {
   if (!m_pData)
     return;
@@ -170,60 +181,40 @@ std::unique_ptr<CJBig2_Image> CJBig2_Image::SubImage(int32_t x,
                                                      int32_t y,
                                                      int32_t w,
                                                      int32_t h) {
-  int32_t m;
-  int32_t n;
-  int32_t j;
-  uint8_t* pLineSrc;
-  uint8_t* pLineDst;
-  uint32_t wTmp;
-  uint8_t* pSrc;
-  uint8_t* pSrcEnd;
-  uint8_t* pDst;
-  uint8_t* pDstEnd;
-  if (w == 0 || h == 0)
-    return nullptr;
-
   auto pImage = pdfium::MakeUnique<CJBig2_Image>(w, h);
-  if (!m_pData) {
-    pImage->Fill(0);
+  if (!pImage->data() || !m_pData)
     return pImage;
-  }
-  if (!pImage->m_pData)
+
+  if (x < 0 || x >= m_nWidth || y < 0 || y >= m_nHeight)
     return pImage;
 
-  pLineSrc = data() + m_nStride * y;
-  pLineDst = pImage->data();
-  m = (x >> 5) << 2;
-  n = x & 31;
+  int32_t m = BIT_INDEX_TO_ALIGNED_BYTE(x);
+  int32_t n = x & 31;
+  int32_t bytes_to_copy = std::min(pImage->m_nStride, m_nStride - m);
+  int32_t lines_to_copy = std::min(pImage->m_nHeight, m_nHeight - y);
+
+  // Fast case when DWORD-aligned.
   if (n == 0) {
-    for (j = 0; j < h; j++) {
-      pSrc = pLineSrc + m;
-      pSrcEnd = pLineSrc + m_nStride;
-      pDst = pLineDst;
-      pDstEnd = pLineDst + pImage->m_nStride;
-      for (; pDst < pDstEnd; pSrc += 4, pDst += 4) {
-        *((uint32_t*)pDst) = *((uint32_t*)pSrc);
-      }
-      pLineSrc += m_nStride;
-      pLineDst += pImage->m_nStride;
+    for (int32_t j = 0; j < lines_to_copy; j++) {
+      const uint8_t* pLineSrc = GetLineUnsafe(y + j);
+      uint8_t* pLineDst = pImage->GetLineUnsafe(j);
+      memcpy(pLineDst, pLineSrc + m, bytes_to_copy);
     }
-  } else {
-    for (j = 0; j < h; j++) {
-      pSrc = pLineSrc + m;
-      pSrcEnd = pLineSrc + m_nStride;
-      pDst = pLineDst;
-      pDstEnd = pLineDst + pImage->m_nStride;
-      for (; pDst < pDstEnd; pSrc += 4, pDst += 4) {
-        if (pSrc + 4 < pSrcEnd) {
-          wTmp = (JBIG2_GETDWORD(pSrc) << n) |
-                 (JBIG2_GETDWORD(pSrc + 4) >> (32 - n));
-        } else {
-          wTmp = JBIG2_GETDWORD(pSrc) << n;
-        }
-        JBIG2_PUTDWORD(pDst, wTmp);
-      }
-      pLineSrc += m_nStride;
-      pLineDst += pImage->m_nStride;
+    return pImage;
+  }
+
+  // Normal slow case.
+  for (int32_t j = 0; j < lines_to_copy; j++) {
+    const uint8_t* pLineSrc = GetLineUnsafe(y + j);
+    uint8_t* pLineDst = pImage->GetLineUnsafe(j);
+    const uint8_t* pSrc = pLineSrc + m;
+    const uint8_t* pSrcEnd = pLineSrc + m_nStride;
+    uint8_t* pDstEnd = pLineDst + bytes_to_copy;
+    for (uint8_t *pDst = pLineDst; pDst < pDstEnd; pSrc += 4, pDst += 4) {
+      uint32_t wTmp = JBIG2_GETDWORD(pSrc) << n;
+      if (pSrc + 4 < pSrcEnd)
+        wTmp |= (JBIG2_GETDWORD(pSrc + 4) >> (32 - n));
+      JBIG2_PUTDWORD(pDst, wTmp);
     }
   }
   return pImage;
@@ -382,7 +373,6 @@ bool CJBig2_Image::ComposeToOpt2(CJBig2_Image* pDst,
   } else {
     uint8_t* sp = nullptr;
     uint8_t* dp = nullptr;
-
     if (s1 > d1) {
       uint32_t shift1 = s1 - d1;
       uint32_t shift2 = 32 - shift1;
diff --git a/core/fxcodec/jbig2/JBig2_Image.h b/core/fxcodec/jbig2/JBig2_Image.h
index 3cf28b0643..b1b1489506 100644
--- a/core/fxcodec/jbig2/JBig2_Image.h
+++ b/core/fxcodec/jbig2/JBig2_Image.h
@@ -34,11 +34,14 @@ class CJBig2_Image {
   int32_t width() const { return m_nWidth; }
   int32_t height() const { return m_nHeight; }
   int32_t stride() const { return m_nStride; }
+
   uint8_t* data() const { return m_pData.Get(); }
 
   int GetPixel(int32_t x, int32_t y) const;
   void SetPixel(int32_t x, int32_t y, int bVal);
 
+  uint8_t* GetLine(int32_t y) const;
+  uint8_t* GetLineUnsafe(int32_t y) const { return data() + y * m_nStride; }
   void CopyLine(int32_t hTo, int32_t hFrom);
   void Fill(bool v);
 
@@ -74,9 +77,9 @@ class CJBig2_Image {
                              const FX_RECT& rtSrc);
 
   MaybeOwned<uint8_t, FxFreeDeleter> m_pData;
-  int32_t m_nWidth;   // 1-bit pixels
-  int32_t m_nHeight;  // lines
-  int32_t m_nStride;  // bytes
+  int32_t m_nWidth = 0;   // 1-bit pixels
+  int32_t m_nHeight = 0;  // lines
+  int32_t m_nStride = 0;  // bytes, must be multiple of 4.
 };
 
 #endif  // CORE_FXCODEC_JBIG2_JBIG2_IMAGE_H_
diff --git a/core/fxcodec/jbig2/JBig2_Image_unittest.cpp b/core/fxcodec/jbig2/JBig2_Image_unittest.cpp
index bebfb6b20d..2b4d897cea 100644
--- a/core/fxcodec/jbig2/JBig2_Image_unittest.cpp
+++ b/core/fxcodec/jbig2/JBig2_Image_unittest.cpp
@@ -8,26 +8,75 @@
 
 #include "core/fxcodec/jbig2/JBig2_Image.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/base/ptr_util.h"
 
 namespace {
 
 const int32_t kWidthPixels = 80;
 const int32_t kWidthBytes = 10;
-const int32_t kStrideBytes = kWidthBytes + 1;  // For testing stride != width.
+const int32_t kStrideBytes = kWidthBytes + 2;  // For testing stride != width.
 const int32_t kHeightLines = 20;
 const int32_t kLargerHeightLines = 100;
 const int32_t kTooLargeHeightLines = 40000000;
 
+void CheckImageEq(CJBig2_Image* img1, CJBig2_Image* img2, int line) {
+  EXPECT_EQ(img1->width(), img2->width());
+  EXPECT_EQ(img1->height(), img2->height());
+  for (int32_t y = 0; y < img1->height(); ++y) {
+    for (int32_t x = 0; x < img1->width(); ++x) {
+      EXPECT_EQ(img1->GetPixel(x, y), img2->GetPixel(x, y))
+          << " at " << x << " " << y << " actual line " << line;
+    }
+  }
+}
+
 }  // namespace
 
+TEST(fxcodec, EmptyImage) {
+  CJBig2_Image empty(0, 0);
+  EXPECT_EQ(empty.width(), 0);
+  EXPECT_EQ(empty.height(), 0);
+
+  // Out-of-bounds SetPixel() is silent no-op.
+  empty.SetPixel(0, 0, true);
+  empty.SetPixel(1, 1, true);
+
+  // Out-of-bounds GetPixel returns 0.
+  EXPECT_EQ(empty.GetPixel(0, 0), 0);
+  EXPECT_EQ(empty.GetPixel(1, 1), 0);
+
+  // Out-of-bounds GetLine() returns null.
+  EXPECT_EQ(empty.GetLine(0), nullptr);
+  EXPECT_EQ(empty.GetLine(1), nullptr);
+}
+
 TEST(fxcodec, JBig2ImageCreate) {
   CJBig2_Image img(kWidthPixels, kHeightLines);
-  img.SetPixel(0, 0, true);
-  img.SetPixel(kWidthPixels - 1, kHeightLines - 1, false);
   EXPECT_EQ(kWidthPixels, img.width());
   EXPECT_EQ(kHeightLines, img.height());
-  EXPECT_TRUE(img.GetPixel(0, 0));
-  EXPECT_FALSE(img.GetPixel(kWidthPixels - 1, kHeightLines - 1));
+  EXPECT_EQ(0, img.GetPixel(0, 0));
+  EXPECT_EQ(0, img.GetLine(0)[0]);
+  EXPECT_EQ(0, img.GetPixel(kWidthPixels - 1, kHeightLines - 1));
+  EXPECT_EQ(0, img.GetLine(kHeightLines - 1)[kWidthBytes - 1]);
+
+  img.SetPixel(0, 0, true);
+  img.SetPixel(kWidthPixels - 1, kHeightLines - 1, true);
+  EXPECT_EQ(1, img.GetPixel(0, 0));
+  EXPECT_EQ(1, img.GetPixel(kWidthPixels - 1, kHeightLines - 1));
+  EXPECT_EQ(0x80, img.GetLine(0)[0]);
+  EXPECT_EQ(0x01, img.GetLine(kHeightLines - 1)[kWidthBytes - 1]);
+
+  // Out-of-bounds SetPixel() is silent no-op.
+  img.SetPixel(-1, 1, true);
+  img.SetPixel(kWidthPixels, kHeightLines, true);
+
+  // Out-of-bounds GetPixel returns 0.
+  EXPECT_EQ(0, img.GetPixel(-1, -1));
+  EXPECT_EQ(0, img.GetPixel(kWidthPixels, kHeightLines));
+
+  // Out-of-bounds GetLine() returns null.
+  EXPECT_EQ(nullptr, img.GetLine(-1));
+  EXPECT_EQ(nullptr, img.GetLine(kHeightLines));
 }
 
 TEST(fxcodec, JBig2ImageCreateTooBig) {
@@ -56,6 +105,14 @@ TEST(fxcodec, JBig2ImageCreateExternalTooBig) {
   EXPECT_EQ(nullptr, img.data());
 }
 
+TEST(fxcodec, JBig2ImageCreateExternalBadStride) {
+  uint8_t buf[kHeightLines * kStrideBytes];
+  CJBig2_Image img(kWidthPixels, kTooLargeHeightLines, kStrideBytes - 1, buf);
+  EXPECT_EQ(0, img.width());
+  EXPECT_EQ(0, img.height());
+  EXPECT_EQ(nullptr, img.data());
+}
+
 TEST(fxcodec, JBig2ImageExpand) {
   CJBig2_Image img(kWidthPixels, kHeightLines);
   img.SetPixel(0, 0, true);
@@ -103,3 +160,140 @@ TEST(fxcodec, JBig2ImageExpandExternalTooBig) {
   EXPECT_TRUE(img.GetPixel(0, 0));
   EXPECT_FALSE(img.GetPixel(kWidthPixels - 1, kHeightLines - 1));
 }
+
+TEST(fxcodec, JBig2EmptyImage) {
+  auto empty = pdfium::MakeUnique<CJBig2_Image>(0, 0);
+
+  // Empty subimage.
+  auto sub1 = empty->SubImage(0, 0, 0, 0);
+  EXPECT_EQ(sub1->width(), 0);
+  EXPECT_EQ(sub1->height(), 0);
+
+  // Larger dimensions are zero-padded.
+  auto sub2 = empty->SubImage(0, 0, 1, 1);
+  EXPECT_EQ(1, sub2->width());
+  EXPECT_EQ(1, sub2->height());
+  EXPECT_EQ(0, sub2->GetPixel(0, 0));
+
+  // Bad dimensions give an empty image.
+  sub2 = empty->SubImage(0, 0, -1, -1);
+  EXPECT_EQ(sub2->width(), 0);
+  EXPECT_EQ(sub2->height(), 0);
+
+  // Bad offsets zero pad the image.
+  auto sub3 = empty->SubImage(-1, -1, 2, 2);
+  EXPECT_EQ(sub3->width(), 2);
+  EXPECT_EQ(sub3->height(), 2);
+
+  // Bad dimensions and bad offsets give an empty image.
+  sub3 = empty->SubImage(-1, -1, -100, -100);
+  EXPECT_EQ(sub3->width(), 0);
+  EXPECT_EQ(sub3->height(), 0);
+}
+
+TEST(fxcodec, JBig2SubImage) {
+  // 1-px wide rectangle in image.
+  uint8_t pattern[5][8] = {
+      {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x01, 0xff, 0x80, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x01, 0x00, 0x80, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x01, 0xff, 0x80, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+  };
+
+  // 1-px wide rectangle in image, offset 2 in x.
+  uint8_t pattern20[5][8] = {
+      {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x07, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x04, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x07, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+  };
+
+  // 1-px wide rectangle in image, offset 2 in x and y, padded.
+  uint8_t pattern22[5][8] = {
+      {0x00, 0x04, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x07, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+      {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+  };
+
+  // Image size a nice clean power of two.
+  auto img32 = pdfium::MakeUnique<CJBig2_Image>(
+      32, 5, 8, reinterpret_cast<uint8_t*>(pattern));
+
+  // Image size not a nice clean value.
+  auto img37 = pdfium::MakeUnique<CJBig2_Image>(
+      37, 5, 8, reinterpret_cast<uint8_t*>(pattern));
+
+  // Expected results to check against.
+  auto expected20 = pdfium::MakeUnique<CJBig2_Image>(
+      30, 5, 8, reinterpret_cast<uint8_t*>(pattern20));
+
+  auto expected22 = pdfium::MakeUnique<CJBig2_Image>(
+      30, 5, 8, reinterpret_cast<uint8_t*>(pattern22));
+
+  auto expected_zeros = pdfium::MakeUnique<CJBig2_Image>(32, 5);
+
+  // Empty subimage.
+  auto sub = img32->SubImage(0, 0, 0, 0);
+  EXPECT_EQ(sub->width(), 0);
+  EXPECT_EQ(sub->height(), 0);
+
+  // Full sub-image.
+  sub = img32->SubImage(0, 0, 32, 5);
+  EXPECT_EQ(sub->width(), 32);
+  EXPECT_EQ(sub->height(), 5);
+  CheckImageEq(img32.get(), sub.get(), __LINE__);
+
+  sub = img37->SubImage(0, 0, 32, 5);
+  EXPECT_EQ(sub->width(), 32);
+  EXPECT_EQ(sub->height(), 5);
+  CheckImageEq(img32.get(), sub.get(), __LINE__);
+
+  // Actual bit manipulations.
+  sub = img32->SubImage(2, 0, 30, 5);
+  CheckImageEq(expected20.get(), sub.get(), __LINE__);
+
+  sub = img37->SubImage(2, 2, 30, 5);
+  CheckImageEq(expected22.get(), sub.get(), __LINE__);
+
+  // Aligned Sub-image including cruft in stride beyond width.
+  sub = img37->SubImage(32, 0, 32, 5);
+  CheckImageEq(expected_zeros.get(), sub.get(), __LINE__);
+
+  // Sub-image waaaaay beyond width.
+  sub = img37->SubImage(2000, 0, 32, 5);
+  CheckImageEq(expected_zeros.get(), sub.get(), __LINE__);
+
+  // Sub-image waaaaay beyond height.
+  sub = img37->SubImage(0, 2000, 32, 5);
+  CheckImageEq(expected_zeros.get(), sub.get(), __LINE__);
+
+  // Sub-image with negative x offset.
+  sub = img37->SubImage(-1, 0, 32, 5);
+  CheckImageEq(expected_zeros.get(), sub.get(), __LINE__);
+
+  // Sub-image with negative y offset.
+  sub = img37->SubImage(0, -1, 32, 5);
+  CheckImageEq(expected_zeros.get(), sub.get(), __LINE__);
+
+  // Sub-image with negative width.
+  sub = img37->SubImage(-1, 0, 32, 5);
+  CheckImageEq(expected_zeros.get(), sub.get(), __LINE__);
+
+  // Sub-image with negative height.
+  sub = img37->SubImage(0, -1, 32, 5);
+  CheckImageEq(expected_zeros.get(), sub.get(), __LINE__);
+
+  // Sub-image wider than original.
+  sub = img37->SubImage(0, 0, 128, 5);
+  EXPECT_EQ(128, sub->width());
+  EXPECT_EQ(5, sub->height());
+
+  // Sub-image higher than original.
+  sub = img37->SubImage(0, 0, 32, 40);
+  EXPECT_EQ(32, sub->width());
+  EXPECT_EQ(40, sub->height());
+}