Bug 65128 – DFG JIT bytecode parser misuses pointers into objects allocated as part of a WTF::Vector

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

The DFG JIT bytecode parser gets a reference to a DFGNode in the DFGGraph, which is a subtype of WTF::Vector<DFGNode, 64>.  It then adds to the graph, and after adding to it, uses the reference several times, and while potentially adding to the graph again.  Adding to the graph means adding to the Vector, which in turn means that the Vector may resize its backing store.  When the backing store is resized, the old reference to the DFGNode in the Vector may become a dangling reference and subsequent uses may either corrupt memory, crash, or otherwise do bad things.  The DFG bytecode parser should not keep references to the innards of Vector alive after the Vector has been resized.

[Tim Horton]

<rdar://problem/9834708>

[Filip Pizlo]

Created attachment 101900 [details]
the patch

Still waiting for the relevant tests to finish running; when they do I'll change the review/commit-queue states to ?.

[Filip Pizlo]

Comment on attachment 101900 [details]
the patch

tests pass, ready for review

[Darin Adler]

Comment on attachment 101900 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101900&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1211
> +            phiNode = m_graph[entry.m_phi]; // reload after vector resize

This won’t do what you think it does!

It will copy the value from the new memory location into the old memory location.

You can’t re-point a reference to a new address with an assignment statement.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 101900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101900&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1211
> > +            phiNode = m_graph[entry.m_phi]; // reload after vector resize
> 
> This won’t do what you think it does!
> 
> It will copy the value from the new memory location into the old memory location.
> 
> You can’t re-point a reference to a new address with an assignment statement.

Good catch!  Fix on the way...

[Filip Pizlo]

Created attachment 101916 [details]
the patch (fixed for real this time)

This addresses the issue that Darin noticed.  Tests still running, will change review/commit-queue to ? once (if) they pass.

[Filip Pizlo]

Comment on attachment 101916 [details]
the patch (fixed for real this time)

Tests pass, ready for review.

[Gavin Barraclough]

Comment on attachment 101916 [details]
the patch (fixed for real this time)

Ooops, good point.

[WebKit Review Bot]

Comment on attachment 101916 [details]
the patch (fixed for real this time)

Clearing flags on attachment: 101916

Committed r91728: <http://trac.webkit.org/changeset/91728>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.