Add GetValidBpc() check in CPDF_DIBSource::CreateDecoder()

Add GetValidBpc() check in CPDF_DIBSource::CreateDecoder()


BUG=406908
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/187483a

[Bo Xu, 2014-08-25 02:02:41]

Hi, Tom, please review this.

[Tom Sepez, 2014-08-25 17:20:11]

Looks like this will fix the issue, but there's some additional cleanup possible
while we're at it.  Thanks.

https://codereview.chromium.org/504673002/diff/1/core/src/fpdfapi/fpdf_render...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/504673002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:561: int
CPDF_DIBSource::CreateDecoder()
Just curious, what does the return value from this function mean? success vs.
failure? Would this be happier as an FX_BOOL, returning TRUE/FALSE rather than
0/1?

https://codereview.chromium.org/504673002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:563: FX_DWORD bpc =
GetValidBpc();
Do we need to check for the bpc == 0 case and return an error?
Also, we can move this call down below the decoder.IsEmpty() call to avoid doing
any work when this short-ciruits?

https://codereview.chromium.org/504673002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:580: if
(pJpegModule->LoadInfo(src_data, src_size, m_Width, m_Height, comps,
(FX_INT32&)bpc, bTransform)) {
This cast looks suspicious. Maybe bpc should be an FX_INT32? Alternatively,
maybe LoadInfo() changes to take an FX_DWORD reference? Casting
pointers/reference types like this can be a source of subtle bugs (when the
underlying sizes don't match, etc.).

If you absolutely can't get around it, you'll need to make a temporary
  FX_INT32 bpc_as_int = bpc;
  sts =  ...( ..., bpc_as_int, ...)
  bpc = bpc_as_int;
To be strictly legal.

[Bo Xu, 2014-08-25 18:16:43]

https://codereview.chromium.org/504673002/diff/1/core/src/fpdfapi/fpdf_render...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/504673002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:561: int
CPDF_DIBSource::CreateDecoder()
On 2014/08/25 17:20:11, Tom Sepez wrote:
> Just curious, what does the return value from this function mean? success vs.
> failure? Would this be happier as an FX_BOOL, returning TRUE/FALSE rather than
> 0/1?

I am not entirely sure for now. This function can return 0, 1 or 2. When decoder
== "JBIG2Decode", it's possible to return 2. From callers of this function, it
seems 1 and 2 means successful. One such caller is at line 345. I can check with
the devs to confirm.

I also change some of the return value of 1 in my previous patch to 0.

https://codereview.chromium.org/504673002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:563: FX_DWORD bpc =
GetValidBpc();
On 2014/08/25 17:20:11, Tom Sepez wrote:
> Do we need to check for the bpc == 0 case and return an error?
> Also, we can move this call down below the decoder.IsEmpty() call to avoid
doing
> any work when this short-ciruits?

Done.

https://codereview.chromium.org/504673002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:580: if
(pJpegModule->LoadInfo(src_data, src_size, m_Width, m_Height, comps,
(FX_INT32&)bpc, bTransform)) {
On 2014/08/25 17:20:11, Tom Sepez wrote:
> This cast looks suspicious. Maybe bpc should be an FX_INT32? Alternatively,
> maybe LoadInfo() changes to take an FX_DWORD reference? Casting
> pointers/reference types like this can be a source of subtle bugs (when the
> underlying sizes don't match, etc.).
> 
> If you absolutely can't get around it, you'll need to make a temporary

Now I use the temporal bpc to get around this.
>   FX_INT32 bpc_as_int = bpc;
>   sts =  ...( ..., bpc_as_int, ...)
>   bpc = bpc_as_int;
> To be strictly legal.

[Tom Sepez, 2014-08-25 18:23:47]

lgtm

[Bo Xu, 2014-08-25 20:34:02]

Committed patchset #2 manually as 187483a (presubmit successful).