Fix for #618267. Adding a method to determine if multiplication has

Fix for #618267. Adding a method to determine if multiplication has
overflow.

BUG=618267
Committed: https://pdfium.googlesource.com/pdfium/+/dda2c0dc502b50d4de66b80305441bfb612ec6c1

[tracy_jiang, 2016-08-26 22:49:30]

The CQ bit was checked by tracy_jiang@foxitsoftware.com to run a CQ dry run

[tracy_jiang, 2016-08-26 22:49:35]

The CQ bit was unchecked by tracy_jiang@foxitsoftware.com

[tracy_jiang, 2016-08-26 22:49:58]

The CQ bit was checked by tracy_jiang@foxitsoftware.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 22:50:00]

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-26 22:50:01]

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

[commit-bot: I haz the power, 2016-08-26 22:50:02]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-pdfium-committers".
Note that this has nothing to do with OWNERS files.

[tracy_jiang, 2016-08-26 22:51:27]

The CQ bit was checked by tracy_jiang@foxitsoftware.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 22:51:29]

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-26 22:51:30]

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

[commit-bot: I haz the power, 2016-08-26 22:51:31]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-pdfium-committers".
Note that this has nothing to do with OWNERS files.

[tracy_jiang, 2016-08-26 22:54:35]

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

[tracy_jiang, 2016-08-26 22:54:36]

Dear all,

This is a Foxit fix for #618267. PTAL.

Best Regards,
-Tracy

[Tom Sepez, 2016-08-26 23:05:29]

Thanks.  This will work.

This bug shows why sizes should be unsigned ... the optimizer can only kill the
existing check because of signed types.

