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.
<rdar://problem/9834708>
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 ?.
Comment on attachment 101900 [details] the patch tests pass, ready for review
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.
(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...
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.
Comment on attachment 101916 [details] the patch (fixed for real this time) Tests pass, ready for review.
Comment on attachment 101916 [details] the patch (fixed for real this time) Ooops, good point.
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>
All reviewed patches have been landed. Closing bug.