Bug 60594 – Protect self in [WebCoreResourceHandleAsDelegate connection:didReceiveDataArray:]

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Pratik Solanki]

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.

[Pratik Solanki]

<rdar://problem/9203259>

[Pratik Solanki]

Created attachment 93038 [details]
Patch

[Alexey Proskuryakov]

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;

[Pratik Solanki]

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.

[Pratik Solanki]

Committed r86200: <http://trac.webkit.org/changeset/86200>