Cherry-pick 252432.1018@safari-7614-branch (792c09f). https://bugs.we…

…bkit.org/show_bug.cgi?id=249996 Use-after-free in FetchBodyConsumer::resolve https://bugs.webkit.org/show_bug.cgi?id=249996 rdar://103649054 Reviewed by Jonathan Bedard and Alex Christensen. Make sure in FetchBodyConsumer that refed promise/source remain protected. We also revert part of an unnecessary and wrong change from https://trac.webkit.org/changeset/227760. This makes sure ReadableStreamToSharedBufferSink callback remains valid until completely executed in close case, as was the case in error case. We use std::exchange instead of move as it is more semantically correct. Covered by added test. * LayoutTests/streams/blob-and-then-expected.txt: Added. * LayoutTests/streams/blob-and-then.html: Added. * Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp: (WebCore::FetchBodyConsumer::resolveWithFormData): (WebCore::FetchBodyConsumer::consumeFormDataAsStream): (WebCore::FetchBodyConsumer::resolve): * Source/WebCore/Modules/streams/ReadableStreamSink.cpp: (WebCore::ReadableStreamToSharedBufferSink::close): (WebCore::ReadableStreamToSharedBufferSink::error): Canonical link: https://commits.webkit.org/252432.1018@safari-7614-branch
WebKit · Apr 5, 2023 · 6f56628 · 6f56628
1 parent 2d9e8d1
commit 6f56628
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 13 deletions.
diff --git a/LayoutTests/streams/blob-and-then-expected.txt b/LayoutTests/streams/blob-and-then-expected.txt
@@ -0,0 +1,3 @@
+
+PASS Ensure redefining then does not alter blob promise
+
diff --git a/LayoutTests/streams/blob-and-then.html b/LayoutTests/streams/blob-and-then.html
@@ -0,0 +1,33 @@
+<!doctype html>
+<html>
+<head>
+<meta charset="utf-8">
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+</head>
+<body>
+<script>
+
+promise_test(async () => {
+    function createReadStream() {
+        const response = new Response(new Blob(['aaaaa']));
+        return response.body;
+    }
+
+    const f = document.body.appendChild(document.createElement('iframe'));
+    const response = new f.contentWindow.Response(createReadStream());
+
+    f.contentWindow.Object.prototype.__defineGetter__('then', () => {
+        delete f.contentWindow.Object.prototype.then;
+
+        f.remove();
+    });
+
+    response.blob();
+
+    await new Promise(resolve => setTimeout(resolve, 50));
+}, "Ensure redefining then does not alter blob promise");
+
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp b/Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp
@@ -267,17 +267,18 @@ void FetchBodyConsumer::resolveWithFormData(Ref<DeferredPromise>&& promise, cons
     if (!context)
         return;
 
-    m_formDataConsumer = makeUnique<FormDataConsumer>(formData, *context, [this, capturedPromise = WTFMove(promise), contentType, builder = SharedBufferBuilder { }](auto&& result) mutable {
+    m_formDataConsumer = makeUnique<FormDataConsumer>(formData, *context, [this, promise = WTFMove(promise), contentType, builder = SharedBufferBuilder { }](auto&& result) mutable {
         if (result.hasException()) {
-            auto promise = WTFMove(capturedPromise);
-            promise->reject(result.releaseException());
+            auto protectedPromise = WTFMove(promise);
+            protectedPromise->reject(result.releaseException());
             return;
         }
 
         auto& value = result.returnValue();
         if (value.empty()) {
+            auto protectedPromise = WTFMove(promise);
             auto buffer = builder.takeAsContiguous();
-            resolveWithData(WTFMove(capturedPromise), contentType, buffer->data(), buffer->size());
+            resolveWithData(WTFMove(protectedPromise), contentType, buffer->data(), buffer->size());
             return;
         }
 
@@ -297,6 +298,7 @@ void FetchBodyConsumer::consumeFormDataAsStream(const FormData& formData, FetchB
         return;
 
     m_formDataConsumer = makeUnique<FormDataConsumer>(formData, *context, [this, source = Ref { source }](auto&& result) {
+        auto protectedSource = source;
         if (result.hasException()) {
             source->error(result.releaseException());
             return;
@@ -326,16 +328,20 @@ void FetchBodyConsumer::resolve(Ref<DeferredPromise>&& promise, const String& co
         ASSERT(!m_sink);
         m_sink = ReadableStreamToSharedBufferSink::create([promise = WTFMove(promise), data = SharedBufferBuilder(), type = m_type, contentType](auto&& result) mutable {
             if (result.hasException()) {
-                promise->reject(result.releaseException());
+                auto protectedPromise = WTFMove(promise);
+                protectedPromise->reject(result.releaseException());
                 return;
             }
 
-            if (auto* chunk = result.returnValue())
-                data.append(chunk->data(), chunk->size());
-            else {
+            auto* chunk = result.returnValue();
+            if (!chunk) {
+                auto protectedPromise = WTFMove(promise);
                 auto buffer = data.takeAsContiguous();
-                resolveWithTypeAndData(WTFMove(promise), type, contentType, buffer->data(), buffer->size());
+                resolveWithTypeAndData(WTFMove(protectedPromise), type, contentType, buffer->data(), buffer->size());
+                return;
             }
+
+            data.append(chunk->data(), chunk->size());
         });
         m_sink->pipeFrom(*stream);
         return;

diff --git a/Source/WebCore/Modules/streams/ReadableStreamSink.cpp b/Source/WebCore/Modules/streams/ReadableStreamSink.cpp
@@ -57,14 +57,20 @@ void ReadableStreamToSharedBufferSink::enqueue(const BufferSource& buffer)
 
 void ReadableStreamToSharedBufferSink::close()
 {
-    if (m_callback)
-        m_callback(nullptr);
+    if (!m_callback)
+        return;
+
+    auto callback = std::exchange(m_callback, { });
+    callback(nullptr);
 }
 
 void ReadableStreamToSharedBufferSink::error(String&& message)
 {
-    if (auto callback = WTFMove(m_callback))
-        callback(Exception { TypeError, WTFMove(message) });
+    if (!m_callback)
+        return;
+
+    auto callback = std::exchange(m_callback, { });
+    callback(Exception { TypeError, WTFMove(message) });
 }
 
 } // namespace WebCore