Clear decoders after the image decoder in the /Filter array

Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any
subsequent decoders are ignored, but remain in the array. However,
later on CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

A check is also added in CPDF_DIBSource::GetScanline to ensure
that the calculated pitch value is <= the (4-aligned) pitch value in the
cached bitmap to prevent future issues.

Also cleans up some NULL usages.

BUG=552046
R=jun_fang@foxitsoftware.com, tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/182d129bcee8f7731b9bbfde0064295ad3b37271

[Oliver Chang, 2015-11-06 22:26:03]

Description was changed from

==========
Only take the last decoder in the /Filter array as the image decoder.

BUG=552046
==========

to

==========
Only take the last decoder in the /Filter array as the image decoder.

This also changes the decoding so that unknown decoders are skipped (but
continues).

BUG=552046
==========

[Oliver Chang, 2015-11-06 22:27:02]

Description was changed from

==========
Only take the last decoder in the /Filter array as the image decoder.

This also changes the decoding so that unknown decoders are skipped (but
continues).

BUG=552046
==========

to

==========
Only take the last decoder in the /Filter array as the image decoder.

This also changes the decoding so that unknown decoders are skipped
(but decoding continues).

BUG=552046
==========

[Oliver Chang, 2015-11-06 22:32:19]

Description was changed from

==========
Only take the last decoder in the /Filter array as the image decoder.

This also changes the decoding so that unknown decoders are skipped
(but decoding continues).

BUG=552046
==========

to

==========
Only take the last decoder in the /Filter array as the image decoder.

This syncs up the behaviour of fpdf_parser_decode.cpp with
fpdf_render_loadimage.cpp.

Stream decoding is changed so that unknown decoders are skipped
(but decoding continues).

Also cleans up some NULL usages.

BUG=552046
==========

[Oliver Chang, 2015-11-06 23:06:59]

Patchset #1 (id:1) has been deleted

[Oliver Chang, 2015-11-06 23:09:32]

ochang@chromium.org changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[Oliver Chang, 2015-11-06 23:09:32]

ptal.

[Oliver Chang, 2015-11-06 23:11:08]

not 100% sure if this is the correct fix here. suggestions are welcome.

[Tom Sepez, 2015-11-06 23:15:53]

tsepez@chromium.org changed reviewers:
+ jun_fang@foxitsoftware.com

[Tom Sepez, 2015-11-06 23:16:05]

+jun because it deals with core pdf behaviour.

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

https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (right):

https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:397: } else if (i ==
DecoderList.GetSize() - 1) {
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:23]

https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (right):

https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:397: } else if (i ==
DecoderList.GetSize() - 1) {
On 2015/11/09 11:09:35, jun_fang wrote:
> 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. 
>  

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)

[Oliver Chang, 2015-11-09 21:32:28]

On 2015/11/09 20:41:23, Oliver Chang wrote:
>
https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
> File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (right):
> 
>
https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:397: } else if (i ==
> DecoderList.GetSize() - 1) {
> On 2015/11/09 11:09:35, jun_fang wrote:
> > 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. 
> >  
> 
> 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)

s/pDecoders->GetCount() - i/pDecoders->GetCount() - i - 1/ in the previous
example.

[Oliver Chang, 2015-11-10 22:20:46]

Jun, friendly ping.

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

https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (right):

https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:397: } else if (i ==
DecoderList.GetSize() - 1) {
On 2015/11/09 20:41:22, Oliver Chang wrote:
> On 2015/11/09 11:09:35, jun_fang wrote:
> > 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. 
> >  
> 
> 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)

 - 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]

https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (right):

https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:397: } else if (i ==
DecoderList.GetSize() - 1) {
On 2015/11/11 10:10:36, jun_fang wrote:
> On 2015/11/09 20:41:22, Oliver Chang wrote:
> > On 2015/11/09 11:09:35, jun_fang wrote:
> > > 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. 
> > >  
> > 
> > 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)
> 
>  - 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;
>   }
> }

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]

https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (right):

