Bug 234916 - length argument passed to didReceiveData can never be negative.
Summary: length argument passed to didReceiveData can never be negative.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on:
Blocks: 232424
  Show dependency treegraph
 
Reported: 2022-01-06 03:17 PST by Jean-Yves Avenard [:jya]
Modified: 2022-01-06 05:04 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.75 KB, patch)
2022-01-06 03:53 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Yves Avenard [:jya] 2022-01-06 03:17:14 PST
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.
Comment 1 Radar WebKit Bug Importer 2022-01-06 03:17:50 PST
<rdar://problem/87190340>
Comment 2 Jean-Yves Avenard [:jya] 2022-01-06 03:53:14 PST
Created attachment 448485 [details]
Patch
Comment 3 EWS 2022-01-06 05:04:33 PST
Committed r287681 (245777@main): <https://commits.webkit.org/245777@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448485 [details].