Bug 191650 – [Curl] Fix Curl Request Scheduler not to release wrong Curl handle when request is cancelled.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

- finalizeTransfer  is called more than once for some situation.
- CurlRequest is not removed on some situation.

The detail will follow.

[Takashi Komori]

CurlRequestScheduler uses CURL pointer as job identifier, but libcurl sometimes re-use released pointer.
Therefore depending on timing, finalizeTransfer might be called more than once.

1)One CurlRequest (request A) is canceled and releases CURL* (pointer P).
2)And another CurlRequest is created (request B) and gets re-used CURL* (pointer P) same to former .
3)CurlRequestScheduler does finalize task for request A using pointer P.
4)When request B is closed, CurlRequestScheduler does finalize task using same pointer P and issue occurs.

Scheduler has to control tasks by CurlRequest* instead of CURL*.

[Takashi Komori]

Created attachment 367302 [details]
Patch

[Fujii Hironori]

(In reply to Takashi Komori from comment #1)
> 3)CurlRequestScheduler does finalize task for request A using pointer P.
> 4)When request B is closed, CurlRequestScheduler does finalize task using
> same pointer P and issue occurs.

How can this happen? IUUC, the sequence looks:

3) CurlRequestScheduler::finalizeTransfer post a task with the handle P
4) worker thread finilizes the handle P
5) CurlRequest::finalizeTransfer release the handle P by curl_easy_cleanup

[Fujii Hironori]

Comment on attachment 367302 [details]
Patch

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

I like overall this change.

> Source/WebCore/ChangeLog:3
> +        [Curl] Bug fix on Curl Request Scheduler.

Please be more informative summary.
What is bug, or what is your change.

> LayoutTests/ChangeLog:9
> +        * http/tests/misc/repeat-open-cancel.html: Added.

You added a new test taking more than 40 seconds to finish in a common test directory. I don't know the policy to add a new such test. I'd like to hear network folks' opinion.

[Fujii Hironori]

Comment on attachment 367302 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlRequestScheduler.h:71
> +    HashSet<CurlRequestSchedulerClient*> m_activeJobs;

You can remove  m_activeJobs By using m_clientMaps with client->handle().

[Takashi Komori]

(In reply to Fujii Hironori from comment #3)
> (In reply to Takashi Komori from comment #1)
> > 3)CurlRequestScheduler does finalize task for request A using pointer P.
> > 4)When request B is closed, CurlRequestScheduler does finalize task using
> > same pointer P and issue occurs.
> 
> How can this happen? IUUC, the sequence looks:
> 
> 3) CurlRequestScheduler::finalizeTransfer post a task with the handle P
> 4) worker thread finilizes the handle P
> 5) CurlRequest::finalizeTransfer release the handle P by curl_easy_cleanup

Correctly is as follows

1) First CurlRequest(request1) is canceled. request1 has handle CURL* P.
2) cancelTransfer task for request1 is added to task queue. (TASK1)
3) Second CurlRequest(request2) is created and startTransfer task is added. (TASK2)
4) didReceiveHeader or didReceiveData callback function is invoked for request1.
5) request1 was canceled (m_cancelled is true), CurlRequest::didReceiveData/CurlRequest::didReceiveHeader return 0.
6) libcurl treats returned 0 as an error.
7) Worker thread gets CURL_WRITE_ERROR from curl_multi_info_read, and add completeTransfer task for handle P (TASK3)
8) (TASK1) is executed, handle P is released.
9) (TASK2) is executed, new handle is created P' for request2 but the address of P' is same as P released just before. P'==P
10) (TASK3) is executed, handle P is released.
11) Because P'==P request2's CURL handle is released before use.

[Fujii Hironori]

I got it. Thank you for the explanation.
m_cancelled is accessed from both threads without locking a mutex.

[Takashi Komori]

Created attachment 367791 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #4)
> Comment on attachment 367302 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367302&action=review
> 
> I like overall this change.
> 
> > Source/WebCore/ChangeLog:3
> > +        [Curl] Bug fix on Curl Request Scheduler.
> 
> Please be more informative summary.
> What is bug, or what is your change.

Basuke Suzuki changed. Thanks! Basuke!

[Takashi Komori]

