Fix buffer size offset error in PNG_Predictor

Fix buffer size offset error in PNG_Predictor

BUG=393602
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/d726307

[Bo Xu, 2014-08-14 17:16:45]

Tom, please review this one, thanks.

[Tom Sepez, 2014-08-14 19:57:21]

On 2014/08/14 17:16:45, Bo Xu wrote:
> Tom, please review this one, thanks.

This doesn't look right somehow.   dest_buf was allocated with a size of
  FX_LPBYTE dest_buf = FX_Alloc( FX_BYTE, row_size * row_count);
so I'd expect to add a stride of row_size, not row_size+1 to pDestData with each
pass to iterate over the start of rows.

I'm a little confused as to why 
int row_count = (data_size + row_size) / (row_size + 1);

if you're rounding count down to a row_size boundary, I'd epexect (data_size /
row_size).
if you're rounding count up to a row_size boundary, I'd expect (data_size +
row_size - 1) / row_size.
Does each row end with some kind of terminating byte, hence the need to add one
here?  A comment might be helpful.

[Tom Sepez, 2014-08-14 20:10:20]

Inspection of the faulting line (413) indicates that a one byte read beyond the
buffer can occur when byte_cnt == data_size-1;

412        for (int byte = 0; byte < row_size && byte_cnt < (int)data_size; byte
++) {
413            FX_BYTE raw_byte = pSrcData[byte + 1];

Next step is to figure out how that might happen.

[Tom Sepez, 2014-08-14 20:53:04]

Looking at this, I think the byte_cnt++ at line 467 needs to move up to before
line 412 to account for reading the tag byte.  Otherwise, a file truncated at a
tag byte will read the one extra byte on 413.

[Tom Sepez, 2014-08-14 20:56:51]

On 2014/08/14 20:53:04, Tom Sepez wrote:
> Looking at this, I think the byte_cnt++ at line 467 needs to move up to before
> line 412 to account for reading the tag byte.  Otherwise, a file truncated at
a
> tag byte will read the one extra byte on 413.

Anyways, we should consider re-working part of this, having to do a switch on a
per-line constant tag inside of a per-pixel loop is sure to be slower unless a
good optimizer can figure this out.

It also looks that the calculation of 
   int row_size = (Colors * BitsPerComponent * Columns + 7) / 8;
might be wrong, but for funky BitsPerCompontent values that should be trapped
earlier.  It looks like this calculation assumes pixels could span bytes, which
they dont IIRC.

[Bo Xu, 2014-08-14 21:49:17]

I agree should move |byte_cnt| up to track for |tag| right after it is used.

This is PNG decoding, so each row has a tag. Then the row_size in srcData is
indeed (row_size + 1), so the row_count is correct. (I was confused and wrong
with the row size in my first patch)

row_size = (Colors * BitsPerComponent * Columns + 7) / 8, should be correct,
what is your thinking on this? For a 24 bit RGB, a pixel can span 3 bytes.

I agree to refactor the switch statement, maybe do in another patch?

[Tom Sepez, 2014-08-15 16:51:30]

LGTM with nit.  Feel free to commit this once you address the nit without
re-review.  Thanks.

https://codereview.chromium.org/466153005/diff/40001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_flate.cpp (right):

https://codereview.chromium.org/466153005/diff/40001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_flate.cpp:412: byte_cnt++;
nit: As I read this, it would make more sense to me to put this at line 401,
right after the reference to srcData[0].  After all, it is the use of that byte
as a tag that is causing the need to increment the byte_cnt.  Keeping them
together might make this more obvious.

[Bo Xu, 2014-08-15 18:08:10]

https://codereview.chromium.org/466153005/diff/40001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_flate.cpp (right):

https://codereview.chromium.org/466153005/diff/40001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_flate.cpp:412: byte_cnt++;
If put byte_cnt++ on line 401, then when tag==0, line 409 has already increment
byte_cnt by an offset of 1, so it will be effectively an offset of 2. I think
then we have to do 
            byte_cnt += move_size
in line 409, right?

On 2014/08/15 16:51:30, Tom Sepez wrote:
> nit: As I read this, it would make more sense to me to put this at line 401,
> right after the reference to srcData[0].  After all, it is the use of that
byte
> as a tag that is causing the need to increment the byte_cnt.  Keeping them
> together might make this more obvious.

[Tom Sepez, 2014-08-15 19:02:13]

https://codereview.chromium.org/466153005/diff/40001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_flate.cpp (right):

https://codereview.chromium.org/466153005/diff/40001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_flate.cpp:412: byte_cnt++;
Yep. Good catch.

[Bo Xu, 2014-08-15 19:24:52]

Committed patchset #3 manually as d726307 (presubmit successful).