Bug 65128 - DFG JIT bytecode parser misuses pointers into objects allocated as part of a WTF::Vector
Summary: DFG JIT bytecode parser misuses pointers into objects allocated as part of a ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-07-25 13:17 PDT by Filip Pizlo
Modified: 2011-07-25 17:25 PDT (History)
4 users (show)

See Also:


Attachments
the patch (2.25 KB, patch)
2011-07-25 13:22 PDT, Filip Pizlo
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
the patch (fixed for real this time) (3.94 KB, patch)
2011-07-25 14:57 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-07-25 13:17:13 PDT
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.
Comment 1 Tim Horton 2011-07-25 13:19:42 PDT
<rdar://problem/9834708>
Comment 2 Filip Pizlo 2011-07-25 13:22:53 PDT
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 3 Filip Pizlo 2011-07-25 13:30:48 PDT
Comment on attachment 101900 [details]
the patch

tests pass, ready for review
Comment 4 Darin Adler 2011-07-25 14:46:48 PDT
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.
Comment 5 Filip Pizlo 2011-07-25 14:47:33 PDT
(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...
Comment 6 Filip Pizlo 2011-07-25 14:57:02 PDT
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 7 Filip Pizlo 2011-07-25 15:04:17 PDT
Comment on attachment 101916 [details]
the patch (fixed for real this time)

Tests pass, ready for review.
Comment 8 Gavin Barraclough 2011-07-25 15:49:30 PDT
Comment on attachment 101916 [details]
the patch (fixed for real this time)

Ooops, good point.
Comment 9 WebKit Review Bot 2011-07-25 17:25:43 PDT
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>
Comment 10 WebKit Review Bot 2011-07-25 17:25:48 PDT
All reviewed patches have been landed.  Closing bug.