Bound total pixels in JBig2 images to avoid overflows later.

Bound total pixels in JBig2 images to avoid overflows later.

Also make these private to ensure they aren't modified so as to
violate the bounds checks applied at creation time.

BUG=633002
Committed: https://pdfium.googlesource.com/pdfium/+/e21501d9427539828b5d547b9d20a752d06914aa

[Tom Sepez, 2016-08-01 22:04:51]

Description was changed from

==========
Bound pixels in JBig2 images to avoid overflows later.

Also make these private to ensure they aren't modified so as to
violate the bounds checks applied at creation time.

BUG=633002
==========

to

==========
Bound pixels in JBig2 images to avoid overflows later.

Also make these private to ensure they aren't modified so as to
violate the bounds checks applied at creation time.

BUG=633002
==========

[Tom Sepez, 2016-08-01 22:04:51]

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

[Tom Sepez, 2016-08-01 22:08:08]

Lei, for review.

[Tom Sepez, 2016-08-01 22:08:26]

Description was changed from

==========
Bound pixels in JBig2 images to avoid overflows later.

Also make these private to ensure they aren't modified so as to
violate the bounds checks applied at creation time.

BUG=633002
==========

to

==========
Bound total pixels in JBig2 images to avoid overflows later.

Also make these private to ensure they aren't modified so as to
violate the bounds checks applied at creation time.

BUG=633002
==========

[Lei Zhang, 2016-08-01 22:49:50]

lgtm with nits.

https://codereview.chromium.org/2202013002/diff/1/core/fxcodec/jbig2/JBig2_Im...
File core/fxcodec/jbig2/JBig2_Image.cpp (right):

https://codereview.chromium.org/2202013002/diff/1/core/fxcodec/jbig2/JBig2_Im...
core/fxcodec/jbig2/JBig2_Image.cpp:18: "Pixel calculatons will overflow");
Are calculatons cousins to Calculon?

https://codereview.chromium.org/2202013002/diff/1/core/fxcodec/jbig2/JBig2_Im...
File core/fxcodec/jbig2/JBig2_Image.h (right):

https://codereview.chromium.org/2202013002/diff/1/core/fxcodec/jbig2/JBig2_Im...
core/fxcodec/jbig2/JBig2_Image.h:78: FX_BOOL m_bNeedFree;
const bool

[Tom Sepez, 2016-08-01 23:21:07]

Lei, re-review required for maximum value change and expand().

[Tom Sepez, 2016-08-01 23:21:13]

https://codereview.chromium.org/2202013002/diff/1/core/fxcodec/jbig2/JBig2_Im...
File core/fxcodec/jbig2/JBig2_Image.cpp (right):

https://codereview.chromium.org/2202013002/diff/1/core/fxcodec/jbig2/JBig2_Im...
core/fxcodec/jbig2/JBig2_Image.cpp:18: "Pixel calculatons will overflow");
On 2016/08/01 22:49:50, Lei Zhang wrote:
> Are calculatons cousins to Calculon?

indeed.  sadly this got removed and replaced by just limiting pixels to int_max
- 31.

https://codereview.chromium.org/2202013002/diff/1/core/fxcodec/jbig2/JBig2_Im...
File core/fxcodec/jbig2/JBig2_Image.h (right):

https://codereview.chromium.org/2202013002/diff/1/core/fxcodec/jbig2/JBig2_Im...
core/fxcodec/jbig2/JBig2_Image.h:78: FX_BOOL m_bNeedFree;
On 2016/08/01 22:49:50, Lei Zhang wrote:
> const bool

Adding this "const" still compiles, but it shouldn't, since the 
expand() method, if implemented proplery, might have to change the setting when
it makes a new buffer.

But it's not implemented properly.

I think you can create an image w/ external buffer, and then expand it in the
h_direction, which might cause an alloc.  But it doesn't set the flag properly
in that case. Instead, it then causes realloc to be invoked on a the external
buffer, which may be non-heap memory.  Yay. 

Given this case, if it reallocs, we leak the newly returned buffer, and make the
caller's ptr invalid.

argh.

[Tom Sepez, 2016-08-01 23:22:33]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 23:22:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-08-01 23:34:28]

On 2016/08/01 23:21:13, Tom Sepez wrote:
> argh.

lgtm, but argh indeed. Clearly needs more unit tests.

[commit-bot: I haz the power, 2016-08-01 23:37:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 23:37:17]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-08-02 19:28:25]

On 2016/08/01 23:34:28, Lei Zhang wrote:
> On 2016/08/01 23:21:13, Tom Sepez wrote:
> > argh.
> 
> lgtm, but argh indeed. Clearly needs more unit tests.
And so it shall be, though just barely, but enough to catch the bogus free I
fixed.
Please re-review 'unittest.cpp

[Lei Zhang, 2016-08-02 19:36:56]

lgtm

https://codereview.chromium.org/2202013002/diff/80001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_Image_unittest.cpp (right):

https://codereview.chromium.org/2202013002/diff/80001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Image_unittest.cpp:37: EXPECT_EQ(nullptr, img.m_pData);
You can also do EXPECT_FALSE(ptr);

[Tom Sepez, 2016-08-02 20:03:58]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 20:04:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 20:25:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 20:25:56]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-08-02 20:35:56]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2016-08-02 20:36:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 20:36:20]

Description was changed from

==========
Bound total pixels in JBig2 images to avoid overflows later.

Also make these private to ensure they aren't modified so as to
violate the bounds checks applied at creation time.

BUG=633002
==========

to

==========
Bound total pixels in JBig2 images to avoid overflows later.

Also make these private to ensure they aren't modified so as to
violate the bounds checks applied at creation time.

BUG=633002

Committed:
https://pdfium.googlesource.com/pdfium/+/e21501d9427539828b5d547b9d20a752d069...
==========

[commit-bot: I haz the power, 2016-08-02 20:36:20]

Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/e21501d9427539828b5d547b9d20a752d069...

[noahman, 2016-10-24 14:26:27]

On 2016/08/02 20:36:20, commit-bot: I haz the power wrote:
> Committed patchset #5 (id:80001) as
>
https://pdfium.googlesource.com/pdfium/+/e21501d9427539828b5d547b9d20a752d069...

Here is also possible to track the issue: http://www.buythesisonline.org. Added
back up copy to my project available in the account.