cURL / Mailing Lists / curl-library / Single Mail

curl-library

Re: Fix deadlock patch in "FLUSH" cookie and more

From: Yousuke Kimoto <yousuke.kimoto_at_gmail.com>
Date: Tue, 15 Jul 2014 17:52:14 +0900

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?

Thank you for your patch.
I misunderstood your comment, I'm sorry.

Your patch is right, Curl_share_lock() and Curl_share_unlock() are not
necessary for Curl_flush_cookies().

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