Detect resursive loading of type3 font char to avoid infinite loop

Detect resursive loading of type3 font char to avoid infinite loop

The original way of detecting loops was passing a level parameter
through various functions. This missed some cases which also lead
to load type3 font char, for example, FindFont() may call
CheckType3FontMetrics() which may eventually lead to LoadChar().

The new way is to store the char loading depth, and abort when the depth
exceeds the max.

BUG=chromium:651304
Committed: https://pdfium.googlesource.com/pdfium/+/d61f958385be285f3f3897ef3a3f010048608f1c

[Wei Li, 2016-09-30 21:46:40]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-10-01 00:42:44]

Patchset #2 (id:40001) has been deleted

[Wei Li, 2016-10-01 00:43:43]

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

[commit-bot: I haz the power, 2016-10-01 00:43:46]

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

[Wei Li, 2016-10-01 00:51:20]

weili@chromium.org changed reviewers:
+ dsinclair@chromium.org, thestig@chromium.org

[Wei Li, 2016-10-01 00:51:21]

pls review, thanks

[Lei Zhang, 2016-10-01 00:53:55]

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

[commit-bot: I haz the power, 2016-10-01 00:55:36]

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

[commit-bot: I haz the power, 2016-10-01 00:55:37]

Dry run: This issue passed the CQ dry run.

[dsinclair, 2016-10-03 13:01:03]

https://codereview.chromium.org/2384853002/diff/60001/core/fpdfapi/fpdf_font/...
File core/fpdfapi/fpdf_font/cpdf_type3font.cpp (left):

https://codereview.chromium.org/2384853002/diff/60001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_type3font.cpp:91: if (level >=
_FPDF_MAX_TYPE3_FORM_LEVEL_)
I don't see any other uses, can the _FPDF_MAX_TYPE3_FORM_LEVEL define be removed
as well?

https://codereview.chromium.org/2384853002/diff/60001/core/fpdfapi/fpdf_font/...
File core/fpdfapi/fpdf_font/cpdf_type3font.cpp (right):

https://codereview.chromium.org/2384853002/diff/60001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_type3font.cpp:92: if (m_bCharLoading)
Do we need to allow a certain level of recursion here? Previously this would
allow us to enter 4 times, now it's once. Does that change matter?

[Wei Li, 2016-10-03 18:42:17]

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

[commit-bot: I haz the power, 2016-10-03 18:42:23]

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-10-03 18:54:15]

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

[commit-bot: I haz the power, 2016-10-03 18:54:16]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2016-10-03 18:56:31]

ptal, thanks

https://codereview.chromium.org/2384853002/diff/60001/core/fpdfapi/fpdf_font/...
File core/fpdfapi/fpdf_font/cpdf_type3font.cpp (left):

https://codereview.chromium.org/2384853002/diff/60001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_type3font.cpp:91: if (level >=
_FPDF_MAX_TYPE3_FORM_LEVEL_)
On 2016/10/03 13:01:02, dsinclair wrote:
> I don't see any other uses, can the _FPDF_MAX_TYPE3_FORM_LEVEL define be
removed
> as well?

N/A now

https://codereview.chromium.org/2384853002/diff/60001/core/fpdfapi/fpdf_font/...
File core/fpdfapi/fpdf_font/cpdf_type3font.cpp (right):

https://codereview.chromium.org/2384853002/diff/60001/core/fpdfapi/fpdf_font/...
core/fpdfapi/fpdf_font/cpdf_type3font.cpp:92: if (m_bCharLoading)
On 2016/10/03 13:01:02, dsinclair wrote:
> Do we need to allow a certain level of recursion here? Previously this would
> allow us to enter 4 times, now it's once. Does that change matter?

A font should not depends on itself, that may cause a parsing loop. But I am not
sure whether any peculiar cases exist. So, let's keep the change similar to the
prior.

[Wei Li, 2016-10-03 18:57:26]

Description was changed from

==========
Detect resursive loading of type3 font char to avoid infinite loop

The original way of detecting loops was passing a level parameter
through various functions. This missed some cases which also lead
to load type3 font char, for example, FindFont() may call
CheckType3FontMetrics() which may eventually lead to LoadChar().

The new way is to store the char loading state, and abort when a loop
is detected.

BUG=chromium:651304
==========

to

==========
Detect resursive loading of type3 font char to avoid infinite loop

The original way of detecting loops was passing a level parameter
through various functions. This missed some cases which also lead
to load type3 font char, for example, FindFont() may call
CheckType3FontMetrics() which may eventually lead to LoadChar().

The new way is to store the char loading depth, and abort when the depth
exceeds the max.

BUG=chromium:651304
==========

[dsinclair, 2016-10-03 19:10:33]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-10-03 19:10:34]

lgtm

[commit-bot: I haz the power, 2016-10-03 19:10:46]

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-03 19:10:58]

Description was changed from

==========
Detect resursive loading of type3 font char to avoid infinite loop

The original way of detecting loops was passing a level parameter
through various functions. This missed some cases which also lead
to load type3 font char, for example, FindFont() may call
CheckType3FontMetrics() which may eventually lead to LoadChar().

The new way is to store the char loading depth, and abort when the depth
exceeds the max.

BUG=chromium:651304
==========

to

==========
Detect resursive loading of type3 font char to avoid infinite loop

The original way of detecting loops was passing a level parameter
through various functions. This missed some cases which also lead
to load type3 font char, for example, FindFont() may call
CheckType3FontMetrics() which may eventually lead to LoadChar().

The new way is to store the char loading depth, and abort when the depth
exceeds the max.

BUG=chromium:651304

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

[commit-bot: I haz the power, 2016-10-03 19:10:58]

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