Bug 111253 – XSSAuditor has a subtle race condition when used with the threaded HTML parser

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

XSSAuditor has a subtle race condition when used with the threaded HTML parser

[Adam Barth]

Created attachment 191110 [details]
Patch

[WebKit Review Bot]

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 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 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 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 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]

Created attachment 191112 [details]
Patch

[Eric Seidel (no email)]

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]

If you bind a const reference to a temporary, then the temporary lives a long as the const ref.

[Adam Barth]

http://www.informit.com/guides/content.aspx?g=cplusplus&seqNum=199

[Eric Seidel (no email)]

(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)]

C++ is even crazier than I thought.

[Eric Seidel (no email)]

Comment on attachment 191112 [details]
Patch

We should consider changing the names of thread safe functions like this one.

[Daniel Bates]

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 on attachment 191112 [details]
Patch

Clearing flags on attachment: 191112

Committed r144549: <http://trac.webkit.org/changeset/144549>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(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.