Bug 111253 - XSSAuditor has a subtle race condition when used with the threaded HTML parser
Summary: XSSAuditor has a subtle race condition when used with the threaded HTML parser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 106127
  Show dependency treegraph
 
Reported: 2013-03-02 10:15 PST by Adam Barth
Modified: 2013-03-02 18:46 PST (History)
11 users (show)

See Also:


Attachments
Patch (1.70 KB, patch)
2013-03-02 10:16 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (1.94 KB, patch)
2013-03-02 10:28 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2013-03-02 10:15:22 PST
XSSAuditor has a subtle race condition when used with the threaded HTML parser
Comment 1 Adam Barth 2013-03-02 10:16:49 PST
Created attachment 191110 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 Early Warning System Bot 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
Comment 4 EFL EWS Bot 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
Comment 5 Early Warning System Bot 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
Comment 6 WebKit Review Bot 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
Comment 7 Adam Barth 2013-03-02 10:28:01 PST
Created attachment 191112 [details]
Patch
Comment 8 Eric Seidel (no email) 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.
Comment 9 Adam Barth 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.
Comment 11 Eric Seidel (no email) 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!?
Comment 12 Eric Seidel (no email) 2013-03-02 14:21:44 PST
C++ is even crazier than I thought.
Comment 13 Eric Seidel (no email) 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.
Comment 14 Daniel Bates 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>.
Comment 15 WebKit Review Bot 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>
Comment 16 WebKit Review Bot 2013-03-02 17:48:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Darin Adler 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.