XSSAuditor has a subtle race condition when used with the threaded HTML parser
Created attachment 191110 [details] Patch
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 on attachment 191110 [details] Patch Attachment 191110 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16844192
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 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 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
Created attachment 191112 [details] Patch
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.
If you bind a const reference to a temporary, then the temporary lives a long as the const ref.
http://www.informit.com/guides/content.aspx?g=cplusplus&seqNum=199
(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!?
C++ is even crazier than I thought.
Comment on attachment 191112 [details] Patch We should consider changing the names of thread safe functions like this one.
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 on attachment 191112 [details] Patch Clearing flags on attachment: 191112 Committed r144549: <http://trac.webkit.org/changeset/144549>
All reviewed patches have been landed. Closing bug.
(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.