111253 2013-03-02 10:15:22 -0800 XSSAuditor has a subtle race condition when used with the threaded HTML parser 2013-03-02 18:46:13 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 106127 1 abarth abarth darin dbates dglazkov eric esprehn+autocc ojan.autocc philn tonyg webkit-ews webkit.review.bot xan.lopez oldest_to_newest 845817 0 abarth 2013-03-02 10:15:22 -0800 XSSAuditor has a subtle race condition when used with the threaded HTML parser 845819 1 191110 abarth 2013-03-02 10:16:49 -0800 Created attachment 191110 Patch 845820 2 191110 webkit.review.bot 2013-03-02 10:20:17 -0800 Comment on attachment 191110 Patch Attachment 191110 did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16778921 845822 3 191110 webkit-ews 2013-03-02 10:22:59 -0800 Comment on attachment 191110 Patch Attachment 191110 did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16844192 845824 4 191110 eflews.bot 2013-03-02 10:23:32 -0800 Comment on attachment 191110 Patch Attachment 191110 did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/16864160 845826 5 191110 webkit-ews 2013-03-02 10:24:31 -0800 Comment on attachment 191110 Patch Attachment 191110 did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16864162 845827 6 191110 webkit.review.bot 2013-03-02 10:25:15 -0800 Comment on attachment 191110 Patch Attachment 191110 did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16885074 845829 7 191112 abarth 2013-03-02 10:28:01 -0800 Created attachment 191112 Patch 845850 8 191112 eric 2013-03-02 12:50:38 -0800 Comment on attachment 191112 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. 845871 9 abarth 2013-03-02 14:00:26 -0800 If you bind a const reference to a temporary, then the temporary lives a long as the const ref. 845872 10 abarth 2013-03-02 14:10:53 -0800 http://www.informit.com/guides/content.aspx?g=cplusplus&seqNum=199 845874 11 eric 2013-03-02 14:21:28 -0800 (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!? 845875 12 eric 2013-03-02 14:21:44 -0800 C++ is even crazier than I thought. 845878 13 191112 eric 2013-03-02 15:22:37 -0800 Comment on attachment 191112 Patch We should consider changing the names of thread safe functions like this one. 845879 14 191112 dbates 2013-03-02 15:24:11 -0800 Comment on attachment 191112 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>. 845888 15 191112 webkit.review.bot 2013-03-02 17:47:57 -0800 Comment on attachment 191112 Patch Clearing flags on attachment: 191112 Committed r144549: <http://trac.webkit.org/changeset/144549> 845889 16 webkit.review.bot 2013-03-02 17:48:01 -0800 All reviewed patches have been landed. Closing bug. 845898 17 darin 2013-03-02 18:46:13 -0800 (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. 191110 2013-03-02 10:16:49 -0800 2013-03-02 10:27:58 -0800 Patch bug-111253-20130302101259.patch text/plain 1745 abarth U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ0NTQyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGQzMGIzMTRmZDhjMjI5 MDc3YjQyMzIzYzkzYzVkN2E2NjE1NjBjNC4uYTkxZGZjNjE4N2M5MTVlMzIyMjczOTgyMTAyZGQ4 MjFlZDQ5YWU1YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEzLTAzLTAyICBBZGFt IEJhcnRoICA8YWJhcnRoQHdlYmtpdC5vcmc+CisKKyAgICAgICAgWFNTQXVkaXRvciBoYXMgYSBz dWJ0bGUgcmFjZSBjb25kaXRpb24gd2hlbiB1c2VkIHdpdGggdGhlIHRocmVhZGVkIEhUTUwgcGFy c2VyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTEy NTMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZSB3 ZXJlIHJlZmluZyBhbmQgZGVyZWZpbmcgYSBTdHJpbmdJbXBsIGZvciBhIG1haW4tdGhyZWFkCisg ICAgICAgIEF0b21pY1N0cmluZy4gVXNpbmcgUXVhbGlmaWVkTmFtZXMgb24gdGhlIGJhY2tncm91 bmQgdGhyZWFkIGlzIHZlcnkKKyAgICAgICAgZnJhZ2lsZSBhbmQgd2Ugc2hvdWxkIGZpZ3VyZSBv dXQgYSBtb3JlIHJvYnVzdCBzb2x1dGlvbi4KKworICAgICAgICAqIGh0bWwvcGFyc2VyL1hTU0F1 ZGl0b3IuY3BwOgorICAgICAgICAoV2ViQ29yZTo6ZmluZEF0dHJpYnV0ZVdpdGhOYW1lKToKKwog MjAxMy0wMy0wMiAgUm9iIEJ1aXMgIDxyYnVpc0ByaW0uY29tPgogCiAgICAgICAgIFRleHQgb3Zl cmZsb3cgZWxsaXBzaXMgd3JvbmcgY29sb3Igd2hlbiB1c2luZyB3ZWJraXQtdGV4dC1maWxsLWNv bG9yCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9odG1sL3BhcnNlci9YU1NBdWRpdG9yLmNw cCBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFyc2VyL1hTU0F1ZGl0b3IuY3BwCmluZGV4IDNhYTY1 YTcxMzZjM2YyN2I1YTM2MDgyNmNhMzczNzRiYjdiN2QyNTcuLjU2ZjNmMDg5ZjBjYzc3ZGVmMDVk ODUxOWU1ODA0MjFlNzFiOTYwZGQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFy c2VyL1hTU0F1ZGl0b3IuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFyc2VyL1hTU0F1 ZGl0b3IuY3BwCkBAIC0xMjAsNyArMTIwLDggQEAgc3RhdGljIGJvb2wgaGFzTmFtZShjb25zdCBI VE1MVG9rZW4mIHRva2VuLCBjb25zdCBRdWFsaWZpZWROYW1lJiBuYW1lKQogCiBzdGF0aWMgYm9v bCBmaW5kQXR0cmlidXRlV2l0aE5hbWUoY29uc3QgSFRNTFRva2VuJiB0b2tlbiwgY29uc3QgUXVh bGlmaWVkTmFtZSYgbmFtZSwgc2l6ZV90JiBpbmRleE9mTWF0Y2hpbmdBdHRyaWJ1dGUpCiB7Ci0g ICAgU3RyaW5nIGF0dHJOYW1lID0gbmFtZS5sb2NhbE5hbWUoKS5zdHJpbmcoKTsKKyAgICAvLyBO b3RpY2UgdGhhdCB3ZSdyZSBjYXJlZnVsIG5vdCB0byByZWYgdGhlIFN0cmluZ0ltcGwgaGVyZSBi ZWNhdXNlIHdlIG1pZ2h0IGJlIG9uIGEgYmFja2dyb3VuZCB0aHJlYWQuCisgICAgY29uc3QgU3Ry aW5nJiBhdHRyTmFtZSA9IG5hbWUubG9jYWxOYW1lKCkuc3RyaW5nKCk7CiAKICAgICBpZiAobmFt ZS5uYW1lc3BhY2VVUkkoKSA9PSBYTGlua05hbWVzOjp4bGlua05hbWVzcGFjZVVSSSkKICAgICAg ICAgYXR0ck5hbWUgPSAieGxpbms6IiArIGF0dHJOYW1lOwo= 191112 2013-03-02 10:28:01 -0800 2013-03-02 17:47:57 -0800 Patch bug-111253-20130302102411.patch text/plain 1982 abarth U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ0NTQyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGQzMGIzMTRmZDhjMjI5 MDc3YjQyMzIzYzkzYzVkN2E2NjE1NjBjNC4uYTkxZGZjNjE4N2M5MTVlMzIyMjczOTgyMTAyZGQ4 MjFlZDQ5YWU1YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEzLTAzLTAyICBBZGFt IEJhcnRoICA8YWJhcnRoQHdlYmtpdC5vcmc+CisKKyAgICAgICAgWFNTQXVkaXRvciBoYXMgYSBz dWJ0bGUgcmFjZSBjb25kaXRpb24gd2hlbiB1c2VkIHdpdGggdGhlIHRocmVhZGVkIEhUTUwgcGFy c2VyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTEy NTMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZSB3 ZXJlIHJlZmluZyBhbmQgZGVyZWZpbmcgYSBTdHJpbmdJbXBsIGZvciBhIG1haW4tdGhyZWFkCisg ICAgICAgIEF0b21pY1N0cmluZy4gVXNpbmcgUXVhbGlmaWVkTmFtZXMgb24gdGhlIGJhY2tncm91 bmQgdGhyZWFkIGlzIHZlcnkKKyAgICAgICAgZnJhZ2lsZSBhbmQgd2Ugc2hvdWxkIGZpZ3VyZSBv dXQgYSBtb3JlIHJvYnVzdCBzb2x1dGlvbi4KKworICAgICAgICAqIGh0bWwvcGFyc2VyL1hTU0F1 ZGl0b3IuY3BwOgorICAgICAgICAoV2ViQ29yZTo6ZmluZEF0dHJpYnV0ZVdpdGhOYW1lKToKKwog MjAxMy0wMy0wMiAgUm9iIEJ1aXMgIDxyYnVpc0ByaW0uY29tPgogCiAgICAgICAgIFRleHQgb3Zl cmZsb3cgZWxsaXBzaXMgd3JvbmcgY29sb3Igd2hlbiB1c2luZyB3ZWJraXQtdGV4dC1maWxsLWNv bG9yCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9odG1sL3BhcnNlci9YU1NBdWRpdG9yLmNw cCBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFyc2VyL1hTU0F1ZGl0b3IuY3BwCmluZGV4IDNhYTY1 YTcxMzZjM2YyN2I1YTM2MDgyNmNhMzczNzRiYjdiN2QyNTcuLjBhZDI5MjBmMDdmNGUwMjY4M2U2 OGJjMjk2NjliMzU5NWRlYzA0OTIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFy c2VyL1hTU0F1ZGl0b3IuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFyc2VyL1hTU0F1 ZGl0b3IuY3BwCkBAIC0xMjAsMTAgKzEyMCw4IEBAIHN0YXRpYyBib29sIGhhc05hbWUoY29uc3Qg SFRNTFRva2VuJiB0b2tlbiwgY29uc3QgUXVhbGlmaWVkTmFtZSYgbmFtZSkKIAogc3RhdGljIGJv b2wgZmluZEF0dHJpYnV0ZVdpdGhOYW1lKGNvbnN0IEhUTUxUb2tlbiYgdG9rZW4sIGNvbnN0IFF1 YWxpZmllZE5hbWUmIG5hbWUsIHNpemVfdCYgaW5kZXhPZk1hdGNoaW5nQXR0cmlidXRlKQogewot ICAgIFN0cmluZyBhdHRyTmFtZSA9IG5hbWUubG9jYWxOYW1lKCkuc3RyaW5nKCk7Ci0KLSAgICBp ZiAobmFtZS5uYW1lc3BhY2VVUkkoKSA9PSBYTGlua05hbWVzOjp4bGlua05hbWVzcGFjZVVSSSkK LSAgICAgICAgYXR0ck5hbWUgPSAieGxpbms6IiArIGF0dHJOYW1lOworICAgIC8vIE5vdGljZSB0 aGF0IHdlJ3JlIGNhcmVmdWwgbm90IHRvIHJlZiB0aGUgU3RyaW5nSW1wbCBoZXJlIGJlY2F1c2Ug d2UgbWlnaHQgYmUgb24gYSBiYWNrZ3JvdW5kIHRocmVhZC4KKyAgICBjb25zdCBTdHJpbmcmIGF0 dHJOYW1lID0gbmFtZS5uYW1lc3BhY2VVUkkoKSA9PSBYTGlua05hbWVzOjp4bGlua05hbWVzcGFj ZVVSSSA/ICJ4bGluazoiICsgbmFtZS5sb2NhbE5hbWUoKS5zdHJpbmcoKSA6IG5hbWUubG9jYWxO YW1lKCkuc3RyaW5nKCk7CiAKICAgICBmb3IgKHNpemVfdCBpID0gMDsgaSA8IHRva2VuLmF0dHJp YnV0ZXMoKS5zaXplKCk7ICsraSkgewogICAgICAgICBpZiAoZXF1YWxJZ25vcmluZ051bGxpdHko dG9rZW4uYXR0cmlidXRlcygpLmF0KGkpLm5hbWUsIGF0dHJOYW1lKSkgewo=