Bug 64129 – Regression(probably 88308): Use-after-free in CounterNode::insertAfter

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Abhishek Arya]

credit: miaubiz
http://code.google.com/p/chromium/issues/detail?id=88216

VULNERABILITY DETAILS
counter related use after free

without memory debugging tools it just hangs forever, asan and vg say it's use after free.

VERSION
Chrome Version: daily + canary + trunk
Operating System: linux 64bit, win7

bisect-builds puts it here:
http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=88127:88339
probably the webkit roll to 88326

REPRODUCTION CASE
attached

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: renderer/hang
Crash State: 
Invalid read of size 8
   at 0x1D289C8: WebCore::CounterNode::insertAfter(WebCore::CounterNode*, WebCore::CounterNode*, WTF::AtomicString const&) (CounterNode.cpp:202)
 Address 0xf4c45a0 is 16 bytes inside a block of size 72 free'd

TESTCASE
<style>
  div::after {
    content:counter(ctr);
    counter-increment:ctr;
  }
</style>
<div></div>
<div>
  <div></div>
</div>
<style>
  div { counter-reset:ctr; }
</style>

Checking out the webkit range from the regression, looks like it might be coming from http://trac.webkit.org/changeset/88308 (which makes changes to content property and the repro has it too :). I cannot see anything else counter node specific in the range. Counter Node are the trouble of past, present and future for security because of CounterDirectiveMap and cloning it might be causing this.

Simon, can you please take a look.

[Lucas Forschler]

<rdar://problem/9745926>

[Cris Neckar]

Created attachment 102305 [details]
Minimized repro

[Cris Neckar]

I will try to describe the issue here although this is rather complex.

Essentially the way CounterNode trees are created relies on makeCounterNode() being called in the order that the render objects fall in the render tree.

When we have a dom such as

<table>
  <td>
    <a>
  </td>
  <b>
</table>

The nodes are created in the order a then b. However, because b is not within a table section it will be moved out of the table resulting an an actual hierarchy of

<b>
<table>
  <td>
    <a>
  </td>
</table>

When counters exist on both a and b the counter node for a is created first and then will later have to become a child of b. The code does not handle this and with a slightly more complicated tree you end up with situations where nodes with different parents are linked as siblings. This code is incredibly fragile to dom mutation and in many cases this ends up causing CounterNodes to be destroyed without being unlinked from the tree. The correct fix for this bug is to either make the existing code handle complex mutations or to reimplement css counters. I believe they are currently very slow so I would tend to lean towards the second alternative. 

In the meantime there is a relatively simple way to ensure that other situations like this do not result in the use of stale pointers. Within the destructor for CounterNode we can check whether the node is still linked into the tree and if so unlink it. In the general case (anything that doesn't currently result in a stale pointer) this will involve 5 null checks in the destructor. In cases where stale pointers are currently caused a somewhat complex safe unlinking must occur. This fix will turn this and other similar bugs into purely functional bugs with no security impact.

[Cris Neckar]

This no longer has security implications after http://trac.webkit.org/changeset/92630

However it is still a functional bug. Assuming this is still open once the above revision rolls into relevant projects the product can be changed from "Security" to "WebKit".

[Eric Seidel (no email)]

Dropping Security per above comment.

[Alexey Proskuryakov]

Could you please re-title the bug to describe what it tracks now?

[Abhishek Arya]

I think Elliot is working on the Counter Node rewrite. so this bug does not need individual tracking.