REGRESSION(r168256): JSString can get 8-bit flag wrong when re-using …

…AtomicStrings. <https://webkit.org/b/133574> <rdar://problem/18051847> Source/JavaScriptCore: The optimization that resolves JSRopeStrings into an existing AtomicString (to save time and memory by avoiding StringImpl allocation) had a bug that it wasn't copying the 8-bit flag from the AtomicString. This could lead to a situation where a 16-bit StringImpl containing only 8-bit characters is sitting in the AtomicString table, is found by the rope resolution optimization, and gives you a rope that thinks it's all 8-bit, but has a fiber with 16-bit characters. Resolving that rope will then yield incorrect results. This was all caught by an assertion, but very hard to reproduce. Test: js/dopey-rope-with-16-bit-propertyname.html Reviewed by Darin Adler. * runtime/JSString.cpp: (JSC::JSRopeString::resolveRopeToAtomicString): (JSC::JSRopeString::resolveRopeToExistingAtomicString): * runtime/JSString.h: (JSC::JSString::setIs8Bit): (JSC::JSString::toExistingAtomicString): LayoutTests: Add a tests that creates a 16-bit AtomicString with only 8-bit characters, then tiers up into baseline JIT and uses that string as part of a rope-within-a-rope and serializes that rope to get an incorrect concatenation. Reviewed by Darin Adler. * js/dopey-rope-with-16-bit-propertyname-expected.txt: Added. * js/dopey-rope-with-16-bit-propertyname.html: Added. Canonical link: https://commits.webkit.org/153901@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@172727 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Aug 18, 2014 · 1ea2edc68e68799d002066b0b77d9ff4ec5ea595 · 1ea2edc
1 parent e7c15f5
commit 1ea2edc68e68799d002066b0b77d9ff4ec5ea595
Showing 6 changed files with 100 additions and 2 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,18 @@
+2014-08-18  Andreas Kling  <akling@apple.com>
+
+        REGRESSION(r168256): JSString can get 8-bit flag wrong when re-using AtomicStrings.
+        <https://webkit.org/b/133574>
+        <rdar://problem/18051847>
+
+        Add a tests that creates a 16-bit AtomicString with only 8-bit characters,
+        then tiers up into baseline JIT and uses that string as part of a rope-within-a-rope
+        and serializes that rope to get an incorrect concatenation.
+
+        Reviewed by Darin Adler.
+
+        * js/dopey-rope-with-16-bit-propertyname-expected.txt: Added.
+        * js/dopey-rope-with-16-bit-propertyname.html: Added.
+
 2014-08-18  Vivek Galatage  <vivek.vg@samsung.com>
 
         Implement CanvasRenderingContext2D direction attribute

diff --git a/LayoutTests/js/dopey-rope-with-16-bit-propertyname-expected.txt b/LayoutTests/js/dopey-rope-with-16-bit-propertyname-expected.txt
@@ -0,0 +1,10 @@
+Test that a 16-bit AtomicString containing only 8-bit characters doesn't confuse the JIT into thinking it's an 8-bit AtomicString.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS globalRope is 'foo.zest'
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/dopey-rope-with-16-bit-propertyname.html b/LayoutTests/js/dopey-rope-with-16-bit-propertyname.html
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf-8">
+<script src="../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+
+description("Test that a 16-bit AtomicString containing only 8-bit characters doesn't confuse the JIT into thinking it's an 8-bit AtomicString.");
+
+o = {};
+
+stringWithEmoji = "zest😐";
+var test16bit = stringWithEmoji.substring(0, 4);
+
+o[test16bit] = "this makes it an AtomicString";
+
+globalRope = "";
+
+function jittable(a, b) {
+    for (var i = 0; i < 5000; ++i) {
+        poisonedRope = a + b;
+        o[poisonedRope] = 1;
+        globalRope = "foo." + poisonedRope;
+    }
+}
+
+jittable("ze", "st");
+
+shouldBe("globalRope", "'foo.zest'");
+
+</script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,33 @@
+2014-08-18  Andreas Kling  <akling@apple.com>
+
+        REGRESSION(r168256): JSString can get 8-bit flag wrong when re-using AtomicStrings.
+        <https://webkit.org/b/133574>
+        <rdar://problem/18051847>
+
+        The optimization that resolves JSRopeStrings into an existing
+        AtomicString (to save time and memory by avoiding StringImpl allocation)
+        had a bug that it wasn't copying the 8-bit flag from the AtomicString.
+
+        This could lead to a situation where a 16-bit StringImpl containing
+        only 8-bit characters is sitting in the AtomicString table, is found
+        by the rope resolution optimization, and gives you a rope that thinks
+        it's all 8-bit, but has a fiber with 16-bit characters.
+
+        Resolving that rope will then yield incorrect results.
+
+        This was all caught by an assertion, but very hard to reproduce.
+
+        Test: js/dopey-rope-with-16-bit-propertyname.html
+
+        Reviewed by Darin Adler.
+
+        * runtime/JSString.cpp:
+        (JSC::JSRopeString::resolveRopeToAtomicString):
+        (JSC::JSRopeString::resolveRopeToExistingAtomicString):
+        * runtime/JSString.h:
+        (JSC::JSString::setIs8Bit):
+        (JSC::JSString::toExistingAtomicString):
+
 2014-08-18  Saam Barati  <sbarati@apple.com>
 
         The parser should generate AST nodes the var declarations with no initializers

