Bounds check before fixed-size memcmp() in CJPX_Decoder::Init().

Bounds check before fixed-size memcmp() in CJPX_Decoder::Init().

BUG=407476
R=jun_fang@foxitsoftware.com

Committed: https://pdfium.googlesource.com/pdfium/+/96f8786

[Tom Sepez, 2014-08-26 19:40:59]

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

[Tom Sepez, 2014-08-26 19:40:59]

Jun, please review.

[Bo Xu, 2014-08-26 20:09:29]

On 2014/08/26 19:40:59, Tom Sepez wrote:
> Jun, please review.

Hi, Tom, I was looking at this issue too. Should we move the size check to the
very beginning of the function? A stream less than 12 bits won't make sense to
decode and will ultimately make obj_read_header return false.

[Tom Sepez, 2014-08-26 21:02:38]

Hi Bo, that's fine. I'll introduce a constant for the strings so that we won't
have widely separated "12"s that need to change together.

[Tom Sepez, 2014-08-26 21:03:05]

On 2014/08/26 21:02:38, Tom Sepez wrote:
> Hi Bo, that's fine. I'll introduce a constant for the string and its length so
that we won't
> have widely separated "12"s that need to change together.

[jun_fang, 2014-08-26 21:48:09]

Please see my comments.

https://codereview.chromium.org/489703004/diff/1/core/src/fxcodec/codec/fx_co...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/489703004/diff/1/core/src/fxcodec/codec/fx_co...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:584: opj_dparameters_t parameters;
if (!src_data || src_size <= 0) {
    return FALSE;
}

https://codereview.chromium.org/489703004/diff/1/core/src/fxcodec/codec/fx_co...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:599: if(m_SrcSize >= 12 &&
FXSYS_memcmp32(m_SrcData, "\x00\x00\x00\x0c\x6a\x50\x20\x20\x0d\x0a\x87\x0a",
12) == 0) {
1. Define an array to save the string "\x00\x00\x00\x0c\x6a\x50\x2
     0\x20\x0d\x0a\x87\x0a". This array's name can be szJP2Header.
2. Use sizeof(arrary_name) to replace '12' that is a magic number.

[Tom Sepez, 2014-08-26 22:00:07]

See PS #3.  I'm not sure what naming convention we're eventually going to adopt
for pdfium, but this reads clearer to me than szJp2

[jun_fang, 2014-08-26 22:01:17]

https://codereview.chromium.org/489703004/diff/1/core/src/fxcodec/codec/fx_co...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/489703004/diff/1/core/src/fxcodec/codec/fx_co...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:584: opj_dparameters_t parameters;
On 2014/08/26 21:48:09, jun_fang wrote:
> if (!src_data || src_size <= 0) {
>     return FALSE;
> }

We can define an array to save the string
"\x00\x00\x00\x0c\x6a\x50\x20\x20\x0d\x0a\x87\x0a" here and check:

char szJP2Header[] = {'\x00','\x00',
'\x00','\x0c','\x6a','\x50','\x20','\x20','\x0d','\x0a','\x87','\x0a'};

if (!src_data || src_size <= sizeof(szJP2Header)/sizeof(char)) {
    return FALSE;
}

[jun_fang, 2014-08-26 22:09:15]

https://codereview.chromium.org/489703004/diff/60001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/489703004/diff/60001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:13: const size_t
ExpectedJP2HeaderSize = sizeof(ExpectedJP2Header) - 1;
are you sure that it needs to subtract 1 (-1)? sizeof(array) indicates the
actual length of an array.

[Tom Sepez, 2014-08-26 22:10:39]

On 2014/08/26 22:01:17, jun_fang wrote:
>
https://codereview.chromium.org/489703004/diff/1/core/src/fxcodec/codec/fx_co...
> File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):
> 
>
https://codereview.chromium.org/489703004/diff/1/core/src/fxcodec/codec/fx_co...
> core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:584: opj_dparameters_t parameters;
> On 2014/08/26 21:48:09, jun_fang wrote:
> > if (!src_data || src_size <= 0) {
> >     return FALSE;
> > }
you mean src_size < sizeof(szJP2Header)

> 
> We can define an array to save the string
> "\x00\x00\x00\x0c\x6a\x50\x20\x20\x0d\x0a\x87\x0a" here and check:
> 
> char szJP2Header[] = {'\x00','\x00',
> '\x00','\x0c','\x6a','\x50','\x20','\x20','\x0d','\x0a','\x87','\x0a'};
> 
> if (!src_data || src_size <= sizeof(szJP2Header)/sizeof(char)) {
>     return FALSE;
> }
I got rid of the /sizeof(char).  Its pointless, and looks wrong in the memcpy
(which wants bytes, not elements).

[jun_fang, 2014-08-26 22:15:03]

https://codereview.chromium.org/489703004/diff/80001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/489703004/diff/80001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:587: if (!src_data || src_size <
sizeof(szJP2Header)) {
it should be '<=' rather than '<', right?

https://codereview.chromium.org/489703004/diff/80001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:605: if(FXSYS_memcmp32(m_SrcData,
szJP2Header, sizeof(szJP2HeaderSize) == 0) {
sizeof(szJP2HeaderSize) should be sizeof(szJP2Header)?

[jun_fang, 2014-08-26 22:18:43]

https://codereview.chromium.org/489703004/diff/100001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/489703004/diff/100001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:12: const unsigned char
szJP2Header[] = { 0x00, 0x00, 0x00, 0x0c, 0x6a, 0x50, 0x20, 0x20, 0x0d, 0x0a,
0x87, 0x0a };
can we move this definition into CJPX_Decoder::Init? If it isn't shared with
others. we don't need to define it as a global variable.

[Tom Sepez, 2014-08-26 22:21:40]

On 2014/08/26 22:18:43, jun_fang wrote:
>
https://codereview.chromium.org/489703004/diff/100001/core/src/fxcodec/codec/...
> File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):
> 
>
https://codereview.chromium.org/489703004/diff/100001/core/src/fxcodec/codec/...
> core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:12: const unsigned char
> szJP2Header[] = { 0x00, 0x00, 0x00, 0x0c, 0x6a, 0x50, 0x20, 0x20, 0x0d, 0x0a,
> 0x87, 0x0a };
> can we move this definition into CJPX_Decoder::Init? If it isn't shared with
> others. we don't need to define it as a global variable.
Sure, that's a good idea.

[jun_fang, 2014-08-26 22:39:50]

On 2014/08/26 22:21:40, Tom Sepez wrote:
> On 2014/08/26 22:18:43, jun_fang wrote:
> >
>
https://codereview.chromium.org/489703004/diff/100001/core/src/fxcodec/codec/...
> > File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):
> > 
> >
>
https://codereview.chromium.org/489703004/diff/100001/core/src/fxcodec/codec/...
> > core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:12: const unsigned char
> > szJP2Header[] = { 0x00, 0x00, 0x00, 0x0c, 0x6a, 0x50, 0x20, 0x20, 0x0d,
0x0a,
> > 0x87, 0x0a };
> > can we move this definition into CJPX_Decoder::Init? If it isn't shared with
> > others. we don't need to define it as a global variable.
> Sure, that's a good idea.

LGTM. Thanks!

[Tom Sepez, 2014-08-26 23:35:16]

Committed patchset #7 manually as 96f8786 (presubmit successful).