| Summary: | [curl] Heap corruption in ~CurlResponse | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||||
| Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | commit-queue, don.olmstead, ews-watchlist, galpeter, ross.kirsling, ryanhaddad, takashi.komori | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Fujii Hironori
2019-05-28 18:03:36 PDT
Created attachment 370817 [details]
t0905-c5526-fltclr-00-c-ag-crash-log.txt
This is a crash log of css2.1/t0905-c5526-fltclr-00-c-ag.html.
Created attachment 374770 [details]
Patch
WTF::StringImpl has reference counter but ref/deref operation is not thread safe. In CurlRequest::start(), String object is used in thread unsafe way implicitly and causes crash. ## How CurlRequest crash. -- Main Thread ------------------ 1) URL object url is generated by isolatedCopy in CurlRequest::start(). 2) CurlRequst::invokeDidReceiveResponseForFile is called. 3) url is copied to m_response.url 4) Woker task is generated. 5) return to CurlRequest::start() 6) At end of CurlRequest::start() url is destructed generated in (1 -- Worker Thread ------------------ a) Start CurlRequest::invokeDidReceiveResponse() as a worker thread. b) response is copied by isolatedCopy(). c) CurlResponse::isolatedCopy() copies url by using URL::isolatedCopy(). d) URL::isolatedCopy first copies itself to result. e) result.m_string is overwritten in URL::isolatedCopy(). Step 1) 3) d) increase reference counter of the same string object. Step 6) e) decrease the reference counter. Because StringImpl::deref doesn't change m_refCount atomically, in this sequence the counter becomes 0 by timing and wrongly destroy the String object and the destroied object causes the crashes. Comment on attachment 374770 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374770&action=review > Source/WebCore/platform/network/curl/CurlRequest.cpp:597 > + response.url = m_request.url(); You are incrementing ref counter of m_request.url() in the worker thread here. > Source/WebCore/platform/network/curl/CurlRequest.cpp:601 > + invokeDidReceiveResponse(response, Action::StartTransfer); CurlResponse is destructed at the end of this lambda scope. It destructs the copied URL in the worker thread here. (In reply to Fujii Hironori from comment #5) > Comment on attachment 374770 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374770&action=review > > > Source/WebCore/platform/network/curl/CurlRequest.cpp:597 > > + response.url = m_request.url(); > > You are incrementing ref counter of m_request.url() in the worker thread > here. > > > Source/WebCore/platform/network/curl/CurlRequest.cpp:601 > > + invokeDidReceiveResponse(response, Action::StartTransfer); > > CurlResponse is destructed at the end of this lambda scope. It destructs the > copied URL in the worker thread here. m_request object should be reffered by only worker thread except in contructor. So the operation in lamda is valid. (In reply to Fujii Hironori from comment #5) > Comment on attachment 374770 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374770&action=review > > > Source/WebCore/platform/network/curl/CurlRequest.cpp:597 > > + response.url = m_request.url(); > > You are incrementing ref counter of m_request.url() in the worker thread > here. > > > Source/WebCore/platform/network/curl/CurlRequest.cpp:601 > > + invokeDidReceiveResponse(response, Action::StartTransfer); > > CurlResponse is destructed at the end of this lambda scope. It destructs the > copied URL in the worker thread here. m_request object should be reffered by only worker thread except in constructor. So copying URL in the lamda is valid. Sorry I submitted double comments wrongly.. I'm talking about m_request.url() not m_request. WTF::URL has a ref-counted object WTF::String. Its StringImpl can be shared with other strings in the main thread. Created attachment 374940 [details]
Patch
(In reply to Fujii Hironori from comment #8) > I'm talking about m_request.url() not m_request. > WTF::URL has a ref-counted object WTF::String. > Its StringImpl can be shared with other strings in the main thread. Made invokeDidReceiveResponseForFileIfNeeded() pass URL object to worker thread by crossThreadCopy. Comment on attachment 374940 [details] 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. (In reply to Fujii Hironori from comment #11) > Comment on attachment 374940 [details] > 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. > Made ticket to fix ref-conted objects issue. https://bugs.webkit.org/show_bug.cgi?id=200156 Created attachment 374957 [details]
Patch
(In reply to Fujii Hironori from comment #11) > > Source/WebCore/platform/network/curl/CurlRequest.h:144 > > + bool invokeDidReceiveResponseForFileIfNeeded(); > > Why did you change this? The previous code looks good to me. Reverted. Comment on attachment 374957 [details] Patch Clearing flags on attachment: 374957 Committed r247874: <https://trac.webkit.org/changeset/247874> All reviewed patches have been landed. Closing bug. |