Bug 191650 - [Curl] Fix Curl Request Scheduler not to release wrong Curl handle when request is cancelled.
Summary: [Curl] Fix Curl Request Scheduler not to release wrong Curl handle when reque...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 197307
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-14 12:53 PST by Basuke Suzuki
Modified: 2019-04-26 02:54 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.93 KB, patch)
2019-04-12 01:05 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (11.95 KB, patch)
2019-04-19 01:15 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (11.96 KB, patch)
2019-04-22 06:45 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (13.26 KB, patch)
2019-04-23 01:32 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (15.40 KB, patch)
2019-04-25 20:00 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2018-11-14 12:53:19 PST
- finalizeTransfer  is called more than once for some situation.
- CurlRequest is not removed on some situation.

The detail will follow.
Comment 1 Takashi Komori 2019-04-12 01:04:28 PDT
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*.
Comment 2 Takashi Komori 2019-04-12 01:05:45 PDT
Created attachment 367302 [details]
Patch
Comment 3 Fujii Hironori 2019-04-12 04:28:43 PDT
(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
Comment 4 Fujii Hironori 2019-04-12 04:45:34 PDT
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.
Comment 5 Fujii Hironori 2019-04-12 07:12:55 PDT
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().
Comment 6 Takashi Komori 2019-04-16 01:08:08 PDT
(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.
Comment 7 Fujii Hironori 2019-04-16 02:14:35 PDT
I got it. Thank you for the explanation.
m_cancelled is accessed from both threads without locking a mutex.
Comment 8 Takashi Komori 2019-04-19 01:15:45 PDT
Created attachment 367791 [details]
Patch
Comment 9 Takashi Komori 2019-04-19 01:17:41 PDT
(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!
Comment 10 Takashi Komori 2019-04-19 01:18:44 PDT
(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.
Comment 11 Takashi Komori 2019-04-19 01:19:03 PDT
(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.
Comment 12 Fujii Hironori 2019-04-21 19:50:18 PDT
(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.
Comment 13 Fujii Hironori 2019-04-21 19:53:00 PDT
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
Comment 14 Takashi Komori 2019-04-22 06:45:34 PDT
Created attachment 367937 [details]
Patch
Comment 15 Takashi Komori 2019-04-22 06:47:47 PDT
(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.
Comment 16 Takashi Komori 2019-04-22 06:50:04 PDT
(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.
Comment 17 Fujii Hironori 2019-04-22 07:05:03 PDT
(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?
Comment 18 Takashi Komori 2019-04-23 01:32:04 PDT
Created attachment 368019 [details]
Patch
Comment 19 Takashi Komori 2019-04-23 01:33:11 PDT
(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.
Comment 20 Fujii Hironori 2019-04-24 22:18:10 PDT
(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.
Comment 21 Fujii Hironori 2019-04-24 22:23:09 PDT
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.
Comment 22 Takashi Komori 2019-04-25 20:00:53 PDT
Created attachment 368298 [details]
Patch
Comment 23 Takashi Komori 2019-04-25 20:01:29 PDT
(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.
Comment 24 EWS Watchlist 2019-04-25 20:03:20 PDT
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.
Comment 25 Takashi Komori 2019-04-25 22:54:10 PDT
(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.
Comment 26 WebKit Commit Bot 2019-04-26 02:52:55 PDT
Comment on attachment 368298 [details]
Patch

Clearing flags on attachment: 368298

Committed r244684: <https://trac.webkit.org/changeset/244684>
Comment 27 WebKit Commit Bot 2019-04-26 02:52:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Radar WebKit Bug Importer 2019-04-26 02:54:38 PDT
<rdar://problem/50237955>