Attachment 372623 Details for Bug 199077 – Patch

[patch] Patch

bug-199077-20190621021239.patch (text/plain), 3.06 KB, created by Yusuke Suzuki on 2019-06-21 02:12:40 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-06-21 02:12:40 PDT

Size: 3.06 KB

patch

obsolete

>Subversion Revision: 246673
>diff --git a/Source/bmalloc/ChangeLog b/Source/bmalloc/ChangeLog
>index ec4db9070e69305067db9d9e52548ab59a43d074..6acd3169ea84ce5751f2617292421ecb60a1dc6b 100644
>--- a/Source/bmalloc/ChangeLog
>+++ b/Source/bmalloc/ChangeLog
>@@ -1,3 +1,27 @@
>+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 can clean up IsoTLS::ensureEntries code, but it is a bit dangerous at this time[1].
>+        So instead, we carefully add defensive code to handle the above case correctly. Later, we will
>+        attempt to clean up IsoTLS::ensureEntries code.
>+
>+        [1]: https://bugs.webkit.org/show_bug.cgi?id=199103
>+
>+        * 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..f277fda2b6f50cc84eae01d6279143eec9ee5233 100644
>--- a/Source/bmalloc/bmalloc/IsoTLS.cpp
>+++ b/Source/bmalloc/bmalloc/IsoTLS.cpp
>@@ -123,11 +123,24 @@ IsoTLS* IsoTLS::ensureEntries(unsigned offset)
>     }
>     
>     if (startEntry) {
>-        startEntry->walkUpToInclusive(
>-            targetEntry,
>-            [&] (IsoTLSEntry* entry) {
>-                entry->construct(tls->m_data + entry->offset());
>-            });
>+        // FIXME: IsoTLS::ensureEntries can be cleaned up. For example, startEntry should be `oldLastEntry ? oldLastEntry->m_next : layout.head()` instead,
>+        // because we already ensured that `RELEASE_BASSERT(!oldLastEntry || oldLastEntry->offset() < offset)`. And startEntry is never nullptr.
>+        // https://bugs.webkit.org/show_bug.cgi?id=199103
>+        if (oldLastEntry) {
>+            if (oldLastEntry != targetEntry) {
>+                oldLastEntry->m_next->walkUpToInclusive(
>+                    targetEntry,
>+                    [&] (IsoTLSEntry* entry) {
>+                        entry->construct(tls->m_data + entry->offset());
>+                    });
>+            }
>+        } else {
>+            startEntry->walkUpToInclusive(
>+                targetEntry,
>+                [&] (IsoTLSEntry* entry) {
>+                    entry->construct(tls->m_data + entry->offset());
>+                });
>+        }
>         
>         tls->m_lastEntry = targetEntry;
>         tls->m_extent = targetEntry->extent();

Actions: View | Formatted Diff | Diff

Attachments on bug 199077: 372622 | 372623 | 372639