https://codereview.chromium.org/1406943005/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:397: } else if (i ==
DecoderList.GetSize() - 1) {
On 2015/11/11 10:10:36, jun_fang wrote:
> On 2015/11/09 20:41:22, Oliver Chang wrote:
> > On 2015/11/09 11:09:35, jun_fang wrote:
> > > 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. 
> > >  
> > 
> > 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)
> 
>  - 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;
>   }
> }

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.

[Oliver Chang, 2015-11-11 22:50:16]

Jun, please see latest patchset when you get the chance. 

I'm not completely sure if it's safe to remove filters from the CPDF_Array at
this point, what do you think?

[jun_fang, 2015-11-12 05:23:40]

On 2015/11/11 22:50:16, Oliver Chang wrote:
> Jun, please see latest patchset when you get the chance. 
> 
> I'm not completely sure if it's safe to remove filters from the CPDF_Array at
> this point, what do you think?

You are right. We have to remove filters from the CPDF_Array.
Because I checked the test file and found that the root cause was:

For the filters in the test file like /Filter [/JBIG2Decode /DCTDecode]
Pdifum thinks there is an image with JBIG2 format when it found the first 
filter /JBIG2Decode FX_BOOL PDF_DataDecode(). So bit per component (bpc) 
was set to 1. 

However, in CPDF_DIBSource::ValidateDictParam(), pdfium tries to correct bpc
based on the last filter in the filter array. The last filter is /DCTDecode
not /JBIG2 so the image's bpc is kept as the default value, 8. 

The different bpcs cause the crasher reported in 552046.

[jun_fang, 2015-11-12 05:35:28]

On 2015/11/12 05:23:40, jun_fang wrote:
> On 2015/11/11 22:50:16, Oliver Chang wrote:
> > Jun, please see latest patchset when you get the chance. 
> > 
> > I'm not completely sure if it's safe to remove filters from the CPDF_Array
at
> > this point, what do you think?
> 
> You are right. We have to remove filters from the CPDF_Array.
> Because I checked the test file and found that the root cause was:
> 
> For the filters in the test file like /Filter [/JBIG2Decode /DCTDecode]
> Pdifum thinks there is an image with JBIG2 format when it found the first 
> filter /JBIG2Decode FX_BOOL PDF_DataDecode(). So bit per component (bpc) 
> was set to 1. 
> 
> However, in CPDF_DIBSource::ValidateDictParam(), pdfium tries to correct bpc
> based on the last filter in the filter array. The last filter is /DCTDecode
> not /JBIG2 so the image's bpc is kept as the default value, 8. 
> 
> The different bpcs cause the crasher reported in 552046.

LGTM with one comment.

[Oliver Chang, 2015-11-12 05:38:33]

On 2015/11/12 05:35:28, jun_fang wrote:
> On 2015/11/12 05:23:40, jun_fang wrote:
> > On 2015/11/11 22:50:16, Oliver Chang wrote:
> > > Jun, please see latest patchset when you get the chance. 
> > > 
> > > I'm not completely sure if it's safe to remove filters from the CPDF_Array
> at
> > > this point, what do you think?
> > 
> > You are right. We have to remove filters from the CPDF_Array.
> > Because I checked the test file and found that the root cause was:
> > 
> > For the filters in the test file like /Filter [/JBIG2Decode /DCTDecode]
> > Pdifum thinks there is an image with JBIG2 format when it found the first 
> > filter /JBIG2Decode FX_BOOL PDF_DataDecode(). So bit per component (bpc) 
> > was set to 1. 
> > 
> > However, in CPDF_DIBSource::ValidateDictParam(), pdfium tries to correct bpc
> > based on the last filter in the filter array. The last filter is /DCTDecode
> > not /JBIG2 so the image's bpc is kept as the default value, 8. 
> > 
> > The different bpcs cause the crasher reported in 552046.
> 
> LGTM with one comment.

Thanks!

What's the comment? (I don't see it?)

