Attachment 372639 Details for Bug 199077 – Patch

[patch] Patch

bug-199077-20190621123657.patch (text/plain), 3.88 KB, created by Yusuke Suzuki on 2019-06-21 12:36:57 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-06-21 12:36:57 PDT

Size: 3.88 KB

patch

obsolete

>Subversion Revision: 246687
>diff --git a/Source/bmalloc/ChangeLog b/Source/bmalloc/ChangeLog
>index ec4db9070e69305067db9d9e52548ab59a43d074..ee236d441a25f07d178a54d11c2e51e6aab99a69 100644
>--- a/Source/bmalloc/ChangeLog
>+++ b/Source/bmalloc/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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
>diff --git a/Source/bmalloc/bmalloc/IsoTLS.cpp b/Source/bmalloc/bmalloc/IsoTLS.cpp
>index c203213b855bdcdc9ed0796fd204ab5d948d8006..b5d322194e3a051eac09a41029ca241f5d47890d 100644
>--- a/Source/bmalloc/bmalloc/IsoTLS.cpp
>+++ b/Source/bmalloc/bmalloc/IsoTLS.cpp
>@@ -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;
> }

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 199077: 372622 | 372623 | 372639