com.apple.WebKit.Networking.Development crashes in WebCore::formOpen()
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 2 Mar 2016 00:01:31 +0000 (00:01 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 2 Mar 2016 00:01:31 +0000 (00:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=154682
<rdar://problem/23550269>

Reviewed by Brent Fulgham.

Speculative fix for a race condition when opening the stream for the next form data element.
Calling CFReadStreamOpen(s) in WebCore::openNextStream() can cause stream s to be closed and
deallocated before CFReadStreamOpen(s) returns.

When WebCore::openNextStream() is called it closes and deallocates the current stream and
then opens a new stream for the next form data element. Calling CFReadStreamOpen() in
WebCore::openNextStream() can lead to WebCore::openNextStream() being re-entered via
WebCore::formEventCallback() from another thread. One example when this can occur is when
the stream being opened has no data (i.e. WebCore::formEventCallback() is called
back with event type kCFStreamEventEndEncountered).

I have been unable to reproduce this crash. We know that it occurs from crash reports.

* platform/network/cf/FormDataStreamCFNet.cpp:
(WebCore::closeCurrentStream): Assert that we had acquired a lock to close the stream.
(WebCore::advanceCurrentStream): Assert that we had acquired a lock to advance the stream.
(WebCore::openNextStream): Acquire a lock before we open the next stream to ensure that
exactly one thread executes this critical section at a time.
(WebCore::formFinalize): Acquire a lock before we close the current stream.
(WebCore::formClose): Ditto.

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

Source/WebCore/ChangeLog
Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp

index 136cf17..3232548 100644 (file)
@@ -1,3 +1,32 @@
+2016-03-01  Daniel Bates  <dabates@apple.com>
+
+        com.apple.WebKit.Networking.Development crashes in WebCore::formOpen()
+        https://bugs.webkit.org/show_bug.cgi?id=154682
+        <rdar://problem/23550269>
+
+        Reviewed by Brent Fulgham.
+
+        Speculative fix for a race condition when opening the stream for the next form data element.
+        Calling CFReadStreamOpen(s) in WebCore::openNextStream() can cause stream s to be closed and
+        deallocated before CFReadStreamOpen(s) returns.
+
+        When WebCore::openNextStream() is called it closes and deallocates the current stream and
+        then opens a new stream for the next form data element. Calling CFReadStreamOpen() in
+        WebCore::openNextStream() can lead to WebCore::openNextStream() being re-entered via
+        WebCore::formEventCallback() from another thread. One example when this can occur is when
+        the stream being opened has no data (i.e. WebCore::formEventCallback() is called
+        back with event type kCFStreamEventEndEncountered).
+
+        I have been unable to reproduce this crash. We know that it occurs from crash reports.
+
+        * platform/network/cf/FormDataStreamCFNet.cpp:
+        (WebCore::closeCurrentStream): Assert that we had acquired a lock to close the stream.
+        (WebCore::advanceCurrentStream): Assert that we had acquired a lock to advance the stream.
+        (WebCore::openNextStream): Acquire a lock before we open the next stream to ensure that
+        exactly one thread executes this critical section at a time.
+        (WebCore::formFinalize): Acquire a lock before we close the current stream.
+        (WebCore::formClose): Ditto.
+
 2016-03-01  Michael Saboff  <msaboff@apple.com>
 
         ASSERT in platform/graphics/mac/ComplexTextController.cpp::capitalize()
index c126824..486133c 100644 (file)
@@ -109,10 +109,13 @@ struct FormStreamFields {
     CFReadStreamRef formStream;
     unsigned long long streamLength;
     unsigned long long bytesSent;
+    Lock streamIsBeingOpenedOrClosedLock;
 };
 
 static void closeCurrentStream(FormStreamFields* form)
 {
+    ASSERT(form->streamIsBeingOpenedOrClosedLock.isHeld());
+
     if (form->currentStream) {
         CFReadStreamClose(form->currentStream);
         CFReadStreamSetClient(form->currentStream, kCFStreamEventNone, 0, 0);
@@ -127,6 +130,8 @@ static void closeCurrentStream(FormStreamFields* form)
 // Return false if we cannot advance the stream. Currently the only possible failure is that the underlying file has been removed or changed since File.slice.
 static bool advanceCurrentStream(FormStreamFields* form)
 {
+    ASSERT(form->streamIsBeingOpenedOrClosedLock.isHeld());
+
     closeCurrentStream(form);
 
     if (form->remainingElements.isEmpty())
@@ -176,6 +181,10 @@ static bool advanceCurrentStream(FormStreamFields* form)
 
 static bool openNextStream(FormStreamFields* form)
 {
+    // CFReadStreamOpen() can cause this function to be re-entered from another thread before it returns.
+    // One example when this can occur is when the stream being opened has no data. See <rdar://problem/23550269>.
+    LockHolder locker(form->streamIsBeingOpenedOrClosedLock);
+
     // Skip over any streams we can't open.
     if (!advanceCurrentStream(form))
         return false;
@@ -213,7 +222,10 @@ static void formFinalize(CFReadStreamRef stream, void* context)
     ASSERT_UNUSED(stream, form->formStream == stream);
 
     callOnMainThread([form] {
-        closeCurrentStream(form);
+        {
+            LockHolder locker(form->streamIsBeingOpenedOrClosedLock);
+            closeCurrentStream(form);
+        }
         delete form;
     });
 }
@@ -281,6 +293,7 @@ static Boolean formCanRead(CFReadStreamRef stream, void* context)
 static void formClose(CFReadStreamRef, void* context)
 {
     FormStreamFields* form = static_cast<FormStreamFields*>(context);
+    LockHolder locker(form->streamIsBeingOpenedOrClosedLock);
 
     closeCurrentStream(form);
 }