Fix extraction of colour components in CPDF_DIBSource::DownSampleScanline32Bit

core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp

Index: core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
diff --git a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
index f6f41d9de11f000598c6858b726afa2a068650ac..53aeb1ed62d195a927e2508997ac03cd0d4d9d19 100644
--- a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
+++ b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
@@ -19,7 +19,7 @@
 
 namespace {
 
-unsigned int _GetBits8(const uint8_t* pData, int bitpos, int nbits) {
+unsigned int _GetBits8(const uint8_t* pData, size_t bitpos, size_t nbits) {

[Tom Sepez, 2015/11/11 21:09:44]

follow-up: bitpos needs to be an uint64_t.

kMaxImageDimension is 0x01ffff, so the maximum bytes in an image won't fit in a
size_t on 32 bit platforms, so image size is limited only by memory there. Its
conceivable that we might have a 1GB image even on such a platform, so bitpos ==
8G won't fit into a size_t there either.  This needs to percolate down through
all the other arithmetic.  Sigh.

nit: lose the leading _ as it is now namespaced.

[Oliver Chang, 2015/11/11 22:30:59]

Done. will fix up callers in a separate CL.

   unsigned int byte = pData[bitpos / 8];

[Tom Sepez, 2015/11/11 21:09:45]

ASSERT(nbits == 1 || nbits == 2 || nbits == 4 ||  nbits == 8 || nbits == 16);
ASSERT((bitpos & (nbits - 1)) == 0);

[Oliver Chang, 2015/11/11 22:30:59]

Done.

   if (nbits == 8) {
     return byte;
@@ -1431,59 +1431,67 @@ 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;
-    const uint8_t* pSrcPixel = nullptr;
+
+    // No need to check for overflow, as |src_x| is bounded by |src_width| and
+    // DownSampleScanline already checked for overflow with the pitch
+    // calculation.
+    size_t byte_offset = src_x;
+    size_t bit_offset = 0;
     if (m_bpc % 8 == 0) {
-      pSrcPixel = pSrcLine + src_x * orig_Bpp;
+      byte_offset *= orig_Bpp;
     } else {
-      pSrcPixel = pSrcLine + (int)(src_x * orig_Not8Bpp);
+      byte_offset *= m_bpc;

[Tom Sepez, 2015/11/11 21:09:45]

nit: this can probably be an infix expression since we are not using safe types.
 And using byte_offset for the temporary is a little misleading since we want to
create a count of bits, then derive bytes from it.

[Oliver Chang, 2015/11/11 22:30:59]

Done. I initially used a safe size_t, but neglected to make it read a bit nicer
after I changed it back.

+      byte_offset *= m_nComponents;
+      bit_offset = byte_offset % 8;
+      byte_offset /= 8;
     }
+
+    const uint8_t* pSrcPixel = pSrcLine + byte_offset;
     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) {
+        for (FX_DWORD j = 0; j < m_nComponents; ++j) {
+          extracted_components[j] =
+              _GetBits8(pSrcPixel, bit_offset + (j * m_bpc), m_bpc) *
+              unit_To8Bpc;
+        }
+        pSrcPixel = extracted_components;
+      }
+

[Tom Sepez, 2015/11/11 21:09:45]

I think there's a problem with {} nesting, I'd expect to see an else branch here
for the n == 8 case.

[Oliver Chang, 2015/11/11 22:30:59]

Acknowledged.

       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;