Continue making XSSAuditor thread safe: Remove unsafe AtomicString compares
authortonyg@chromium.org <tonyg@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 2 Feb 2013 08:25:21 +0000 (08:25 +0000)
committertonyg@chromium.org <tonyg@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 2 Feb 2013 08:25:21 +0000 (08:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=108557

Reviewed by Adam Barth.

Unfortunately HTMLNames comparisons will always be false on a non-main thread
with our current design, so we have to use some "threadSafeMatch" helpers written
for the HTMLBackgroundParser.

Also factor out threadSafeMatch() methods to HTMLParserIdioms.

No new tests because no new functionality.

* html/parser/BackgroundHTMLParser.cpp:
(WebCore):
* html/parser/HTMLParserIdioms.cpp:
(WebCore::threadSafeEqual):
(WebCore):
(WebCore::threadSafeMatch):
* html/parser/HTMLParserIdioms.h:
(WebCore):
* html/parser/XSSAuditor.cpp:
(WebCore::XSSAuditor::eraseAttributeIfInjected):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@141686 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/html/parser/BackgroundHTMLParser.cpp
Source/WebCore/html/parser/HTMLParserIdioms.cpp
Source/WebCore/html/parser/HTMLParserIdioms.h
Source/WebCore/html/parser/XSSAuditor.cpp

index 88ff1f6..5e084a5 100644 (file)
@@ -1,3 +1,29 @@
+2013-02-02  Tony Gentilcore  <tonyg@chromium.org>
+
+        Continue making XSSAuditor thread safe: Remove unsafe AtomicString compares
+        https://bugs.webkit.org/show_bug.cgi?id=108557
+
+        Reviewed by Adam Barth.
+
+        Unfortunately HTMLNames comparisons will always be false on a non-main thread
+        with our current design, so we have to use some "threadSafeMatch" helpers written
+        for the HTMLBackgroundParser.
+
+        Also factor out threadSafeMatch() methods to HTMLParserIdioms.
+
+        No new tests because no new functionality.
+
+        * html/parser/BackgroundHTMLParser.cpp:
+        (WebCore):
+        * html/parser/HTMLParserIdioms.cpp:
+        (WebCore::threadSafeEqual):
+        (WebCore):
+        (WebCore::threadSafeMatch):
+        * html/parser/HTMLParserIdioms.h:
+        (WebCore):
+        * html/parser/XSSAuditor.cpp:
+        (WebCore::XSSAuditor::eraseAttributeIfInjected):
+
 2013-02-01  James Simonsen  <simonjam@chromium.org>
 
         Add didChangePriority() to ResourceHandle
index ec46152..b9c2649 100644 (file)
@@ -31,6 +31,7 @@
 
 #include "HTMLDocumentParser.h"
 #include "HTMLNames.h"
+#include "HTMLParserIdioms.h"
 #include "HTMLParserThread.h"
 #include "HTMLTokenizer.h"
 #include "MathMLNames.h"
@@ -56,18 +57,6 @@ static void checkThatTokensAreSafeToSendToAnotherThread(const CompactHTMLTokenSt
 // FIXME: Tune this constant based on a benchmark. The current value was choosen arbitrarily.
 static const size_t pendingTokenLimit = 4000;
 
-static bool threadSafeEqual(StringImpl* a, StringImpl* b)
-{
-    if (a->hash() != b->hash())
-        return false;
-    return StringHash::equal(a, b);
-}
-
-static bool threadSafeMatch(const String& localName, const QualifiedName& qName)
-{
-    return threadSafeEqual(localName.impl(), qName.localName().impl());
-}
-
 ParserMap& parserMap()
 {
     // This initialization assumes that this will be initialize on the main thread before
index e5201ce..2ca943e 100644 (file)
 #include "HTMLParserIdioms.h"
 
 #include "Decimal.h"
+#include "QualifiedName.h"
 #include <limits>
 #include <wtf/MathExtras.h>
 #include <wtf/text/AtomicString.h>
 #include <wtf/text/StringBuilder.h>
+#include <wtf/text/StringHash.h>
 
 namespace WebCore {
 
@@ -275,4 +277,21 @@ bool parseHTMLNonNegativeInteger(const String& input, unsigned& value)
     return parseHTMLNonNegativeIntegerInternal(start, start + length, value);
 }
 
+static bool threadSafeEqual(StringImpl* a, StringImpl* b)
+{
+    if (a->hash() != b->hash())
+        return false;
+    return StringHash::equal(a, b);
+}
+
+bool threadSafeMatch(const QualifiedName& a, const QualifiedName& b)
+{
+    return threadSafeEqual(a.localName().impl(), b.localName().impl());
+}
+
+bool threadSafeMatch(const String& localName, const QualifiedName& qName)
+{
+    return threadSafeEqual(localName.impl(), qName.localName().impl());
+}
+
 }
index 577e02e..53d0ff7 100644 (file)
@@ -31,6 +31,7 @@
 namespace WebCore {
 
 class Decimal;
+class QualifiedName;
 
 // Space characters as defined by the HTML specification.
 bool isHTMLSpace(UChar);
@@ -85,6 +86,9 @@ inline bool isNotHTMLSpace(UChar character)
     return !isHTMLSpace(character);
 }
 
+bool threadSafeMatch(const QualifiedName&, const QualifiedName&);
+bool threadSafeMatch(const String&, const QualifiedName&);
+
 }
 
 #endif
index 4b2d5e8..9efa282 100644 (file)
@@ -486,9 +486,9 @@ bool XSSAuditor::eraseAttributeIfInjected(HTMLToken& token, const QualifiedName&
     if (findAttributeWithName(token, attributeName, indexOfAttribute)) {
         const HTMLToken::Attribute& attribute = token.attributes().at(indexOfAttribute);
         if (isContainedInRequest(decodedSnippetForAttribute(token, attribute, treatment))) {
-            if (attributeName == srcAttr && isLikelySafeResource(String(attribute.m_value.data(), attribute.m_value.size())))
+            if (threadSafeMatch(attributeName, srcAttr) && isLikelySafeResource(String(attribute.m_value.data(), attribute.m_value.size())))
                 return false;
-            if (attributeName == http_equivAttr && !isDangerousHTTPEquiv(String(attribute.m_value.data(), attribute.m_value.size())))
+            if (threadSafeMatch(attributeName, http_equivAttr) && !isDangerousHTTPEquiv(String(attribute.m_value.data(), attribute.m_value.size())))
                 return false;
             token.eraseValueOfAttribute(indexOfAttribute);
             if (!replacementValue.isEmpty())