Clear decoders after the image decoder in the /Filter array

core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp

Index: core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp
diff --git a/core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp b/core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp
index e68fcb6800a30792db44dc2ec3f56e96c260a882..18233c764a61c9bd0a32ec01d413adfd431bb118 100644
--- a/core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp
+++ b/core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp
@@ -362,7 +362,7 @@ FX_BOOL PDF_DataDecode(const uint8_t* src_buf,
     // Use ToDictionary here because we can push NULL into the ParamList.
     CPDF_Dictionary* pParam =
         ToDictionary(static_cast<CPDF_Object*>(ParamList[i]));
-    uint8_t* new_buf = NULL;
+    uint8_t* new_buf = nullptr;
     FX_DWORD new_size = (FX_DWORD)-1;
     int offset = -1;
     if (decoder == FX_BSTRC("FlateDecode") || decoder == FX_BSTRC("Fl")) {
@@ -394,19 +394,21 @@ FX_BOOL PDF_DataDecode(const uint8_t* src_buf,
         return TRUE;
       }
       offset = RunLengthDecode(last_buf, last_size, new_buf, new_size);
-    } else {
+    } else if (i == DecoderList.GetSize() - 1) {

[jun_fang, 2015/11/09 11:09:35]

It's too strict checking here. 
The filters DCT(JPEG) and CCITT are just used for image compression.

for image compression, the procedure can be:
raw images -> DCT/CCITT -> other loss-less compression

for decompression:
compressed data -> loss-less decompression -> DCT/CCITT decoders -> raw images

DCT or CCITT should be the first encoders so that they should be
the last decoders in the filter pipeline (for decoding). 

If DCT or CCITT decoder is found in the filter pipeline, we don't need to check
the other filters. 
 

[Oliver Chang, 2015/11/09 20:41:22]

Apologies if I misunderstood something, but why is this too strict here?

To clarify some points (correct me if I'm wrong):

- If the decoder is an image one
(CCITTFaxDecode/JBIG2Decode/DCTDecode/JPXDecode), it should be the last one in
the /Filter list. 

- The previous behaviour is: if we fall into the "else", which means the current
decoder could be anything that didn't match the previous cases, including
CCITTFaxDecode/JBIG2Decode/DCTDecode/JPXDecode, it's assumed to be an image
decoder (unless it's "Crypt") and we return. |ImageEncoding| is set regardless
of whether if it's the last filter or if it's a valid filter name. This causes
issues later as discussed in the bug.

- The new behaviour is: |ImageEncoding| is only set if the filter is the last
one in the array (I realised there is a potential error with "Crypt" filters, so
I moved it into it's own else case above). This means that image encoders
specified earlier in the array are skipped, and only the last one is taken. If
multiple image encoders are specified, I assume it's undefined behaviour? (I
can't find anything in the PDF spec).

Would a better fix instead be to keep the original behaviour of ignoring
everything past the first image decoder, but also clearing out subsequent
entries of the /Filters |CPDF_Array|? 

e.g. pDecoders->RemoveAt(i + 1, pDecoders->GetCount() - i); (the
CPDF_Array::RemoveAt method will need to be modified to take a |nCount| like the
one in CFX_ArrayTemplate<T>::RemoveAt)

[jun_fang, 2015/11/11 10:10:36]

 - The previous behavior is: all valid or invalid decoders will be ignored after
the first image decoder is found. 
 - The new behavior is: we assume that an image decoder shall be the last filter
in the filter array.

Usually, there is only one image decoder in the filter array. As you mentioned
that it was undefined behavior if multiple image encoders were specified.

Let me explain why I think it's stricter than previous for your CL:
For example, we have a filter array in a PDF file. Filter N can be a valid or
invalid filter.
/filter [/filter 1 /filter 2 ... /DCTDecode /filter N]
For this case, your CL will ignore the only one image decoder.

My proposal for this CL is:
for (int i = 0; i < pDecoders->GetCount(); ++i) {
  if (valid non-image decoders except Crypt) {    
    if (i == pDecoders->GetCount() -1) {
      ImageEncoding = decoder;
      return;
    }
    decode();
    continue;
  } else if (valid image decoders) {
    ImageEncoding = decoder;
    return;
  } else { //Crypt or invalid decoders
    continue;
  }
}

[jun_fang, 2015/11/11 10:13:39]

I don't have the permission to access the test file. I would like to check the
test file if you can send it to me. Thanks!

[Oliver Chang, 2015/11/11 17:25:19]

Thanks for the clarification!

I see, so we still want to decode with the image decoder even if there are other
(ignored) decoders specified after it in the /Filter array. Is this part of the
standard, or just a best-effort "let me guess what you mean" approach?

It seems that in fpdf_render_loadimage.cpp, image decoders are assumed to be at
the last position in the /Filter array. See:

else if (CPDF_Array* pArray = pFilter->AsArray()) {
      if (pArray->GetString(pArray->GetCount() - 1) ==
              FX_BSTRC("CCITTFaxDecode") ||
          pArray->GetString(pArray->GetCount() - 1) ==
              FX_BSTRC("JBIG2Decode")) {
        m_bpc = 1;
        m_nComponents = 1;
      }
      if (pArray->GetString(pArray->GetCount() - 1) == FX_BSTRC("DCTDecode")) {
        // Previously, pArray->GetString(pArray->GetCount() - 1) ==
        // FX_BSTRC("RunLengthDecode") was checked in the "if" statement as
        // well,
        // but too many documents don't conform to it.
        m_bpc = 8;
      }

The problem with your suggestion is that the bug is still there: if we return
early and take the earlier |decoder| as the |ImageEncoding|, later on |m_bpc|
will be "fixed" to use a image decoder in the last position.

What do you think of my last suggestion (clearing out the Filters CPDF_Array
past the image decoder)? This ensures that the image decoder is the last filter,
as is expected in fpdf_render_loadimage.cpp. Because I assume we're in very
different timezones, I'll upload the patch preemptively later today.

Also, I've sent you the testcase via email. 

+      // Check if this is an image decoder.
       if (decoder == FX_BSTRC("DCT")) {
         decoder = "DCTDecode";
       } else if (decoder == FX_BSTRC("CCF")) {
         decoder = "CCITTFaxDecode";
-      } else if (decoder == FX_BSTRC("Crypt")) {
-        continue;
       }
       ImageEncoding = decoder;
       pImageParms = pParam;
       dest_buf = (uint8_t*)last_buf;
       dest_size = last_size;
       return TRUE;
+    } else {
+      // Ignore all other decoders.
+      continue;
     }
     if (last_buf != src_buf) {
       FX_Free(last_buf);
@@ -419,7 +421,7 @@ FX_BOOL PDF_DataDecode(const uint8_t* src_buf,
     last_size = new_size;
   }
   ImageEncoding = "";
-  pImageParms = NULL;
+  pImageParms = nullptr;
   dest_buf = last_buf;
   dest_size = last_size;
   return TRUE;