[webkit-reviews] review denied: [Bug 198320] [curl] Heap corruption in ~CurlResponse : [Attachment 374940] Patch

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Takashi Komori
<Takashi.Komori at sony.com>'s request for review:
Bug 198320: [curl] Heap corruption in ~CurlResponse
https://bugs.webkit.org/show_bug.cgi?id=198320

Attachment 374940: Patch

https://bugs.webkit.org/attachment.cgi?id=374940&action=review




--- Comment #11 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 374940
  --> https://bugs.webkit.org/attachment.cgi?id=374940
Patch

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

> Source/WebCore/platform/network/curl/CurlRequest.cpp:464
> +	   auto resourceError = ResourceError::httpError(result,
m_request.url().isolatedCopy(), type);

CurlRequest::didCompleteTransfer is called in the worker thread.
You are going to create an isolated copy of m_request.url() by doing
m_request.url().isolatedCopy().
Strictly speaking this is not allowed.
Because the URL object can be modified in the main thread at the same time.
Please file a another bug at the moment.

> Source/WebCore/platform/network/curl/CurlRequest.cpp:601
> +	   invokeDidReceiveResponse(response, Action::StartTransfer);

This code doesn't look good.
'response' object is destructed in the worker thread.
If objects in 'response' object would be copied in invokeDidReceiveResponse,
ref-counter of those objects are accessed in both threads.
To avoid such problem, you should pass the 'response' by using WTFMove.
> invokeDidReceiveResponse(WTFMove(response), Action::StartTransfer);

As far as I read the code, there is no such copying in
invokeDidReceiveResponse.
So, please file another bug at the moment.

> Source/WebCore/platform/network/curl/CurlRequest.h:144
> +    bool invokeDidReceiveResponseForFileIfNeeded();

Why did you change this? The previous code looks good to me.