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

[Daniel Stenberg]

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?

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