Make the XML entity recursion check more precise.

Make the XML entity recursion check more precise.

libxml doesn't detect entity recursion specifically but has a variety
of related checks, such as entities not expanding too deeply or
producing exponential blow-ups in content.

Because entity declarations are parsed in a separate context with
their own element recursion budget, a recursive entity can overflow
the stack using a lot of open elements (but within the per-context
limit) as it slowly consumes (but does not exhaust) the entity depth
budget.

This adds a specific, precise check for recursive entities that
detects entity recursion specifically and fails immediately.

The existing entity expansion depth checks are still relevant for long
chains of different entities.

BUG=628581
Committed: https://crrev.com/acca03bd98815a12daf812471f649041f5381571
Cr-Commit-Position: refs/heads/master@{#436899}

[dominicc (has gone to gerrit), 2016-11-30 07:50:16]

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

[commit-bot: I haz the power, 2016-11-30 07:51:02]

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

[dominicc (has gone to gerrit), 2016-11-30 07:51:30]

dominicc@chromium.org changed reviewers:
+ scottmg@chromium.org

[dominicc (has gone to gerrit), 2016-11-30 07:51:30]

PTAL

[commit-bot: I haz the power, 2016-11-30 11:25:58]

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

[commit-bot: I haz the power, 2016-11-30 11:25:59]

Dry run: This issue passed the CQ dry run.

[scottmg, 2016-11-30 16:22:58]

lgtm with a fix to copy (or just explicit initialization if you don't think it
makes sense to copy the state of |guard|).

https://codereview.chromium.org/2539003002/diff/1/third_party/libxml/src/enti...
File third_party/libxml/src/entities.c (right):

https://codereview.chromium.org/2539003002/diff/1/third_party/libxml/src/enti...
third_party/libxml/src/entities.c:935: return(cur);
->guard is not copied here.

[ddkilzer, 2016-11-30 23:30:22]

https://codereview.chromium.org/2539003002/diff/1/third_party/libxml/src/incl...
File third_party/libxml/src/include/libxml/entities.h (right):

https://codereview.chromium.org/2539003002/diff/1/third_party/libxml/src/incl...
third_party/libxml/src/include/libxml/entities.h:68: xmlEntityRecursionGuard
guard;
FYI, changing struct _xmlEntity (which is vended as typedef 'xmlEntity' in
libxml2 public headers) will cause binary compatibility issues for platforms
that ship libxml2 as a public library.

Obviously this isn't an issue for Chrome if it uses its own copy of libxml2
internally, but it means that this fix can't be upstreamed as-is, and if this
bug is fixed upstream, it will use a different approach.

[dominicc (has gone to gerrit), 2016-12-01 01:43:42]

On 2016/11/30 at 23:30:22, ddkilzer wrote:
>
https://codereview.chromium.org/2539003002/diff/1/third_party/libxml/src/incl...
> File third_party/libxml/src/include/libxml/entities.h (right):
> 
>
https://codereview.chromium.org/2539003002/diff/1/third_party/libxml/src/incl...
> third_party/libxml/src/include/libxml/entities.h:68: xmlEntityRecursionGuard
guard;
> FYI, changing struct _xmlEntity (which is vended as typedef 'xmlEntity' in
libxml2 public headers) will cause binary compatibility issues for platforms
that ship libxml2 as a public library.
> 
> Obviously this isn't an issue for Chrome if it uses its own copy of libxml2
internally, but it means that this fix can't be upstreamed as-is, and if this
bug is fixed upstream, it will use a different approach.

That's a good point ddkilzer. I suppose the same applies to the parser context.
Possibly the C stack could carry around the set of entities being expanded,
although the runtime complexity of checking that will be a problem.

I'm not clear on when libxml2 will make callbacks. Maybe I could use a special
bit pattern in _xmlEntity::checked and clients will be none the wiser.

[dominicc (has gone to gerrit), 2016-12-07 07:13:24]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-12-07 07:13:24]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2539003002/#ps20001
(title: "Feedback.")

[commit-bot: I haz the power, 2016-12-07 07:13:39]

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-12-07 09:39:47]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481094804301580,
"parent_rev": "e00843c54f19bd084d730756947502e9e2e96a53", "commit_rev":
"1d1eef81f9ec8adcbd3a0174738a002308164c81"}

[commit-bot: I haz the power, 2016-12-07 09:40:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-07 09:43:07]

Description was changed from

==========
Make the XML entity recursion check more precise.

libxml doesn't detect entity recursion specifically but has a variety
of related checks, such as entities not expanding too deeply or
producing exponential blow-ups in content.

Because entity declarations are parsed in a separate context with
their own element recursion budget, a recursive entity can overflow
the stack using a lot of open elements (but within the per-context
limit) as it slowly consumes (but does not exhaust) the entity depth
budget.

This adds a specific, precise check for recursive entities that
detects entity recursion specifically and fails immediately.

The existing entity expansion depth checks are still relevant for long
chains of different entities.

BUG=628581
==========

to

==========
Make the XML entity recursion check more precise.

libxml doesn't detect entity recursion specifically but has a variety
of related checks, such as entities not expanding too deeply or
producing exponential blow-ups in content.

Because entity declarations are parsed in a separate context with
their own element recursion budget, a recursive entity can overflow
the stack using a lot of open elements (but within the per-context
limit) as it slowly consumes (but does not exhaust) the entity depth
budget.

This adds a specific, precise check for recursive entities that
detects entity recursion specifically and fails immediately.

The existing entity expansion depth checks are still relevant for long
chains of different entities.

BUG=628581

Committed: https://crrev.com/acca03bd98815a12daf812471f649041f5381571
Cr-Commit-Position: refs/heads/master@{#436899}
==========

[commit-bot: I haz the power, 2016-12-07 09:43:08]

Patchset 2 (id:??) landed as
https://crrev.com/acca03bd98815a12daf812471f649041f5381571
Cr-Commit-Position: refs/heads/master@{#436899}