(In reply to Fujii Hironori from comment #5)
> Comment on attachment 367302 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367302&action=review
> 
> > Source/WebCore/platform/network/curl/CurlRequestScheduler.h:71
> > +    HashSet<CurlRequestSchedulerClient*> m_activeJobs;
> 
> You can remove  m_activeJobs By using m_clientMaps with client->handle().

To control thread stopping CurlRequestScheduler::stopThreadIfNoMoreJobRunning() is using m_activeJobs.
Because Client with null CURL handle can exist (when setupTransfer failed), using m_clientMap for this purpose seems to be difficult.

[Takashi Komori]

(In reply to Fujii Hironori from comment #7)
> I got it. Thank you for the explanation.
> m_cancelled is accessed from both threads without locking a mutex.

FIXED.

[Fujii Hironori]

(In reply to Takashi Komori from comment #11)
> (In reply to Fujii Hironori from comment #7)
> > I got it. Thank you for the explanation.
> > m_cancelled is accessed from both threads without locking a mutex.
> 
> FIXED.

Hmm, it looks badly designed to me.
What about m_curlHandle? 
m_curlHandle is written in CurlRequest::setupTransfer in curl thread,
but accessed in main thread in CurlRequest::cancel via isCompletedOrCancelled.

[Fujii Hironori]

Comment on attachment 367791 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:99
> +        LockHolder locker(m_mutex);

I think you should use holdLock.
auto locker = holdLock(lock);
https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/Locker.h?rev=244495#L121

[Takashi Komori]

Created attachment 367937 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #12)
> (In reply to Takashi Komori from comment #11)
> > (In reply to Fujii Hironori from comment #7)
> > > I got it. Thank you for the explanation.
> > > m_cancelled is accessed from both threads without locking a mutex.
> > 
> > FIXED.
> 
> Hmm, it looks badly designed to me.
> What about m_curlHandle? 
> m_curlHandle is written in CurlRequest::setupTransfer in curl thread,
> but accessed in main thread in CurlRequest::cancel via
> isCompletedOrCancelled.

Before patch CurlRequest::cancel was using isCompletedOrCancelled but with new patch it isn't using.

[Takashi Komori]

(In reply to Fujii Hironori from comment #13)
> Comment on attachment 367791 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367791&action=review
> 
> > Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:99
> > +        LockHolder locker(m_mutex);
> 
> I think you should use holdLock.
> auto locker = holdLock(lock);
> https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/Locker.
> h?rev=244495#L121

Applied holdLock to new added m_cancelMutex.

[Fujii Hironori]

(In reply to Takashi Komori from comment #15)
> Before patch CurlRequest::cancel was using isCompletedOrCancelled but with
> new patch it isn't using.

Oh, you are right?

How about other call sites of isCompletedOrCancelled?
For example, CurlRequest::pausedStatusChanged?

How about other member variables?
For example, m_formDataStream?

[Takashi Komori]

Created attachment 368019 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #17)
> (In reply to Takashi Komori from comment #15)
> > Before patch CurlRequest::cancel was using isCompletedOrCancelled but with
> > new patch it isn't using.
> 
> Oh, you are right?
> 
> How about other call sites of isCompletedOrCancelled?
> For example, CurlRequest::pausedStatusChanged?

Introduced m_statusMutex. Now m_curlHandle is not accessed from main thread directly.

> How about other member variables?
> For example, m_formDataStream?

About other variables and functions, we have a plan to re-design scheduler to split tasks for main thread and worker thread completely.
In this ticket, we should focus on fixing curl scheduler's incorrect behavior.

[Fujii Hironori]

(In reply to Takashi Komori from comment #19)
> About other variables and functions, we have a plan to re-design scheduler
> to split tasks for main thread and worker thread completely.
> In this ticket, we should focus on fixing curl scheduler's incorrect
> behavior.

Sounds a good idea.

[Fujii Hironori]

Comment on attachment 368019 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:104
>      m_runThread = false;

In the original code, m_runThread was guarded by m_mutex.
However, m_runThread is not guarded by m_mutex consistently. It makes no sence to fix only this one.

[Takashi Komori]

Created attachment 368298 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #21)
> Comment on attachment 368019 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368019&action=review
> 
> > Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:104
> >      m_runThread = false;
> 
> In the original code, m_runThread was guarded by m_mutex.
> However, m_runThread is not guarded by m_mutex consistently. It makes no
> sence to fix only this one.

Fixed mutex guard for m_runThread.

[EWS Watchlist]

Attachment 368298 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:152:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Takashi Komori]

(In reply to Build Bot from comment #24)
> Attachment 368298 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/platform/network/curl/CurlRequestScheduler.cpp:152: 
> This { should be at the end of the previous line  [whitespace/braces] [4]
> Total errors found: 1 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Style checker's behavior seems to be wrong.
Block in while loop must be valid.

[WebKit Commit Bot]

Comment on attachment 368298 [details]
Patch

Clearing flags on attachment: 368298

Committed r244684: <https://trac.webkit.org/changeset/244684>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50237955>