2014/07/15 3:39、Daniel Stenberg <dan...@haxx.se> のメール：
> On Sun, 13 Jul 2014, yousuke.kim...@gmail.com wrote:
>
>> This issue was reported in http://curl.haxx.se/mail/lib-2014-02/0184.html.
>> Unfortunately the reporter didn't suggest his patch.
>>
>> This issue is caused by locking CURL_LOCK_DATA_COOKIE with specified cookie
>> files. In this case, CURL_LOCK_DATA_COOKIE is locked before executing
>> "FLUSH", and then Curl_cookie_loadfiles locks CURL_LOCK_DATA_COOKIE. That is
>> deadlock.
>
> Thanks for the patch. My reading of it says it is incorrect though:
>
> - for FLUSH you (still) take the lock before Curl_flush_cookies() is called
> and then within Curl_flush_cookies() it locks the same mutex again.
>
> Shouldn't the patch then just have those lines removed, like this?
To understand this issue more deeply, I'd like to explain about the deadlock
issue.
The deadlock sequence is as follows:
1) curl share object is being used.
2) To load cookie files, CURLOPT_COOKIEFILE is set to specify the cookie by
using curl_easy_setopt()
3) To flush cookie, CURLOPT_COOKIELIST and "FLUSH" is set by using
curl_easy_setopt().
Then, in Curl_setopt() : url.c
Curl_share_lock(data, CURL_LOCK_DATA_COOKIE, CURL_LOCK_ACCESS_SINGLE);
is invoked, and CURL_LOCK_DATA_COOKIE is locked.
Next, "FLUSH", Curl_flush_cookies() is invoked.
The function is written as follows:
void Curl_flush_cookies(struct SessionHandle *data, int cleanup) : cookie.c
{
if(data->set.str[STRING_COOKIEJAR]) {
if(data->change.cookielist) {
/* If there is a list of cookie files to read, do it first so that
we have all the told files read before we write the new jar.
Curl_cookie_loadfiles() LOCKS and UNLOCKS the share itself! */
Curl_cookie_loadfiles(data);
}
....
As CURLOPT_COOKIEFILE is specified, Curl_cookie_loadfiles() is invoked.
Curl_cookie_loadfiles() locks CURL_LOCK_DATA_COOKIE by itself.
Then CURL_LOCK_DATA_COOKIE is locked twice in this situation.
void Curl_cookie_loadfiles(struct SessionHandle *data)
{
struct curl_slist *list = data->change.cookielist;
if(list) {
Curl_share_lock(data, CURL_LOCK_DATA_COOKIE, CURL_LOCK_ACCESS_SINGLE); //
<--- The cookie mutex is locked twice here.
while(list) {
data->cookies = Curl_cookie_init(data,
list->data,
data->cookies,
data->set.cookiesession);
list = list->next;
}
curl_slist_free_all(data->change.cookielist); /* clean up list */
data->change.cookielist = NULL; /* don't do this again! */
Curl_share_unlock(data, CURL_LOCK_DATA_COOKIE);
}
}
I guess there are other patches to fix this issue. In my patch,
only url.c will be modified. If the mutex is created with recursive
option like PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP, my patch is not necessary.
Does libcurl expect that the mutex for CURL_LOCK_DATA_COOKIE is created
as recursive mutex?
I think libcurl should consider where CURL_LOCK_DATA_COOKIE is locked
to avoid like this issue.
Thanks,
Yousuke
>
> From fcf80e75c39a54b8be53d7040fe8970c3b13a542 Mon Sep 17 00:00:00 2001
> From: Daniel Stenberg <dan...@haxx.se>
> Date: Mon, 14 Jul 2014 20:38:18 +0200
> Subject: [PATCH] cookie: avoid mutex deadlock
>
> ... by removing the extra mutex locks around th call to
> Curl_flush_cookies() which takes care of the locking itself already.
> ---
> lib/url.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lib/url.c b/lib/url.c
> index 87ebe00..1d05975 100644
> --- a/lib/url.c
> +++ b/lib/url.c
> @@ -1167,22 +1167,24 @@ CURLcode Curl_setopt(struct SessionHandle *data,
> CURLoption option,
> argptr = va_arg(param, char *);
>
> if(argptr == NULL)
> break;
>
> - Curl_share_lock(data, CURL_LOCK_DATA_COOKIE, CURL_LOCK_ACCESS_SINGLE);
> -
> if(Curl_raw_equal(argptr, "ALL")) {
> /* clear all cookies */
> + Curl_share_lock(data, CURL_LOCK_DATA_COOKIE, CURL_LOCK_ACCESS_SINGLE);
> Curl_cookie_clearall(data->cookies);
> + Curl_share_unlock(data, CURL_LOCK_DATA_COOKIE);
> }
> else if(Curl_raw_equal(argptr, "SESS")) {
> /* clear session cookies */
> + Curl_share_lock(data, CURL_LOCK_DATA_COOKIE, CURL_LOCK_ACCESS_SINGLE);
> Curl_cookie_clearsess(data->cookies);
> + Curl_share_unlock(data, CURL_LOCK_DATA_COOKIE);
> }
> else if(Curl_raw_equal(argptr, "FLUSH")) {
> - /* flush cookies to file */
> + /* flush cookies to file, takes care of the locking */
> Curl_flush_cookies(data, 0);
> }
> else {
> if(!data->cookies)
> /* if cookie engine was not running, activate it */
> @@ -1191,23 +1193,24 @@ CURLcode Curl_setopt(struct SessionHandle *data,
> CURLoption option,
> argptr = strdup(argptr);
> if(!argptr) {
> result = CURLE_OUT_OF_MEMORY;
> }
> else {
> + Curl_share_lock(data, CURL_LOCK_DATA_COOKIE,
> CURL_LOCK_ACCESS_SINGLE);
>
> if(checkprefix("Set-Cookie:", argptr))
> /* HTTP Header format line */
> Curl_cookie_add(data, data->cookies, TRUE, argptr + 11, NULL, NULL);
>
> else
> /* Netscape format line */
> Curl_cookie_add(data, data->cookies, FALSE, argptr, NULL, NULL);
>
> + Curl_share_unlock(data, CURL_LOCK_DATA_COOKIE);
> free(argptr);
> }
> }
> - Curl_share_unlock(data, CURL_LOCK_DATA_COOKIE);
>
> break;
> #endif /* CURL_DISABLE_COOKIES */
>
> case CURLOPT_HTTPGET:
> --
> 2.0.1
>
>
> --
>
> / daniel.haxx.se
> -------------------------------------------------------------------
> List admin: http://cool.haxx.se/list/listinfo/curl-library
> Etiquette: http://curl.haxx.se/mail/etiquette.html
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