diff --git a/Source/JavaScriptCore/runtime/JSString.cpp b/Source/JavaScriptCore/runtime/JSString.cpp
@@ -161,17 +161,20 @@ void JSRopeString::resolveRopeToAtomicString(ExecState* exec) const
     if (m_length > maxLengthForOnStackResolve) {
         resolveRope(exec);
         m_value = AtomicString(m_value);
+        setIs8Bit(m_value.impl()->is8Bit());
         return;
     }
 
     if (is8Bit()) {
         LChar buffer[maxLengthForOnStackResolve];
         resolveRopeInternal8(buffer);
         m_value = AtomicString(buffer, m_length);
+        setIs8Bit(m_value.impl()->is8Bit());
     } else {
         UChar buffer[maxLengthForOnStackResolve];
         resolveRopeInternal16(buffer);
         m_value = AtomicString(buffer, m_length);
+        setIs8Bit(m_value.impl()->is8Bit());
     }
 
     clearFibers();
@@ -193,6 +196,7 @@ AtomicStringImpl* JSRopeString::resolveRopeToExistingAtomicString(ExecState* exe
         resolveRope(exec);
         if (AtomicStringImpl* existingAtomicString = AtomicString::find(m_value.impl())) {
             m_value = *existingAtomicString;
+            setIs8Bit(m_value.impl()->is8Bit());
             clearFibers();
             return existingAtomicString;
         }
@@ -204,6 +208,7 @@ AtomicStringImpl* JSRopeString::resolveRopeToExistingAtomicString(ExecState* exe
         resolveRopeInternal8(buffer);
         if (AtomicStringImpl* existingAtomicString = AtomicString::find(buffer, m_length)) {
             m_value = *existingAtomicString;
+            setIs8Bit(m_value.impl()->is8Bit());
             clearFibers();
             return existingAtomicString;
         }
@@ -212,6 +217,7 @@ AtomicStringImpl* JSRopeString::resolveRopeToExistingAtomicString(ExecState* exe
         resolveRopeInternal16(buffer);
         if (AtomicStringImpl* existingAtomicString = AtomicString::find(buffer, m_length)) {
             m_value = *existingAtomicString;
+            setIs8Bit(m_value.impl()->is8Bit());
             clearFibers();
             return existingAtomicString;
         }

diff --git a/Source/JavaScriptCore/runtime/JSString.h b/Source/JavaScriptCore/runtime/JSString.h
@@ -187,7 +187,7 @@ namespace JSC {
 
         bool isRope() const { return m_value.isNull(); }
         bool is8Bit() const { return m_flags & Is8Bit; }
-        void setIs8Bit(bool flag)
+        void setIs8Bit(bool flag) const
         {
             if (flag)
                 m_flags |= Is8Bit;
@@ -201,7 +201,7 @@ namespace JSC {
         bool tryHashConsLock();
         void releaseHashConsLock();
 
-        unsigned m_flags;
+        mutable unsigned m_flags;
 
         // A string is represented either by a String or a rope of fibers.
         unsigned m_length;
@@ -477,6 +477,7 @@ namespace JSC {
             return static_cast<AtomicStringImpl*>(m_value.impl());
         if (AtomicStringImpl* existingAtomicString = AtomicString::find(m_value.impl())) {
             m_value = *existingAtomicString;
+            setIs8Bit(m_value.impl()->is8Bit());
             return existingAtomicString;
         }
         return nullptr;