https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
core/fxcodec/codec/fx_codec_tiff.cpp:82: int
_TIFFIfMultiplicationOverflow(tmsize_t op1, tmsize_t op2) {
Should be bool return value.  Also, we return true when it doesn't overflow
(right?), which is opposite of what the name suggests to me.  Maybe
_TIFFMultiplicationSafeFromOverflow() or some such?

https://codereview.chromium.org/2284063002/diff/1/third_party/libtiff/tif_aux.c
File third_party/libtiff/tif_aux.c (right):

https://codereview.chromium.org/2284063002/diff/1/third_party/libtiff/tif_aux...
third_party/libtiff/tif_aux.c:72: if (nmemb && elem_size &&
_TIFFIfMultiplicationOverflow(nmemb, elem_size))//&& bytes / elem_size == nmemb
remove the code following the //

[tracy_jiang, 2016-08-26 23:13:02]

On 2016/08/26 23:05:29, Tom Sepez wrote:
> Thanks.  This will work.
> 
> This bug shows why sizes should be unsigned ... the optimizer can only kill
the
> existing check because of signed types.
> 
>
https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
> File core/fxcodec/codec/fx_codec_tiff.cpp (right):
> 
>
https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
> core/fxcodec/codec/fx_codec_tiff.cpp:82: int
> _TIFFIfMultiplicationOverflow(tmsize_t op1, tmsize_t op2) {
> Should be bool return value.  Also, we return true when it doesn't overflow
> (right?), which is opposite of what the name suggests to me.  Maybe
> _TIFFMultiplicationSafeFromOverflow() or some such?
> 
>
https://codereview.chromium.org/2284063002/diff/1/third_party/libtiff/tif_aux.c
> File third_party/libtiff/tif_aux.c (right):
> 
>
https://codereview.chromium.org/2284063002/diff/1/third_party/libtiff/tif_aux...
> third_party/libtiff/tif_aux.c:72: if (nmemb && elem_size &&
> _TIFFIfMultiplicationOverflow(nmemb, elem_size))//&& bytes / elem_size ==
nmemb
> remove the code following the //

Dear Tom,

we already uploaded a second patch set removing the unnecessary comments.

-Tracy

[Tom Sepez, 2016-08-26 23:19:32]

On 2016/08/26 23:13:02, tracy_jiang wrote:
> On 2016/08/26 23:05:29, Tom Sepez wrote:
> > Thanks.  This will work.
> > 
> > This bug shows why sizes should be unsigned ... the optimizer can only kill
> the
> > existing check because of signed types.
> > 
> >
>
https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
> > File core/fxcodec/codec/fx_codec_tiff.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
> > core/fxcodec/codec/fx_codec_tiff.cpp:82: int
> > _TIFFIfMultiplicationOverflow(tmsize_t op1, tmsize_t op2) {
> > Should be bool return value.  Also, we return true when it doesn't overflow
> > (right?), which is opposite of what the name suggests to me.  Maybe
> > _TIFFMultiplicationSafeFromOverflow() or some such?
> > 
> >
>
https://codereview.chromium.org/2284063002/diff/1/third_party/libtiff/tif_aux.c
> > File third_party/libtiff/tif_aux.c (right):
> > 
> >
>
https://codereview.chromium.org/2284063002/diff/1/third_party/libtiff/tif_aux...
> > third_party/libtiff/tif_aux.c:72: if (nmemb && elem_size &&
> > _TIFFIfMultiplicationOverflow(nmemb, elem_size))//&& bytes / elem_size ==
> nmemb
> > remove the code following the //
> 
> Dear Tom,
> 
> we already uploaded a second patch set removing the unnecessary comments.
> 
> -Tracy

Ah, good.  What about the naming issue I mentioned?

[tracy_jiang, 2016-08-26 23:44:16]

https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
core/fxcodec/codec/fx_codec_tiff.cpp:82: int
_TIFFIfMultiplicationOverflow(tmsize_t op1, tmsize_t op2) {
On 2016/08/26 23:05:28, Tom Sepez wrote:
> Should be bool return value.  Also, we return true when it doesn't overflow
> (right?), which is opposite of what the name suggests to me.  Maybe
> _TIFFMultiplicationSafeFromOverflow() or some such?

We will change the naming logic. It made sense to us.
The reason we used int was that this method is declared in
third_party/libtiff/tiffio.h. tiffio.h holds a bunch of 
C function signatures and will not allow 'bool' to be compiled
We declared it in tiffio.h was because other tiff auxiliary
methods are declared in the same place like _TIFFmemcpy etc.
Also in libtiff there is not a thing similar to FX_BOOL in
non-third-party places.
So do you think we should use int here or should we typedef a 
TIFF_BOOL?

[Tom Sepez, 2016-08-26 23:54:50]

On 2016/08/26 23:44:16, tracy_jiang wrote:
>
https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
> File core/fxcodec/codec/fx_codec_tiff.cpp (right):
> 
>
https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
> core/fxcodec/codec/fx_codec_tiff.cpp:82: int
> _TIFFIfMultiplicationOverflow(tmsize_t op1, tmsize_t op2) {
> On 2016/08/26 23:05:28, Tom Sepez wrote:
> > Should be bool return value.  Also, we return true when it doesn't overflow
> > (right?), which is opposite of what the name suggests to me.  Maybe
> > _TIFFMultiplicationSafeFromOverflow() or some such?
> 
> We will change the naming logic. It made sense to us.
> The reason we used int was that this method is declared in
> third_party/libtiff/tiffio.h. tiffio.h holds a bunch of 
> C function signatures and will not allow 'bool' to be compiled
> We declared it in tiffio.h was because other tiff auxiliary
> methods are declared in the same place like _TIFFmemcpy etc.
> Also in libtiff there is not a thing similar to FX_BOOL in
> non-third-party places.
> So do you think we should use int here or should we typedef a 
> TIFF_BOOL?

int will be just fine in that case.  thanks.

[tracy_jiang, 2016-08-27 00:12:08]

On 2016/08/26 23:54:50, Tom Sepez wrote:
> On 2016/08/26 23:44:16, tracy_jiang wrote:
> >
>
https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
> > File core/fxcodec/codec/fx_codec_tiff.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2284063002/diff/1/core/fxcodec/codec/fx_codec...
> > core/fxcodec/codec/fx_codec_tiff.cpp:82: int
> > _TIFFIfMultiplicationOverflow(tmsize_t op1, tmsize_t op2) {
> > On 2016/08/26 23:05:28, Tom Sepez wrote:
> > > Should be bool return value.  Also, we return true when it doesn't
overflow
> > > (right?), which is opposite of what the name suggests to me.  Maybe
> > > _TIFFMultiplicationSafeFromOverflow() or some such?
> > 
> > We will change the naming logic. It made sense to us.
> > The reason we used int was that this method is declared in
> > third_party/libtiff/tiffio.h. tiffio.h holds a bunch of 
> > C function signatures and will not allow 'bool' to be compiled
> > We declared it in tiffio.h was because other tiff auxiliary
> > methods are declared in the same place like _TIFFmemcpy etc.
> > Also in libtiff there is not a thing similar to FX_BOOL in
> > non-third-party places.
> > So do you think we should use int here or should we typedef a 
> > TIFF_BOOL?
> 
> int will be just fine in that case.  thanks.

Tom,

I kept the name but changed the logic, thinking this way is more readable.
PTAL at the 3rd patch set.

-Tracy

[Tom Sepez, 2016-08-29 16:20:50]

Code is fine, can you create a .patch file and update README.pdfium in the
libtiff directory?

[tracy_jiang, 2016-08-29 20:05:13]

On 2016/08/29 16:20:50, Tom Sepez wrote:
> Code is fine, can you create a .patch file and update README.pdfium in the
> libtiff directory?

We added them in patch set 4. PTAL.

[Tom Sepez, 2016-08-29 20:25:49]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-08-29 20:25:50]

lgtm

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

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-29 20:42:59]

Description was changed from

==========
Fix for #618267. Adding a method to determine if multiplication has
overflow.

BUG=618267
==========

to

==========
Fix for #618267. Adding a method to determine if multiplication has
overflow.

BUG=618267

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

[commit-bot: I haz the power, 2016-08-29 20:43:01]

Committed patchset #4 (id:60001) as
https://pdfium.googlesource.com/pdfium/+/dda2c0dc502b50d4de66b80305441bfb612e...