Bug 198320 – [curl] Heap corruption in ~CurlResponse

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

[curl] Heap corruption in ~CurlResponse

I run run-webkit-tests with WinCairo port Debug builds of trunk@245803.

> python ./Tools/Scripts/run-webkit-tests --debug  --wincairo --no-new-test-results --dump-render-tree

Some tests are crashing flaky with a following callstack.

STACK_TEXT:  
> 00007fff`26e12848 00007fff`26d4ecb1 ntdll!RtlpLogHeapFailure+0x45
> 00007fff`26e12850 00007fff`26d5ce22 ntdll!RtlFreeHeap+0x9a9a2
> 00007fff`26e12858 00007fff`235fc7eb ucrtbase!_free_base+0x1b
> 00007fff`26e12860 00007ffe`f792d924 WTF!WTF::fastFree+0x14
> 00007fff`26e12868 00007ffe`f79fd7cd WTF!WTF::StringImpl::destroy+0x1d
> 00007fff`26e12870 00007ffe`f7914781 WTF!WTF::StringImpl::deref+0x31
> 00007fff`26e12878 00007ffe`f791470f WTF!WTF::derefIfNotNull<WTF::StringImpl>+0x1f
> 00007fff`26e12880 00007ffe`f79145d8 WTF!WTF::RefPtr<WTF::StringImpl,WTF::DumbPtrTraits<WTF::StringImpl> >::~RefPtr+0x38
> 00007fff`26e12888 00007ffe`f79131a3 WTF!WTF::String::~String+0x13
> 00007fff`26e12890 00007ffe`f798b2a3 WTF!WTF::URL::~URL+0x13
> 00007fff`26e12898 00007ffe`e92f14cf WebKit!WebCore::CurlResponse::~CurlResponse+0x3f
> 00007fff`26e128a0 00007ffe`e92f2ac2 WebKit!WebCore::CurlRequest::~CurlRequest+0x72
> 00007fff`26e128a8 00007ffe`e92f1bac WebKit!WebCore::CurlRequest::~CurlRequest+0x2c
> 00007fff`26e128b0 00007ffe`e7984bd1 WebKit!WTF::ThreadSafeRefCounted<WebCore::CurlRequest,WTF::DestructionThread::Any>::deref+0x61
> 00007fff`26e128b8 00007ffe`e92f1b07 WebKit!WebCore::CurlRequest::release+0x17
> 00007fff`26e128c0 00007ffe`ea52d717 WebKit!WebCore::CurlRequestScheduler::finalizeTransfer::<unnamed-tag>::operator+0x17
> 00007fff`26e128c8 00007ffe`ea52d6d7 WebKit!WTF::Detail::CallableWrapper<`lambda at ..\..\Source\WebCore\platform\network\curl\CurlRequestScheduler.cpp:259:26',void>::call+0x17
> 00007fff`26e128d0 00007ffe`f7925380 WTF!WTF::Function<void +0x90
> 00007fff`26e128d8 00007ffe`f7946ba4 WTF!WTF::dispatchFunctionsFromMainThread+0x164
> 00007fff`26e128e0 00007ffe`f7a37d9c WTF!WTF::ThreadingWindowWndProc+0x2c
> 00007fff`26e128e8 00007fff`2492ca66 USER32!UserCallWinProcCheckWow+0x266
> 00007fff`26e128f0 00007fff`2492c582 USER32!DispatchMessageWorker+0x1b2
> 00007fff`26e128f8 00007ffe`f7afd98d DumpRenderTreeLib!runTest+0xd3d
> 00007fff`26e12900 00007ffe`f7afc00a DumpRenderTreeLib!main+0x69a
> 00007fff`26e12908 00007ffe`f7afdebb DumpRenderTreeLib!dllLauncherEntryPoint+0x1b
> 00007fff`26e12910 00007ff7`30911423 DumpRenderTree!main+0x423
> 00007fff`26e12918 00007ff7`30914bc4 DumpRenderTree!__scrt_common_main_seh+0x10c
> 00007fff`26e12920 00007fff`24667974 KERNEL32!BaseThreadInitThunk+0x14
> 00007fff`26e12928 00007fff`26d1a271 ntdll!RtlUserThreadStart+0x21

[Fujii Hironori]

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.

[Takashi Komori]

Created attachment 374770 [details]
Patch

[Takashi Komori]

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.

[Takashi Komori]

## 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.

[Fujii Hironori]

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.

[Takashi Komori]

(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.

[Takashi Komori]

Sorry I submitted double comments wrongly..

[Fujii Hironori]

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.

[Takashi Komori]

Created attachment 374940 [details]
Patch

[Takashi Komori]

(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.

[Fujii Hironori]

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.

[Takashi Komori]

(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

[Takashi Komori]

Created attachment 374957 [details]
Patch

[Takashi Komori]

(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.

[WebKit Commit Bot]

Comment on attachment 374957 [details]
Patch

Clearing flags on attachment: 374957

Committed r247874: <https://trac.webkit.org/changeset/247874>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53664606>