Changeset 260383 in webkit

Timestamp:

Apr 20, 2020 12:10:00 PM (4 years ago)

Author:

Darin Adler

Message:

REGRESSION (r253987): StringImpl::adopt(Vector&&) copies only half of the characters in the Vector when copying across malloc zones
​https://bugs.webkit.org/show_bug.cgi?id=210736

Reviewed by Yusuke Suzuki.

• wtf/text/StringImpl.cpp:

(WTF::StringImpl::replace): Use copyCharacters instead of memcpy.

• wtf/text/StringImpl.h: Use std::is_same_v instead of std::is_same.

(WTF::StringImpl::StringImpl): Use copyCharacters instead of memcpy.
Also drop C-style cast to cast away const.
(WTF::StringImpl::adopt): Don't even try to adopt across malloc zones.

Location:

trunk/Source/WTF

Files:

• ChangeLog (modified) (1 diff)
• wtf/text/StringImpl.cpp (modified) (1 diff)
• wtf/text/StringImpl.h (modified) (4 diffs)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2020-04-20  Darin Adler  <darin@apple.com>
+
+        REGRESSION (r253987): StringImpl::adopt(Vector&&) copies only half of the characters in the Vector when copying across malloc zones
+        https://bugs.webkit.org/show_bug.cgi?id=210736
+
+        Reviewed by Yusuke Suzuki.
+
+        * wtf/text/StringImpl.cpp:
+        (WTF::StringImpl::replace): Use copyCharacters instead of memcpy.
+
+        * wtf/text/StringImpl.h: Use std::is_same_v instead of std::is_same.
+        (WTF::StringImpl::StringImpl): Use copyCharacters instead of memcpy.
+        Also drop C-style cast to cast away const.
+        (WTF::StringImpl::adopt): Don't even try to adopt across malloc zones.
+
 2020-04-20  Darin Adler  <darin@apple.com>
 

trunk/Source/WTF/wtf/text/StringImpl.cpp

@@ -1303 +1303 @@
     auto newImpl = createUninitializedInternalNonEmpty(m_length, data);
 
-    memcpy(data, m_data16, i * sizeof(UChar));
+    copyCharacters(data, m_data16, i);
     for (unsigned j = i; j != m_length; ++j) {
         UChar character = m_data16[j];

trunk/Source/WTF/wtf/text/StringImpl.h

@@ -893 +893 @@
     : StringImplShape(s_refCountIncrement, length, static_cast<const LChar*>(nullptr), s_hashFlag8BitBuffer | StringNormal | BufferOwned)
 {
-    if constexpr (std::is_same<Malloc, StringImplMalloc>::value)
+    if constexpr (std::is_same_v<Malloc, StringImplMalloc>)
         m_data8 = characters.leakPtr();
     else {
-        m_data8 = static_cast<const LChar*>(StringImplMalloc::malloc(length));
-        memcpy((void*)m_data8, characters.get(), length);
+        auto data8 = static_cast<LChar*>(StringImplMalloc::malloc(length * sizeof(LChar)));
+        copyCharacters(data8, characters.get(), length);
+        m_data8 = data8;
     }
 
@@ -928 +929 @@
     : StringImplShape(s_refCountIncrement, length, static_cast<const UChar*>(nullptr), StringNormal | BufferOwned)
 {
-    if constexpr (std::is_same<Malloc, StringImplMalloc>::value)
+    if constexpr (std::is_same_v<Malloc, StringImplMalloc>)
         m_data16 = characters.leakPtr();
     else {
-        m_data16 = static_cast<const UChar*>(StringImplMalloc::malloc(length * sizeof(UChar)));
-        memcpy((void*)m_data16, characters.get(), length * sizeof(UChar));
+        auto data16 = static_cast<UChar*>(StringImplMalloc::malloc(length * sizeof(UChar)));
+        copyCharacters(data16, characters.get(), length);
+        m_data16 = data16;
     }
 
@@ -1032 +1034 @@
 inline Ref<StringImpl> StringImpl::adopt(Vector<CharacterType, inlineCapacity, OverflowHandler, minCapacity, Malloc>&& vector)
 {
-    if (size_t size = vector.size()) {
-        ASSERT(vector.data());
-        if (size > MaxLength)
+    if constexpr (std::is_same_v<Malloc, StringImplMalloc>) {
+        auto length = vector.size();
+        if (!length)
+            return *empty();
+        if (length > MaxLength)
             CRASH();
-
-        if constexpr (std::is_same<Malloc, StringImplMalloc>::value)
-            return adoptRef(*new StringImpl(vector.releaseBuffer(), size));
-        else {
-            // We have to copy between malloc zones.
-            auto vectorBuffer = vector.releaseBuffer();
-            auto stringImplBuffer = MallocPtr<CharacterType, StringImplMalloc>::malloc(size);
-            memcpy(stringImplBuffer.get(), vectorBuffer.get(), size);
-            return adoptRef(*new StringImpl(WTFMove(stringImplBuffer), size));
-        }
-    }
-    return *empty();
+        return adoptRef(*new StringImpl(vector.releaseBuffer(), length));
+    } else
+        return create(vector.data(), vector.size());
 }
 
@@ -1135 +1130 @@
 inline void StringImpl::copyCharacters(DestinationCharacterType* destination, const SourceCharacterType* source, unsigned numCharacters)
 {
-    static_assert(std::is_same<SourceCharacterType, LChar>::value || std::is_same<SourceCharacterType, UChar>::value);
-    static_assert(std::is_same<DestinationCharacterType, LChar>::value || std::is_same<DestinationCharacterType, UChar>::value);
-    if constexpr (std::is_same<SourceCharacterType, DestinationCharacterType>::value) {
+    static_assert(std::is_same_v<SourceCharacterType, LChar> || std::is_same_v<SourceCharacterType, UChar>);
+    static_assert(std::is_same_v<DestinationCharacterType, LChar> || std::is_same_v<DestinationCharacterType, UChar>);
+    if constexpr (std::is_same_v<SourceCharacterType, DestinationCharacterType>) {
         if (numCharacters == 1) {
             *destination = *source;