Bug 111253

Summary: XSSAuditor has a subtle race condition when used with the threaded HTML parser
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dbates, dglazkov, eric, esprehn+autocc, ojan.autocc, philn, tonyg, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 106127    
Attachments:
Description Flags
Patch
none
Patch none

Adam Barth
Reported 2013-03-02 10:15:22 PST
XSSAuditor has a subtle race condition when used with the threaded HTML parser
Attachments
Patch (1.70 KB, patch)
2013-03-02 10:16 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (1.94 KB, patch)
2013-03-02 10:28 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2013-03-02 10:16:49 PST
Created attachment 191110 [details] Patch
WebKit Review Bot
Comment 2 2013-03-02 10:20:17 PST
Comment on attachment 191110 [details] Patch Attachment 191110 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16778921
Early Warning System Bot
Comment 3 2013-03-02 10:22:59 PST
Comment on attachment 191110 [details] Patch Attachment 191110 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16844192
EFL EWS Bot
Comment 4 2013-03-02 10:23:32 PST
Comment on attachment 191110 [details] Patch Attachment 191110 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/16864160
Early Warning System Bot
Comment 5 2013-03-02 10:24:31 PST
Comment on attachment 191110 [details] Patch Attachment 191110 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16864162
WebKit Review Bot
Comment 6 2013-03-02 10:25:15 PST
Comment on attachment 191110 [details] Patch Attachment 191110 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16885074
Adam Barth
Comment 7 2013-03-02 10:28:01 PST
Created attachment 191112 [details] Patch
Eric Seidel (no email)
Comment 8 2013-03-02 12:50:38 PST
Comment on attachment 191112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191112&action=review > Source/WebCore/html/parser/XSSAuditor.cpp:124 > + const String& attrName = name.namespaceURI() == XLinkNames::xlinkNamespaceURI ? "xlink:" + name.localName().string() : name.localName().string(); The temporary string created by "xlink:" + name.localName().string() goes away at the end of this line. :) This code will crash.
Adam Barth
Comment 9 2013-03-02 14:00:26 PST
If you bind a const reference to a temporary, then the temporary lives a long as the const ref.
Eric Seidel (no email)
Comment 11 2013-03-02 14:21:28 PST
(In reply to comment #9) > If you bind a const reference to a temporary, then the temporary lives a long as the const ref. WAT!?
Eric Seidel (no email)
Comment 12 2013-03-02 14:21:44 PST
C++ is even crazier than I thought.
Eric Seidel (no email)
Comment 13 2013-03-02 15:22:37 PST
Comment on attachment 191112 [details] Patch We should consider changing the names of thread safe functions like this one.
Daniel Bates
Comment 14 2013-03-02 15:24:11 PST
Comment on attachment 191112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191112&action=review >> Source/WebCore/html/parser/XSSAuditor.cpp:124 >> + const String& attrName = name.namespaceURI() == XLinkNames::xlinkNamespaceURI ? "xlink:" + name.localName().string() : name.localName().string(); > > The temporary string created by "xlink:" + name.localName().string() goes away at the end of this line. :) This code will crash. From my understanding of the C++ standard, this code is correct by properties of a temporary object, 12.2/5 ([class.temporary]); initialization of a reference type, 8.5.3/5 ([dcl.init.ref]); the lifetime of a reference type, 3.7/3 ([basic.stc]); and the definition of automatic storage duration in 3.7.3/1 ([basic.stc.auto]). For completeness the 11/02/2012 draft of the C++ standard (N3485) is at <http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2012/n3485.pdf>.
WebKit Review Bot
Comment 15 2013-03-02 17:47:57 PST
Comment on attachment 191112 [details] Patch Clearing flags on attachment: 191112 Committed r144549: <http://trac.webkit.org/changeset/144549>
WebKit Review Bot
Comment 16 2013-03-02 17:48:01 PST
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 17 2013-03-02 18:46:13 PST
(In reply to comment #11) > (In reply to comment #9) > > If you bind a const reference to a temporary, then the temporary lives a long as the const ref. > > WAT!? Surprised me when I learned about it too.
Note You need to log in before you can comment on or make changes to this bug.