2014/07/15 3:39、Daniel Stenberg <daniel_at_haxx.se> のメール：

> On Sun, 13 Jul 2014, yousuke.kimoto_at_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 <daniel_at_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
Received on 2014-07-15