60594 2011-05-10 16:06:43 -0700 Protect self in [WebCoreResourceHandleAsDelegate connection:didReceiveDataArray:] 2011-05-10 17:47:02 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 psolanki psolanki ap psolanki oldest_to_newest 401605 0 psolanki 2011-05-10 16:06:43 -0700 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. 401606 1 psolanki 2011-05-10 16:07:16 -0700 <rdar://problem/9203259> 401617 2 93038 psolanki 2011-05-10 16:20:24 -0700 Created attachment 93038 Patch 401631 3 93038 ap 2011-05-10 16:31:52 -0700 Comment on attachment 93038 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; 401667 4 93038 psolanki 2011-05-10 17:31:31 -0700 Comment on attachment 93038 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. 401674 5 psolanki 2011-05-10 17:47:02 -0700 Committed r86200: <http://trac.webkit.org/changeset/86200> 93038 2011-05-10 16:20:24 -0700 2011-05-10 17:31:31 -0700 Patch bug-60594-20110510162022.patch text/plain 2405 psolanki U3VidmVyc2lvbiBSZXZpc2lvbjogODYxNjMKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAxZWZhNzdlMGFhY2VhZWE2 ZmUzZTg0YjU5YTEyYjI4ZDQ0ZTA2Zjk4Li5kYjkzMjVhMzc5ZGU3YWU0MzE4MmE1ZTg4MWVkMzJi MmM2MTljMTEwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTEtMDUtMTAgIFByYXRp ayBTb2xhbmtpICA8cHNvbGFua2lAYXBwbGUuY29tPgorCisgICAgICAgIFByb3RlY3Qgc2VsZiBp biBbV2ViQ29yZVJlc291cmNlSGFuZGxlQXNEZWxlZ2F0ZSBjb25uZWN0aW9uOmRpZFJlY2VpdmVE YXRhQXJyYXk6XQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9NjA1OTQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICAqIHBsYXRmb3JtL25ldHdvcmsvbWFjL1Jlc291cmNlSGFuZGxlTWFjLm1tOgorICAgICAgICAo LVtXZWJDb3JlUmVzb3VyY2VIYW5kbGVBc0RlbGVnYXRlIGNvbm5lY3Rpb246ZGlkUmVjZWl2ZURh dGFBcnJheTpdKTogVGhlIGRpZFJlY2VpdmVEYXRhKCkKKyAgICAgICAgY2FsbGJhY2sgb24gY2xp ZW50IGNhbiByZXN1bHQgaW4gdGhlIGxvYWQgYmVpbmcgY2FuY2VsbGVkLiBUaGlzIHJlc3VsdHMg aW4gdGhlIGRlbGVnYXRlCisgICAgICAgIChzZWxmKSBiZWluZyBmcmVlZC4gIFByb3RlY3Qgc2Vs ZiBkdXJpbmcgdGhlIGxvb3Agc28gd2UgY2FuIGNoZWNrIGZvciBtX2hhbmRsZSBhbmQgc2FmZWx5 CisgICAgICAgIHJldHVybiB3aXRob3V0IGNyYXNoaW5nLgorCiAyMDExLTA1LTEwICBFcmljIENh cmxzb24gIDxlcmljLmNhcmxzb25AYXBwbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IERh cmluIEFkbGVyLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9t YWMvUmVzb3VyY2VIYW5kbGVNYWMubW0gYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3Jr L21hYy9SZXNvdXJjZUhhbmRsZU1hYy5tbQppbmRleCAzY2RkOTM5ZjU1ODc2ZWFlYTI0MmZiMTlh ZGQ2YTQwZjlmYzhlZGVlLi4zZTk3MjUxZmZiNjJlMGIzMDNjZmEzMDgwOGNjNTg4YTg2MjE5Zjg5 IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL21hYy9SZXNvdXJj ZUhhbmRsZU1hYy5tbQorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL21hYy9S ZXNvdXJjZUhhbmRsZU1hYy5tbQpAQCAtODI2LDggKzgyNiwxNCBAQCBTdHJpbmcgUmVzb3VyY2VI YW5kbGU6OnByaXZhdGVCcm93c2luZ1N0b3JhZ2VTZXNzaW9uSWRlbnRpZmllckRlZmF1bHRCYXNl KCkKICAgICBpZiAobV9oYW5kbGUtPmNsaWVudCgpLT5zdXBwb3J0c0RhdGFBcnJheSgpKQogICAg ICAgICBtX2hhbmRsZS0+Y2xpZW50KCktPmRpZFJlY2VpdmVEYXRhQXJyYXkobV9oYW5kbGUsIHJl aW50ZXJwcmV0X2Nhc3Q8Q0ZBcnJheVJlZj4oZGF0YUFycmF5KSk7CiAgICAgZWxzZSB7Ci0gICAg ICAgIGZvciAoTlNEYXRhICpkYXRhIGluIGRhdGFBcnJheSkKLSAgICAgICAgICAgIG1faGFuZGxl LT5jbGllbnQoKS0+ZGlkUmVjZWl2ZURhdGEobV9oYW5kbGUsIHN0YXRpY19jYXN0PGNvbnN0IGNo YXIqPihbZGF0YSBieXRlc10pLCBbZGF0YSBsZW5ndGhdLCBzdGF0aWNfY2FzdDxpbnQ+KFtkYXRh IGxlbmd0aF0pKTsKKyAgICAgICAgLy8gVGhlIGNhbGwgdG8gZGlkUmVjZWl2ZURhdGEgYmVsb3cg Y291bGQgY2FuY2VsIGEgbG9hZCwgd2hpY2ggd291bGQgcmVzdWx0IGluIHRoZSBkZWxlZ2F0ZQor ICAgICAgICAvLyAoc2VsZikgYmVpbmcgcmVsZWFzZWQuIFJldGFpbiBzZWxmIHNvIHdlIGNhbiBh dCBsZWFzdCBjaGVjayB0aGUgdmFsdWUgb2YgbV9oYW5kbGUgd2l0aG91dAorICAgICAgICAvLyBj cmFzaGluZyBhbmQgcmV0dXJuIHNhZmVseSBpbiBjYXNlIHRoZSBsb2FkIGdvdCBjYW5jZWxsZWQu CisgICAgICAgIFJldGFpblB0cjxXZWJDb3JlUmVzb3VyY2VIYW5kbGVBc0RlbGVnYXRlPiBwcm90 ZWN0RGVsZWdhdGUoc2VsZik7CisgICAgICAgIGZvciAoTlNEYXRhICpkYXRhIGluIGRhdGFBcnJh eSkgeworICAgICAgICAgICAgaWYgKG1faGFuZGxlICYmIG1faGFuZGxlLT5jbGllbnQoKSkKKyAg ICAgICAgICAgICAgICBtX2hhbmRsZS0+Y2xpZW50KCktPmRpZFJlY2VpdmVEYXRhKG1faGFuZGxl LCBzdGF0aWNfY2FzdDxjb25zdCBjaGFyKj4oW2RhdGEgYnl0ZXNdKSwgW2RhdGEgbGVuZ3RoXSwg c3RhdGljX2Nhc3Q8aW50PihbZGF0YSBsZW5ndGhdKSk7CisgICAgICAgIH0KICAgICB9CiAgICAg cmV0dXJuOwogfQo=