Changeset 252674 in webkit

Timestamp:

Nov 19, 2019 6:28:52 PM (4 years ago)

Author:

ysuzuki@apple.com

Message:

[JSC] MetadataTable::sizeInBytes should not touch m_rawBuffer in UnlinkedMetadataTable unless MetadataTable is linked to that UnlinkedMetadataTable
​https://bugs.webkit.org/show_bug.cgi?id=204390

Reviewed by Mark Lam.

We have a race issue here. When calling MetadataTable::sizeInBytes, we call UnlinkedMetadataTable::sizeInBytes since we change the result based on
whether this MetadataTable is linked to this UnlinkedMetadataTable or not. The problem is that we are calling UnlinkedMetadataTable::totalSize
unconditionally in UnlinkedMetadataTable::sizeInBytes, and this is touching m_rawBuffer unconditionally. This is not correct since it is possible
that this m_rawBuffer is realloced while we are calling MetadataTable::sizeInBytes in GC thread.

1. The GC thread is calling MetadataTable::sizeInBytes for MetadataTable "A".
2. The main thread is destroying MetadataTable "B".
3. MetadataTable "B" is linked to UnlinkedMetadataTable "C".
4. MetadataTable "A" is pointing to UnlinkedMetadataTable "C".
5. "A" is touching UnlinkedMetadataTable::m_rawBuffer in "C", called from MetadataTable::sizeInBytes.
6. (2) destroys MetadataTable "B", and realloc UnlinkedMetadataTable::m_rawBuffer in "C".
7. (5) can touch already freed buffer.

This patch fixes UnlinkedMetadataTable::sizeInBytes: not touching m_rawBuffer unless it is owned by the caller's MetadataTable. We need to call
UnlinkedMetadataTable::sizeInBytes anyway since we need to adjust the result based on whether the caller MetadataTable is linked to this UnlinkedMetadataTable.

• bytecode/UnlinkedMetadataTableInlines.h:

(JSC::UnlinkedMetadataTable::sizeInBytes):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecode/UnlinkedMetadataTableInlines.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-11-19  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] MetadataTable::sizeInBytes should not touch m_rawBuffer in UnlinkedMetadataTable unless MetadataTable is linked to that UnlinkedMetadataTable
+        https://bugs.webkit.org/show_bug.cgi?id=204390
+
+        Reviewed by Mark Lam.
+
+        We have a race issue here. When calling MetadataTable::sizeInBytes, we call UnlinkedMetadataTable::sizeInBytes since we change the result based on
+        whether this MetadataTable is linked to this UnlinkedMetadataTable or not. The problem is that we are calling `UnlinkedMetadataTable::totalSize`
+        unconditionally in UnlinkedMetadataTable::sizeInBytes, and this is touching m_rawBuffer unconditionally. This is not correct since it is possible
+        that this m_rawBuffer is realloced while we are calling MetadataTable::sizeInBytes in GC thread.
+
+            1. The GC thread is calling MetadataTable::sizeInBytes for MetadataTable "A".
+            2. The main thread is destroying MetadataTable "B".
+            3. MetadataTable "B" is linked to UnlinkedMetadataTable "C".
+            4. MetadataTable "A" is pointing to UnlinkedMetadataTable "C".
+            5. "A" is touching UnlinkedMetadataTable::m_rawBuffer in "C", called from MetadataTable::sizeInBytes.
+            6. (2) destroys MetadataTable "B", and realloc UnlinkedMetadataTable::m_rawBuffer in "C".
+            7. (5) can touch already freed buffer.
+
+        This patch fixes UnlinkedMetadataTable::sizeInBytes: not touching m_rawBuffer unless it is owned by the caller's MetadataTable. We need to call
+        UnlinkedMetadataTable::sizeInBytes anyway since we need to adjust the result based on whether the caller MetadataTable is linked to this UnlinkedMetadataTable.
+
+        * bytecode/UnlinkedMetadataTableInlines.h:
+        (JSC::UnlinkedMetadataTable::sizeInBytes):
+
 2019-11-19  Fujii Hironori  <Hironori.Fujii@sony.com>
 

trunk/Source/JavaScriptCore/bytecode/UnlinkedMetadataTableInlines.h

@@ -89 +89 @@
     // In this case, we return the size of the table minus the offset table,
     // which was already accounted for in the UnlinkedCodeBlock.
-    size_t result = totalSize();
+
+    // Be careful not to touch m_rawBuffer if this metadataTable is not owning it.
+    // It is possible that, m_rawBuffer is realloced in the other thread while we are accessing here.
+    size_t result = metadataTable.totalSize();
     if (metadataTable.buffer() == buffer()) {
         ASSERT(m_isLinked);