(Reland) Fix extraction of colour components in CPDF_DIBSource::DownSampleScanline32Bit

Previously, if |m_bpc| was < 8 (e.g. 4), this function may still try to
access the source components as if |m_bpc| == 8. Even when it fell into
the codepath that tried to do the right thing in this case, it was
wrong.

BUG=554151
R=tsepez@chromium.org, thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/9b99615806e358fdb396d1cb162ee2e69c2a20ec

Review URL: https://codereview.chromium.org/1433423002 .

6 files changed, 189 insertions, 30 deletions

diff --git a/BUILD.gn b/BUILD.gn
index a9d58a174d..bc68804d11 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -153,7 +153,7 @@ source_set("test_support") {
     "testing/fx_string_testhelpers.h",
     "testing/test_support.cpp",
     "testing/test_support.h",
-    "testing/utils/path_service.cpp",
+    'testing/utils/path_service.cpp',
   ]
   deps = [
     "//testing/gmock",
@@ -748,6 +748,7 @@ test("pdfium_embeddertests") {
     "core/src/fpdfapi/fpdf_page/fpdf_page_func_embeddertest.cpp",
     "core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp",
     "core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp",
+    "core/src/fpdfapi/fpdf_render/fpdf_render_load_image_embeddertest.cpp",
     "core/src/fpdfapi/fpdf_render/fpdf_render_pattern_embeddertest.cpp",
     "fpdfsdk/src/fpdf_dataavail_embeddertest.cpp",
     "fpdfsdk/src/fpdfdoc_embeddertest.cpp",
diff --git a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
index f6f41d9de1..bdd5e9aa9c 100644
--- a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
+++ b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
@@ -19,7 +19,9 @@
 
 namespace {
 
-unsigned int _GetBits8(const uint8_t* pData, int bitpos, int nbits) {
+unsigned int GetBits8(const uint8_t* pData, uint64_t bitpos, size_t nbits) {
+  ASSERT(nbits == 1 || nbits == 2 || nbits == 4 || nbits == 8 || nbits == 16);
+  ASSERT((bitpos & (nbits - 1)) == 0);
   unsigned int byte = pData[bitpos / 8];
   if (nbits == 8) {
     return byte;
@@ -993,11 +995,11 @@ void CPDF_DIBSource::TranslateScanline24bpp(uint8_t* dest_scan,
           int src_bit_pos = 0;
           int dest_byte_pos = 0;
           for (int column = 0; column < m_Width; column++) {
-            int R = _GetBits8(src_scan, src_bit_pos, m_bpc);
+            int R = GetBits8(src_scan, src_bit_pos, m_bpc);
             src_bit_pos += m_bpc;
-            int G = _GetBits8(src_scan, src_bit_pos, m_bpc);
+            int G = GetBits8(src_scan, src_bit_pos, m_bpc);
             src_bit_pos += m_bpc;
-            int B = _GetBits8(src_scan, src_bit_pos, m_bpc);
+            int B = GetBits8(src_scan, src_bit_pos, m_bpc);
             src_bit_pos += m_bpc;
             R = NORMALCOLOR_MAX(R, max_data);
             G = NORMALCOLOR_MAX(G, max_data);
@@ -1051,7 +1053,7 @@ void CPDF_DIBSource::TranslateScanline24bpp(uint8_t* dest_scan,
     int dest_byte_pos = 0;
     for (int column = 0; column < m_Width; column++) {
       for (FX_DWORD color = 0; color < m_nComponents; color++) {
-        int data = _GetBits8(src_scan, src_bit_pos, m_bpc);
+        int data = GetBits8(src_scan, src_bit_pos, m_bpc);
         color_values[color] = m_pCompData[color].m_DecodeMin +
                               m_pCompData[color].m_DecodeStep * data;
         src_bit_pos += m_bpc;
@@ -1146,7 +1148,7 @@ const uint8_t* CPDF_DIBSource::GetScanline(int line) const {
       for (int col = 0; col < m_Width; col++) {
         int color_index = 0;
         for (FX_DWORD color = 0; color < m_nComponents; color++) {
-          int data = _GetBits8(pSrcLine, src_bit_pos, m_bpc);
+          int data = GetBits8(pSrcLine, src_bit_pos, m_bpc);
           color_index |= data << (color * m_bpc);
           src_bit_pos += m_bpc;
         }
@@ -1368,7 +1370,7 @@ void CPDF_DIBSource::DownSampleScanline8Bit(int orig_Bpp,
     for (FX_DWORD col = 0; col < src_width; col++) {
       int color_index = 0;
       for (FX_DWORD color = 0; color < m_nComponents; color++) {
-        int data = _GetBits8(pSrcLine, src_bit_pos, m_bpc);
+        int data = GetBits8(pSrcLine, src_bit_pos, m_bpc);
         color_index |= data << (color * m_bpc);
         src_bit_pos += m_bpc;
       }
@@ -1431,59 +1433,66 @@ void CPDF_DIBSource::DownSampleScanline32Bit(int orig_Bpp,
                                              int clip_width) const {
   int last_src_x = -1;
   FX_ARGB last_argb = FXARGB_MAKE(0xFF, 0xFF, 0xFF, 0xFF);
-  FX_FLOAT orig_Not8Bpp = (FX_FLOAT)m_bpc * (FX_FLOAT)m_nComponents / 8.0f;
   FX_FLOAT unit_To8Bpc = 255.0f / ((1 << m_bpc) - 1);
   for (int i = 0; i < clip_width; i++) {
     int dest_x = clip_left + i;
     FX_DWORD src_x = (bFlipX ? (dest_width - dest_x - 1) : dest_x) *
                      (int64_t)src_width / dest_width;
     src_x %= src_width;
+
+    // No need to check for 32-bit overflow, as |src_x| is bounded by
+    // |src_width| and DownSampleScanline already checked for overflow with the
+    // pitch calculation.
     const uint8_t* pSrcPixel = nullptr;
+    size_t bit_offset = 0;
     if (m_bpc % 8 == 0) {
       pSrcPixel = pSrcLine + src_x * orig_Bpp;
     } else {
-      pSrcPixel = pSrcLine + (int)(src_x * orig_Not8Bpp);
+      size_t num_bits = src_x * m_bpc * m_nComponents;
+      pSrcPixel = pSrcLine + num_bits / 8;
+      bit_offset = num_bits % 8;
     }
+
     uint8_t* pDestPixel = dest_scan + i * dest_Bpp;
     FX_ARGB argb;
     if (src_x == last_src_x) {
       argb = last_argb;
     } else {
+      CFX_FixedBufGrow<uint8_t, 128> extracted_components(m_nComponents);
+      if (m_bpc % 8 != 0) {
+        uint64_t src_bit_pos = bit_offset;
+        for (FX_DWORD j = 0; j < m_nComponents; ++j) {
+          extracted_components[j] = static_cast<uint8_t>(
+              GetBits8(pSrcPixel, src_bit_pos, m_bpc) * unit_To8Bpc);
+          src_bit_pos += m_bpc;
+        }
+        pSrcPixel = extracted_components;
+      }
+
       if (m_pColorSpace) {
-        CFX_FixedBufGrow<uint8_t, 128> temp(orig_Bpp);
         uint8_t color[4];
         const FX_BOOL bTransMask = TransMask();
         if (m_bDefaultDecode) {
-          if (m_bpc < 8) {
-            int src_bit_pos = 0;
-            if (src_x % 2) {
-              src_bit_pos = 4;
-            }
-            for (FX_DWORD j = 0; j < m_nComponents; ++j) {
-              temp[j] = (uint8_t)(_GetBits8(pSrcPixel, src_bit_pos, m_bpc) *
-                                  unit_To8Bpc);
-              src_bit_pos += m_bpc;
-            }
-            m_pColorSpace->TranslateImageLine(color, temp, 1, 0, 0, bTransMask);
-          } else {
-            m_pColorSpace->TranslateImageLine(color, pSrcPixel, 1, 0, 0,
-                                              bTransMask);
-          }
+          m_pColorSpace->TranslateImageLine(color, pSrcPixel, 1, 0, 0,
+                                            bTransMask);
         } else {
           for (FX_DWORD j = 0; j < m_nComponents; ++j) {
+            FX_FLOAT component_value =
+                static_cast<FX_FLOAT>(extracted_components[j]);
             int color_value =
                 (int)((m_pCompData[j].m_DecodeMin +
-                       m_pCompData[j].m_DecodeStep * (FX_FLOAT)pSrcPixel[j]) *
+                       m_pCompData[j].m_DecodeStep * component_value) *
                           255.0f +
                       0.5f);
-            temp[j] =
+            extracted_components[j] =
                 color_value > 255 ? 255 : (color_value < 0 ? 0 : color_value);
           }
-          m_pColorSpace->TranslateImageLine(color, temp, 1, 0, 0, bTransMask);
+          m_pColorSpace->TranslateImageLine(color, extracted_components, 1, 0,
+                                            0, bTransMask);
         }
         argb = FXARGB_MAKE(0xFF, color[2], color[1], color[0]);
       } else {
-        argb = FXARGB_MAKE(0xFf, pSrcPixel[2], pSrcPixel[1], pSrcPixel[0]);
+        argb = FXARGB_MAKE(0xFF, pSrcPixel[2], pSrcPixel[1], pSrcPixel[0]);
       }
       if (m_bColorKey) {
         int alpha = 0xFF;
diff --git a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage_embeddertest.cpp b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage_embeddertest.cpp
new file mode 100644
index 0000000000..1633249d0d
--- /dev/null
+++ b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage_embeddertest.cpp
@@ -0,0 +1,19 @@
+// Copyright 2015 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "testing/embedder_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class FPDFRenderLoadImageEmbeddertest : public EmbedderTest {};
+
+TEST_F(FPDFRenderLoadImageEmbeddertest, Bug_554151) {
+  // Test scanline downsampling with a BitsPerComponent of 4.
+  // Should not crash.
+  EXPECT_TRUE(OpenDocument("bug_554151.pdf"));
+  FPDF_PAGE page = LoadPage(0);
+  EXPECT_NE(nullptr, page);
+  FPDF_BITMAP bitmap = RenderPage(page);
+  FPDFBitmap_Destroy(bitmap);
+  UnloadPage(page);
+}
diff --git a/pdfium.gyp b/pdfium.gyp
index 33c4e19624..0bf0742f0e 100644
--- a/pdfium.gyp
+++ b/pdfium.gyp
@@ -746,6 +746,7 @@
         'core/src/fpdfapi/fpdf_page/fpdf_page_func_embeddertest.cpp',
         'core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp',
         'core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_embeddertest.cpp',
+        'core/src/fpdfapi/fpdf_render/fpdf_render_loadimage_embeddertest.cpp',
         'core/src/fpdfapi/fpdf_render/fpdf_render_pattern_embeddertest.cpp',
         'fpdfsdk/src/fpdf_dataavail_embeddertest.cpp',
         'fpdfsdk/src/fpdfdoc_embeddertest.cpp',
diff --git a/testing/resources/bug_554151.in b/testing/resources/bug_554151.in
new file mode 100644
index 0000000000..3087159f51
--- /dev/null
+++ b/testing/resources/bug_554151.in
@@ -0,0 +1,59 @@
+{{header}}
+
+{{object 1 0}}
+<<
+  /Pages 2 0 R
+>>
+endobj
+
+{{object 2 0}}
+<<
+  /Kids [ 3 0 R ]
+>>
+endobj
+
+{{object 3 0}}
+<<
+  /Contents 6 0 R
+  /Resources 5 0 R
+>>
+endobj
+
+{{object 6 0}}
+<<>>
+stream
+612 0.0 0.0 104000 0 0 cm
+/XBad Do
+endstream
+endobj
+
+{{object 5 0}}
+<<
+  /XObject <<
+    /XBad 7 0 R
+  >>
+>>
+endobj
+
+{{object 7 0}}
+<<
+  /Type /XObject
+  /Subtype /Image
+  /Width 612
+  /Height 104000
+  /ColorSpace /DeviceRGB
+  /BitsPerComponent 4
+  /Length 0
+  /Decode [1.0]
+>>
+stream
+FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
+endobj
+
+{{xref}}
+trailer <<
+  /Root 1 0 R
+  /Size 8
+>>
+{{startxref}}
+%%EOF
diff --git a/testing/resources/bug_554151.pdf b/testing/resources/bug_554151.pdf
new file mode 100644
index 0000000000..fa770a7b37
--- /dev/null
+++ b/testing/resources/bug_554151.pdf
@@ -0,0 +1,70 @@
+%PDF-1.7
+% ò¤ô
+
+1 0 obj
+<<
+  /Pages 2 0 R
+>>
+endobj
+
+2 0 obj
+<<
+  /Kids [ 3 0 R ]
+>>
+endobj
+
+3 0 obj
+<<
+  /Contents 6 0 R
+  /Resources 5 0 R
+>>
+endobj
+
+6 0 obj
+<<>>
+stream
+612 0.0 0.0 104000 0 0 cm
+/XBad Do
+endstream
+endobj
+
+5 0 obj
+<<
+  /XObject <<
+    /XBad 7 0 R
+  >>
+>>
+endobj
+
+7 0 obj
+<<
+  /Type /XObject
+  /Subtype /Image
+  /Width 612
+  /Height 104000
+  /ColorSpace /DeviceRGB
+  /BitsPerComponent 4
+  /Length 0
+  /Decode [1.0]
+>>
+stream
+FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
+endobj
+
+xref
+0 8
+0000000000 65535 f 
+0000000016 00000 n 
+0000000053 00000 n 
+0000000093 00000 n 
+0000000000 65535 f 
+0000000225 00000 n 
+0000000152 00000 n 
+0000000282 00000 n 
+trailer <<
+  /Root 1 0 R
+  /Size 8
+>>
+startxref
+1370
+%%EOF