Bug 60594

Summary: Protect self in [WebCoreResourceHandleAsDelegate connection:didReceiveDataArray:]
Product: WebKit Reporter: Pratik Solanki <psolanki>
Component: PlatformAssignee: Pratik Solanki <psolanki>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, psolanki
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch ap: review+, ap: commit-queue-

Description Pratik Solanki 2011-05-10 16:06:43 PDT
In [WebCoreResourceHandleAsDelegate connection:didReceiveDataArray:], we do

       for (NSData *data in dataArray)
           m_handle->client()->didReceiveData(m_handle, static_cast<const char*>([data bytes]), [data length], static_cast<int>([data length]));

However, in the didReceiveData() callback, the client can cancel the load which would result in the delegate (self) being freed. This means we could crash in the subsequent iteration of the loop.
Comment 1 Pratik Solanki 2011-05-10 16:07:16 PDT
<rdar://problem/9203259>
Comment 2 Pratik Solanki 2011-05-10 16:20:24 PDT
Created attachment 93038 [details]
Patch
Comment 3 Alexey Proskuryakov 2011-05-10 16:31:52 PDT
Comment on attachment 93038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93038&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=60594

Please add a Radar link here, too.

Also, there needs to be an explanation why there are no tests (e.g. "no bot tests CFNETWORK_DATA_ARRAY_CALLBACK").

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:831
> +        // The call to didReceiveData below could cancel a load, which would result in the delegate
> +        // (self) being released. Retain self so we can at least check the value of m_handle without
> +        // crashing and return safely in case the load got cancelled.

Please remove the second sentence - it just states what the code below does.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:832
> +        RetainPtr<WebCoreResourceHandleAsDelegate> protectDelegate(self);

protect(self) reads more like an English sentence than protectDelegate(self).

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:835
> +            if (m_handle && m_handle->client())
> +                m_handle->client()->didReceiveData(m_handle, static_cast<const char*>([data bytes]), [data length], static_cast<int>([data length]));

Is the check for m_handle->client() necessary? Let's not add it unless it's necessary, since it would be confusing readers otherwise.

I'd have used an early return:

            if (!m_handle)
                break;
Comment 4 Pratik Solanki 2011-05-10 17:31:31 PDT
Comment on attachment 93038 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93038&action=review

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=60594
> 
> Please add a Radar link here, too.
> 
> Also, there needs to be an explanation why there are no tests (e.g. "no bot tests CFNETWORK_DATA_ARRAY_CALLBACK").

Done.

>> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:831
>> +        // crashing and return safely in case the load got cancelled.
> 
> Please remove the second sentence - it just states what the code below does.

Ok.

>> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:835
>> +                m_handle->client()->didReceiveData(m_handle, static_cast<const char*>([data bytes]), [data length], static_cast<int>([data length]));
> 
> Is the check for m_handle->client() necessary? Let's not add it unless it's necessary, since it would be confusing readers otherwise.
> 
> I'd have used an early return:
> 
>             if (!m_handle)
>                 break;

Removed the check for m_handle->client() since its not necessary. And added the early return.

Thanks for the review. I'll commit with the changes you suggested.
Comment 5 Pratik Solanki 2011-05-10 17:47:02 PDT
Committed r86200: <http://trac.webkit.org/changeset/86200>