[bmalloc] IsoTLS Layout extension initializes one IsoTLSEntry twice
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Jun 2019 21:31:17 +0000 (21:31 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Jun 2019 21:31:17 +0000 (21:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199077

Reviewed by Saam Barati.

Found that IsoTLS::ensureEntries can construct the same IsoTLSEntry many times, it can leak memory because the construction clears previous fields including freelist.

1. We have oldLastEntry.
2. In that case, startEntry is oldLastEntry.
3. We find some targetEntry.
4. Finally, if startEntry exists, we newly construct [startEntry, targetEntry]
5. In the above sequence, oldLastEntry (== startEntry) is constructed again, while oldLastEntry is already constructed previously.

We fix this issue by changing the startEntry. We already have `RELEASE_BASSERT(!oldLastEntry || oldLastEntry->offset() < offset);`
assertion. This means that `oldLastEntry->m_next` must exist, otherwise the following loop would not find a `targetEntry`. And `layout.head()`
must return non nullptr at `IsoTLS::ensureEntries` because `IsoTLS::ensureEntries` requires that `IsoHeap<>` is initialized, and `IsoHeap<>`
must add at least one TLS entry to the IsoTLSLayout.

* bmalloc/IsoTLS.cpp:
(bmalloc::IsoTLS::ensureEntries):

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

Source/bmalloc/ChangeLog
Source/bmalloc/bmalloc/IsoTLS.cpp

index ec4db90..9b203e9 100644 (file)
@@ -1,3 +1,26 @@
+2019-06-21  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [bmalloc] IsoTLS Layout extension initializes one IsoTLSEntry twice
+        https://bugs.webkit.org/show_bug.cgi?id=199077
+
+        Reviewed by Saam Barati.
+
+        Found that IsoTLS::ensureEntries can construct the same IsoTLSEntry many times, it can leak memory because the construction clears previous fields including freelist.
+
+        1. We have oldLastEntry.
+        2. In that case, startEntry is oldLastEntry.
+        3. We find some targetEntry.
+        4. Finally, if startEntry exists, we newly construct [startEntry, targetEntry]
+        5. In the above sequence, oldLastEntry (== startEntry) is constructed again, while oldLastEntry is already constructed previously.
+
+        We fix this issue by changing the startEntry. We already have `RELEASE_BASSERT(!oldLastEntry || oldLastEntry->offset() < offset);`
+        assertion. This means that `oldLastEntry->m_next` must exist, otherwise the following loop would not find a `targetEntry`. And `layout.head()`
+        must return non nullptr at `IsoTLS::ensureEntries` because `IsoTLS::ensureEntries` requires that `IsoHeap<>` is initialized, and `IsoHeap<>`
+        must add at least one TLS entry to the IsoTLSLayout.
+
+        * bmalloc/IsoTLS.cpp:
+        (bmalloc::IsoTLS::ensureEntries):
+
 2019-06-19  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [bmalloc] IsoHeap's initialization is racy with IsoHeap::isInitialized
index c203213..b5d3221 100644 (file)
@@ -82,21 +82,19 @@ IsoTLS* IsoTLS::ensureEntries(unsigned offset)
     IsoTLSEntry* oldLastEntry = tls ? tls->m_lastEntry : nullptr;
     RELEASE_BASSERT(!oldLastEntry || oldLastEntry->offset() < offset);
     
-    IsoTLSEntry* startEntry = oldLastEntry ? oldLastEntry : layout.head();
+    IsoTLSEntry* startEntry = oldLastEntry ? oldLastEntry->m_next : layout.head();
+    RELEASE_BASSERT(startEntry);
     
     IsoTLSEntry* targetEntry = startEntry;
-    size_t requiredCapacity = 0;
-    if (startEntry) {
-        for (;;) {
-            RELEASE_BASSERT(targetEntry);
-            RELEASE_BASSERT(targetEntry->offset() <= offset);
-            if (targetEntry->offset() == offset)
-                break;
-            targetEntry = targetEntry->m_next;
-        }
+    for (;;) {
         RELEASE_BASSERT(targetEntry);
-        requiredCapacity = targetEntry->extent();
+        RELEASE_BASSERT(targetEntry->offset() <= offset);
+        if (targetEntry->offset() == offset)
+            break;
+        targetEntry = targetEntry->m_next;
     }
+    RELEASE_BASSERT(targetEntry);
+    size_t requiredCapacity = targetEntry->extent();
     
     if (!tls || requiredCapacity > tls->m_capacity) {
         size_t requiredSize = sizeForCapacity(requiredCapacity);
@@ -122,16 +120,14 @@ IsoTLS* IsoTLS::ensureEntries(unsigned offset)
         set(tls);
     }
     
-    if (startEntry) {
-        startEntry->walkUpToInclusive(
-            targetEntry,
-            [&] (IsoTLSEntry* entry) {
-                entry->construct(tls->m_data + entry->offset());
-            });
-        
-        tls->m_lastEntry = targetEntry;
-        tls->m_extent = targetEntry->extent();
-    }
+    startEntry->walkUpToInclusive(
+        targetEntry,
+        [&] (IsoTLSEntry* entry) {
+            entry->construct(tls->m_data + entry->offset());
+        });
+    
+    tls->m_lastEntry = targetEntry;
+    tls->m_extent = targetEntry->extent();
     
     return tls;
 }