234916 2022-01-06 03:17:14 -0800 length argument passed to didReceiveData can never be negative. 2022-01-06 05:04:36 -0800 1 1 1 Unclassified WebKit WebCore Misc. Other Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 232424 1 jean-yves.avenard jean-yves.avenard ews-watchlist hi joepeck pangle webkit-bug-importer youennf oldest_to_newest 1828551 0 jean-yves.avenard 2022-01-06 03:17:14 -0800 As seen in bug 232424; We have code pattern such as: https://webkit-search.igalia.com/webkit/source/Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp#127-136 ``` void didReceiveData(const uint8_t* data, int dataLength) override { if (!dataLength) return; if (dataLength == -1) dataLength = strlen(reinterpret_cast<const char*>(data)); m_responseText.append(m_decoder->decode(data, dataLength)); } ``` If the dataLength argument passed is negative; then the length will be calculated from running strlen on the uint8_t buffer. We also have this code pattern in: - XMLHttpRequest::didReceiveData and: - WorkerScriptLoader::didReceiveData The common code path to end in those routines is from : DocumentThreadableLoader::didReceiveData https://webkit-search.igalia.com/webkit/source/Source/WebCore/loader/DocumentThreadableLoader.cpp#447-455 The data itself originates from the network process where to carry the size it will use either a size_t (unsigned) or a FragmentedSharedBuffer (also unsigned size() ) so the size_t ends up being cast to an int64_t over IPC and cast again to an int. If the data is cached; it originates from CachedRawResource::didAddClient https://webkit-search.igalia.com/webkit/source/Source/WebCore/loader/cache/CachedRawResource.cpp#172-177 if (m_data) { m_data->forEachSegment([&](auto& segment) { if (hasClient(*client)) client->dataReceived(*this, segment.data(), segment.size()); }); } and here segment.size() returns a size_t if the data is not cached, it comes from: void SubresourceLoader::didReceiveBuffer or SubresourceLoader::didReceiveData; which in either case will use an unsigned for the dataLength. For non-webkit 2 code; the curl library always use size_t to pass the data length ; cocoa platform are using NSData which also uses an unsigned int to specify the size. In some code such as: bool FrameLoader::willLoadMediaElementURL we have length set to -1: https://webkit-search.igalia.com/webkit/source/Source/WebCore/loader/FrameLoader.cpp#1690 however, in this case the const uint8_t* is null and as such the length parameter is not used. length/dataLength as used with didReceiveData methods can never be negative. To simplify bug 232424, as mentioned in the review, we should simplify the code separately and remove the test checking that the length == -1. 1828552 1 webkit-bug-importer 2022-01-06 03:17:50 -0800 <rdar://problem/87190340> 1828555 2 448485 jean-yves.avenard 2022-01-06 03:53:14 -0800 Created attachment 448485 Patch 1828568 3 ews-feeder 2022-01-06 05:04:33 -0800 Committed r287681 (245777@main): <https://commits.webkit.org/245777@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448485. 448485 2022-01-06 03:53:14 -0800 2022-01-06 05:04:35 -0800 Patch bug-234916-20220106225312.patch text/plain 2818 jean-yves.avenard U3VidmVyc2lvbiBSZXZpc2lvbjogMjg3NjcyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMmVmZDZkNDY3NWVlYTcx ODdlN2YzMzA4YjFkN2JiNzY3OWNlZjZhOS4uOWJhYjUwOTg4NTc0M2ExZDhmNGY1NmRjZGIxM2I5 MzE3ZTEyNzQzMyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDIyLTAxLTA2ICBKZWFu LVl2ZXMgQXZlbmFyZCAgPGp5YUBhcHBsZS5jb20+CisKKyAgICAgICAgbGVuZ3RoIGFyZ3VtZW50 IHBhc3NlZCB0byBkaWRSZWNlaXZlRGF0YSBjYW4gbmV2ZXIgYmUgbmVnYXRpdmUuCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMzQ5MTYKKyAgICAgICAg cmRhcjovL3Byb2JsZW0vODcxOTAzNDAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBTaWduZWQgaW50IGFyZ3VtZW50IHdpbGwgYmUgcmVtb3ZlZCBpbiBi dWcgMjMyNDI0LgorCisgICAgICAgIE5vIGNoYW5nZSBpbiBvYnNlcnZhYmxlIGJlaGF2aW91ci4g Q292ZXJlZCBieSBleGlzdGluZyB0ZXN0cy4KKworICAgICAgICAqIGluc3BlY3Rvci9hZ2VudHMv SW5zcGVjdG9yTmV0d29ya0FnZW50LmNwcDoKKyAgICAgICAgKiB3b3JrZXJzL1dvcmtlclNjcmlw dExvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpXb3JrZXJTY3JpcHRMb2FkZXI6OmRpZFJl Y2VpdmVEYXRhKToKKyAgICAgICAgKiB4bWwvWE1MSHR0cFJlcXVlc3QuY3BwOgorICAgICAgICAo V2ViQ29yZTo6WE1MSHR0cFJlcXVlc3Q6OmRpZFJlY2VpdmVEYXRhKToKKwogMjAyMi0wMS0wNSAg V2Vuc29uIEhzaWVoICA8d2Vuc29uX2hzaWVoQGFwcGxlLmNvbT4KIAogICAgICAgICBNb2RhbCBj b250YWluZXIgb2JzZXJ2ZXIgc2hvdWxkIGRldGVjdCBhbmQgc3VwcHJlc3MgZWxlbWVudHMgdGhh dCBwcmV2ZW50IHVzZXIgaW50ZXJhY3Rpb24KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2lu c3BlY3Rvci9hZ2VudHMvSW5zcGVjdG9yTmV0d29ya0FnZW50LmNwcCBiL1NvdXJjZS9XZWJDb3Jl L2luc3BlY3Rvci9hZ2VudHMvSW5zcGVjdG9yTmV0d29ya0FnZW50LmNwcAppbmRleCAzY2ViNGI0 N2YxMDY4YTBhZmVhZjAwZjE3OWMyODAwYzQ3NmJkNmIxLi4zYTEzYjg1MmQ4MmQyNWVlNTExYWU5 Mjc3OTkzNmNkZDBmMGYzNTU4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9pbnNwZWN0b3Iv YWdlbnRzL0luc3BlY3Rvck5ldHdvcmtBZ2VudC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvaW5z cGVjdG9yL2FnZW50cy9JbnNwZWN0b3JOZXR3b3JrQWdlbnQuY3BwCkBAIC0xMjksOSArMTI5LDYg QEAgcHVibGljOgogICAgICAgICBpZiAoIWRhdGFMZW5ndGgpCiAgICAgICAgICAgICByZXR1cm47 CiAKLSAgICAgICAgaWYgKGRhdGFMZW5ndGggPT0gLTEpCi0gICAgICAgICAgICBkYXRhTGVuZ3Ro ID0gc3RybGVuKHJlaW50ZXJwcmV0X2Nhc3Q8Y29uc3QgY2hhcio+KGRhdGEpKTsKLQogICAgICAg ICBtX3Jlc3BvbnNlVGV4dC5hcHBlbmQobV9kZWNvZGVyLT5kZWNvZGUoZGF0YSwgZGF0YUxlbmd0 aCkpOwogICAgIH0KIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvd29ya2Vycy9Xb3JrZXJT Y3JpcHRMb2FkZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvd29ya2Vycy9Xb3JrZXJTY3JpcHRMb2Fk ZXIuY3BwCmluZGV4IGJmNWE0NGZlMzQ4ZDJmZmNlOTE2ZmIxOTQ5MTVlNjdiZmRhOWFlYzEuLjMy MzVkY2ViNjFkOWNiODU2MDJhZWJmY2ZjODlmZmQzYTRlNjA5ZTcgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL3dvcmtlcnMvV29ya2VyU2NyaXB0TG9hZGVyLmNwcAorKysgYi9Tb3VyY2UvV2Vi Q29yZS93b3JrZXJzL1dvcmtlclNjcmlwdExvYWRlci5jcHAKQEAgLTIxNSw5ICsyMTUsNiBAQCB2 b2lkIFdvcmtlclNjcmlwdExvYWRlcjo6ZGlkUmVjZWl2ZURhdGEoY29uc3QgdWludDhfdCogZGF0 YSwgaW50IGxlbikKICAgICBpZiAoIWxlbikKICAgICAgICAgcmV0dXJuOwogICAgIAotICAgIGlm IChsZW4gPT0gLTEpCi0gICAgICAgIGxlbiA9IHN0cmxlbihyZWludGVycHJldF9jYXN0PGNvbnN0 IGNoYXIqPihkYXRhKSk7Ci0KICAgICBtX3NjcmlwdC5hcHBlbmQobV9kZWNvZGVyLT5kZWNvZGUo ZGF0YSwgbGVuKSk7CiB9CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3htbC9YTUxIdHRw UmVxdWVzdC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS94bWwvWE1MSHR0cFJlcXVlc3QuY3BwCmluZGV4 IDliMTEyYjlkYjJjZDg0ZDAzMmY1OWRhZmE4ZThlMzhlMDcwZTUzZjcuLmZmOGRjYTVhMzlhMDE0 NTAwN2M1NDVhZTFjOWY0MWU5ZWE5MGQyZGMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3ht bC9YTUxIdHRwUmVxdWVzdC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUveG1sL1hNTEh0dHBSZXF1 ZXN0LmNwcApAQCAtMTA1NSw5ICsxMDU1LDYgQEAgdm9pZCBYTUxIdHRwUmVxdWVzdDo6ZGlkUmVj ZWl2ZURhdGEoY29uc3QgdWludDhfdCogZGF0YSwgaW50IGxlbikKICAgICBpZiAoIWxlbikKICAg ICAgICAgcmV0dXJuOwogCi0gICAgaWYgKGxlbiA9PSAtMSkKLSAgICAgICAgbGVuID0gc3RybGVu KHJlaW50ZXJwcmV0X2Nhc3Q8Y29uc3QgY2hhcio+KGRhdGEpKTsKLQogICAgIGlmICh1c2VEZWNv ZGVyKQogICAgICAgICBtX3Jlc3BvbnNlQnVpbGRlci5hcHBlbmQobV9kZWNvZGVyLT5kZWNvZGUo ZGF0YSwgbGVuKSk7CiAgICAgZWxzZSB7Cg==