I did explain the very same in the bug itself (https://crbug.com/552046),
perhaps it wasn't communicated well :(

[jun_fang, 2015-11-12 05:41:07]

https://codereview.chromium.org/1406943005/diff/120001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1406943005/diff/120001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1092: if (m_pCachedBitmap
&& src_pitch_value <= m_pCachedBitmap->GetPitch()) {
src_pitch_value should equal m_pCachedBitmap->GetPitch().

[Oliver Chang, 2015-11-12 05:53:57]

Patchset #8 (id:160001) has been deleted

[Oliver Chang, 2015-11-12 05:54:02]

Patchset #7 (id:140001) has been deleted

[Oliver Chang, 2015-11-12 06:00:42]

Description was changed from

==========
Only take the last decoder in the /Filter array as the image decoder.

This syncs up the behaviour of fpdf_parser_decode.cpp with
fpdf_render_loadimage.cpp.

Stream decoding is changed so that unknown decoders are skipped
(but decoding continues).

Also cleans up some NULL usages.

BUG=552046
==========

to

==========
Clear decoders after the image decoder in the /Filter array.

This syncs up the behaviour of fpdf_parser_decode.cpp with
fpdf_render_loadimage.cpp:
CPDF_DIBSource::ValidateDictParam expects the image decoder to be the last in
the array.

Stream decoding is changed so that unknown decoders are skipped
(but decoding continues).

Also cleans up some NULL usages.

BUG=552046
==========

[Oliver Chang, 2015-11-12 06:04:01]

Description was changed from

==========
Clear decoders after the image decoder in the /Filter array.

This syncs up the behaviour of fpdf_parser_decode.cpp with
fpdf_render_loadimage.cpp:
CPDF_DIBSource::ValidateDictParam expects the image decoder to be the last in
the array.

Stream decoding is changed so that unknown decoders are skipped
(but decoding continues).

Also cleans up some NULL usages.

BUG=552046
==========

to

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, the first image decoder is taken, and any 
subsequent decoders are ignored. However, later on
CPDF_DIBSource::ValidateDictParam expects the image decoder
to be the last in the array.

Also cleans up some NULL usages.

BUG=552046
==========

[Oliver Chang, 2015-11-12 06:06:23]

Description was changed from

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, the first image decoder is taken, and any 
subsequent decoders are ignored. However, later on
CPDF_DIBSource::ValidateDictParam expects the image decoder
to be the last in the array.

Also cleans up some NULL usages.

BUG=552046
==========

to

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However, later on
CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

Also cleans up some NULL usages.

BUG=552046
==========

[Oliver Chang, 2015-11-12 06:06:39]

Description was changed from

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However, later on
CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

Also cleans up some NULL usages.

BUG=552046
==========

to

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However,
later on CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

Also cleans up some NULL usages.

BUG=552046
==========

[jun_fang, 2015-11-12 08:51:12]

On 2015/11/12 05:41:07, jun_fang wrote:
>
https://codereview.chromium.org/1406943005/diff/120001/core/src/fpdfapi/fpdf_...
> File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):
> 
>
https://codereview.chromium.org/1406943005/diff/120001/core/src/fpdfapi/fpdf_...
> core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1092: if
(m_pCachedBitmap
> && src_pitch_value <= m_pCachedBitmap->GetPitch()) {
> src_pitch_value should equal m_pCachedBitmap->GetPitch().

LGTM again.

[Oliver Chang, 2015-11-12 16:59:23]

On 2015/11/12 08:51:12, jun_fang wrote:
> On 2015/11/12 05:41:07, jun_fang wrote:
> >
>
https://codereview.chromium.org/1406943005/diff/120001/core/src/fpdfapi/fpdf_...
> > File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1406943005/diff/120001/core/src/fpdfapi/fpdf_...
> > core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1092: if
> (m_pCachedBitmap
> > && src_pitch_value <= m_pCachedBitmap->GetPitch()) {
> > src_pitch_value should equal m_pCachedBitmap->GetPitch().
> 
> LGTM again.

Thanks again Jun. Any comments before I land this, Tom, Lei?

[Tom Sepez, 2015-11-12 17:39:19]

lgtm

https://codereview.chromium.org/1406943005/diff/180001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1406943005/diff/180001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:500: ASSERT(IsArray());
nit: probably pointless given that we've cleaned up casting quite a bit.

https://codereview.chromium.org/1406943005/diff/180001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1406943005/diff/180001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1094: if (m_pCachedBitmap
&& src_pitch_value == m_pCachedBitmap->GetPitch()) {
can you explain why we need this check here, maybe as part of the CL
description?

[Oliver Chang, 2015-11-12 17:47:28]

Description was changed from

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However,
later on CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

Also cleans up some NULL usages.

BUG=552046
==========

to

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However,
later on CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

A check is also added in CPDF_DIBSource::GetScanline to ensure
that the calculated pitch value matches the expected value in the
cached bitmap.

Also cleans up some NULL usages.

BUG=552046
==========

[Oliver Chang, 2015-11-12 17:50:10]

Description was changed from

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However,
later on CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

A check is also added in CPDF_DIBSource::GetScanline to ensure
that the calculated pitch value matches the expected value in the
cached bitmap.

Also cleans up some NULL usages.

BUG=552046
==========

to

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However,
later on CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

A check is also added in CPDF_DIBSource::GetScanline to ensure
that the calculated pitch value matches the expected value in the
cached bitmap to prevent future issues.

Also cleans up some NULL usages.

BUG=552046
==========

[Oliver Chang, 2015-11-12 18:39:45]

https://codereview.chromium.org/1406943005/diff/120001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1406943005/diff/120001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1092: if (m_pCachedBitmap
&& src_pitch_value <= m_pCachedBitmap->GetPitch()) {
On 2015/11/12 05:41:07, jun_fang wrote:
> src_pitch_value should equal m_pCachedBitmap->GetPitch().

This broke some things, as DIBitmap calculates the pitch using:

pitch = (width * (format & 0xff) + 31) / 32 * 4; // Like in CalculatePitch32,
which is aligned to 4 bytes.

While src_pitch is calculated using CalculatePitch8. (We'll very likely want to
cleanup the corresponding arithmetic in DIBitmap).

I'm going to keep this as "<=" to prevent future overflows for now.

https://codereview.chromium.org/1406943005/diff/180001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1406943005/diff/180001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:500: ASSERT(IsArray());
On 2015/11/12 17:39:19, Tom Sepez wrote:
> nit: probably pointless given that we've cleaned up casting quite a bit.

removed.

https://codereview.chromium.org/1406943005/diff/180001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1406943005/diff/180001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1094: if (m_pCachedBitmap
&& src_pitch_value == m_pCachedBitmap->GetPitch()) {
On 2015/11/12 17:39:19, Tom Sepez wrote:
> can you explain why we need this check here, maybe as part of the CL
> description?

Done. Modified the CL description, also changed slightly back to what i had
before. See my reply to Jun's comment.

[Oliver Chang, 2015-11-12 18:41:41]

Description was changed from

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However,
later on CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

A check is also added in CPDF_DIBSource::GetScanline to ensure
that the calculated pitch value matches the expected value in the
cached bitmap to prevent future issues.

Also cleans up some NULL usages.

BUG=552046
==========

to

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However,
later on CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

A check is also added in CPDF_DIBSource::GetScanline to ensure
that the calculated pitch value is <= the pitch value in the
cached bitmap to prevent future issues.

Also cleans up some NULL usages.

BUG=552046
==========

[Oliver Chang, 2015-11-12 18:42:04]

Description was changed from

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However,
later on CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

A check is also added in CPDF_DIBSource::GetScanline to ensure
that the calculated pitch value is <= the pitch value in the
cached bitmap to prevent future issues.

Also cleans up some NULL usages.

BUG=552046
==========

to

==========
Clear decoders after the image decoder in the /Filter array.

During decoding, when an image decoder is encountered, any 
subsequent decoders are ignored, but remain in the array. However,
later on CPDF_DIBSource::ValidateDictParam expects the image
decoder to be the last in the array, causing issues.

A check is also added in CPDF_DIBSource::GetScanline to ensure
that the calculated pitch value is <= the (4-aligned) pitch value in the
cached bitmap to prevent future issues.

Also cleans up some NULL usages.

BUG=552046
==========

[Oliver Chang, 2015-11-12 18:43:34]

Committed patchset #9 (id:220001) manually as
182d129bcee8f7731b9bbfde0064295ad3b37271 (presubmit successful).