Use FX_SAFE_UINT32 on CPDF_ToUnicodeMap::Load

Use FX_SAFE_UINT32 on CPDF_ToUnicodeMap::Load

m_Map maps to unsigned integer, but m_MultiCharBuf.GetLength() returns
an integer. There will be integer overflow if the length is big, and
UBSAN will complain. Thus, using FX_SAFE_UINT32. Replacing with uint32
would work as well: the point is to consider the length as uint instead
of int.

BUG=chromium:652232
Committed: https://pdfium.googlesource.com/pdfium/+/89f9ee3b8f3b4756f05ff48055f4bff7353201e2

[npm, 2016-10-04 15:34:45]

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

[commit-bot: I haz the power, 2016-10-04 15:34:47]

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

[npm, 2016-10-04 15:35:06]

Description was changed from

==========
Use FX_SAFE_UINT32 on CPDF_ToUnicodeMap::Load

m_Map maps to unsigned integer, but m_MultiCharBuf.GetLength() returns
an integer. There will be integer overflow if the length is big, and
UBSAN will complain. Thus, using FX_SAFE_UINT32. Replacing with uint32
would work as well: the point is to consider the length as uint instead
of int.

BUG=chromium:652232
==========

to

==========
Use FX_SAFE_UINT32 on CPDF_ToUnicodeMap::Load

m_Map maps to unsigned integer, but m_MultiCharBuf.GetLength() returns
an integer. There will be integer overflow if the length is big, and
UBSAN will complain. Thus, using FX_SAFE_UINT32. Replacing with uint32
would work as well: the point is to consider the length as uint instead
of int.

BUG=chromium:652232
==========

[npm, 2016-10-04 15:35:06]

npm@chromium.org changed reviewers:
+ dsinclair@chromium.org, thestig@chromium.org, tsepez@chromium.org

[npm, 2016-10-04 15:47:36]

PTAL

[commit-bot: I haz the power, 2016-10-04 15:50:37]

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

[commit-bot: I haz the power, 2016-10-04 15:50:38]

Dry run: This issue passed the CQ dry run.

[dsinclair, 2016-10-04 17:15:31]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-10-04 17:15:31]

lgtm

https://codereview.chromium.org/2393573002/diff/1/core/fpdfapi/fpdf_font/fpdf...
File core/fpdfapi/fpdf_font/fpdf_font.cpp (right):

https://codereview.chromium.org/2393573002/diff/1/core/fpdfapi/fpdf_font/fpdf...
core/fpdfapi/fpdf_font/fpdf_font.cpp:288: FX_SAFE_UINT32 uni =
m_MultiCharBuf.GetLength();
Followup: These 6 lines are basically the same in the 3 chunks, can they be
factored out into a helper?

[commit-bot: I haz the power, 2016-10-04 17:15:40]

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-10-04 17:15:57]

Description was changed from

==========
Use FX_SAFE_UINT32 on CPDF_ToUnicodeMap::Load

m_Map maps to unsigned integer, but m_MultiCharBuf.GetLength() returns
an integer. There will be integer overflow if the length is big, and
UBSAN will complain. Thus, using FX_SAFE_UINT32. Replacing with uint32
would work as well: the point is to consider the length as uint instead
of int.

BUG=chromium:652232
==========

to

==========
Use FX_SAFE_UINT32 on CPDF_ToUnicodeMap::Load

m_Map maps to unsigned integer, but m_MultiCharBuf.GetLength() returns
an integer. There will be integer overflow if the length is big, and
UBSAN will complain. Thus, using FX_SAFE_UINT32. Replacing with uint32
would work as well: the point is to consider the length as uint instead
of int.

BUG=chromium:652232

Committed:
https://pdfium.googlesource.com/pdfium/+/89f9ee3b8f3b4756f05ff48055f4bff73532...
==========

[commit-bot: I haz the power, 2016-10-04 17:15:58]

Committed patchset #1 (id:1) as
https://pdfium.googlesource.com/pdfium/+/89f9ee3b8f3b4756f05ff48055f4bff73532...

[Tom Sepez, 2016-10-04 17:41:21]

https://codereview.chromium.org/2393573002/diff/1/core/fpdfapi/fpdf_font/fpdf...
File core/fpdfapi/fpdf_font/fpdf_font.cpp (right):

https://codereview.chromium.org/2393573002/diff/1/core/fpdfapi/fpdf_font/fpdf...
core/fpdfapi/fpdf_font/fpdf_font.cpp:232:
m_MultiCharBuf.AppendChar(destcode.GetLength());
Drive-by: do we prefer skipping/defaulting the value insteadof crashing here?

also uni = uni * 0x10000 + 0xffff; might be cleaner.

[npm, 2016-10-04 19:24:17]

https://codereview.chromium.org/2393573002/diff/1/core/fpdfapi/fpdf_font/fpdf...
File core/fpdfapi/fpdf_font/fpdf_font.cpp (right):

https://codereview.chromium.org/2393573002/diff/1/core/fpdfapi/fpdf_font/fpdf...
core/fpdfapi/fpdf_font/fpdf_font.cpp:232:
m_MultiCharBuf.AppendChar(destcode.GetLength());
On 2016/10/04 17:41:21, Tom Sepez wrote:
> Drive-by: do we prefer skipping/defaulting the value insteadof crashing here?
> 
> also uni = uni * 0x10000 + 0xffff; might be cleaner.

I would say we prefer not crashing whenever possible because crashing is the
worst possible user experience... But not sure, maybe dsinclair@ knows?

https://codereview.chromium.org/2393573002/diff/1/core/fpdfapi/fpdf_font/fpdf...
core/fpdfapi/fpdf_font/fpdf_font.cpp:288: FX_SAFE_UINT32 uni =
m_MultiCharBuf.GetLength();
On 2016/10/04 17:15:31, dsinclair wrote:
> Followup: These 6 lines are basically the same in the 3 chunks, can they be
> factored out into a helper?

Sounds good

[dsinclair, 2016-10-04 19:27:59]

https://codereview.chromium.org/2393573002/diff/1/core/fpdfapi/fpdf_font/fpdf...
File core/fpdfapi/fpdf_font/fpdf_font.cpp (right):

https://codereview.chromium.org/2393573002/diff/1/core/fpdfapi/fpdf_font/fpdf...
core/fpdfapi/fpdf_font/fpdf_font.cpp:232:
m_MultiCharBuf.AppendChar(destcode.GetLength());
On 2016/10/04 19:24:17, npm wrote:
> On 2016/10/04 17:41:21, Tom Sepez wrote:
> > Drive-by: do we prefer skipping/defaulting the value insteadof crashing
here?
> > 
> > also uni = uni * 0x10000 + 0xffff; might be cleaner.
> 
> I would say we prefer not crashing whenever possible because crashing is the
> worst possible user experience... But not sure, maybe dsinclair@ knows?


Do we know what a sensible default should be? If there is a sensible value we
can return that might be better.