cookies: lock when using CURLINFO_COOKIELIST/Curl_cookie_list #1896

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@pps83
Contributor

pps83 commented Sep 18, 2017

No description provided.

@pps83

This comment has been minimized.

Show comment Hide comment
@pps83

pps83 Sep 18, 2017

Contributor

Curl_cookie_list itself could have been locked internally. This function is used in a single place, so this should fix threading issues with cookie access

Contributor

pps83 commented Sep 18, 2017

Curl_cookie_list itself could have been locked internally. This function is used in a single place, so this should fix threading issues with cookie access

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Sep 19, 2017

Member

Thanks!

I agree with the fix, but I think I'd prefer to get the locks done within Curl_cookie_list() instead, simply because all the other cookie functions do their locking within them and getinfo.c otherwise doesn't do any locking...

Member

bagder commented Sep 19, 2017

Thanks!

I agree with the fix, but I think I'd prefer to get the locks done within Curl_cookie_list() instead, simply because all the other cookie functions do their locking within them and getinfo.c otherwise doesn't do any locking...

@jay jay added the HTTP label Sep 19, 2017

@bagder

Please do the lock in the cookie function itself, that should also fix the compiler warnings this caused.

@pps83

This comment has been minimized.

Show comment Hide comment
@pps83

pps83 Sep 19, 2017

Contributor

simply because all the other cookie functions do their locking

It's not entirely true: some cookie functions do not do locking, they would deadlock if they did (if user provided locks aren't recursive): Curl_cookie_init, Curl_cookie_add, Curl_cookie_getlist, Curl_cookie_clearall, Curl_cookie_freelist, Curl_cookie_clearsess, Curl_cookie_cleanup, Curl_flush_cookies.

For example, Curl_cookie_getlist is always locked outside.
Curl_cookie_add is locked outside in http.c and url.c, but not locked when used by Curl_cookie_init. Curl_cookie_init is locked outside only when called from Curl_cookie_loadfiles, but not locked when used from curl_easy_duphandle or from curl_share_setopt (which are other cases of data race). There are also a couple of uses from Curl_setopt, but in these functions perhaps locking is needed only when setting up shared pointer to constructed cookies. Also, looks like curl_easy_duphandle dups cookies even when they are shared, but I'm not familiar with curl code to be sure if it actually has this bug.

The reason I got this warning (that was promoted to an error) is because I use single source compilation (e.g. all curl files are included inside another .cpp file).

Contributor

pps83 commented Sep 19, 2017

simply because all the other cookie functions do their locking

It's not entirely true: some cookie functions do not do locking, they would deadlock if they did (if user provided locks aren't recursive): Curl_cookie_init, Curl_cookie_add, Curl_cookie_getlist, Curl_cookie_clearall, Curl_cookie_freelist, Curl_cookie_clearsess, Curl_cookie_cleanup, Curl_flush_cookies.

For example, Curl_cookie_getlist is always locked outside.
Curl_cookie_add is locked outside in http.c and url.c, but not locked when used by Curl_cookie_init. Curl_cookie_init is locked outside only when called from Curl_cookie_loadfiles, but not locked when used from curl_easy_duphandle or from curl_share_setopt (which are other cases of data race). There are also a couple of uses from Curl_setopt, but in these functions perhaps locking is needed only when setting up shared pointer to constructed cookies. Also, looks like curl_easy_duphandle dups cookies even when they are shared, but I'm not familiar with curl code to be sure if it actually has this bug.

The reason I got this warning (that was promoted to an error) is because I use single source compilation (e.g. all curl files are included inside another .cpp file).

@pps83 pps83 changed the title from lock cookies when using CURLINFO_COOKIELIST to cookies: lock when using CURLINFO_COOKIELIST/Curl_cookie_list Sep 19, 2017

@bagder

bagder approved these changes Sep 19, 2017

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Sep 19, 2017

Member

Personally I think I prefer doing it like this to avoid the risk of missing an unlock:

diff --git a/lib/cookie.c b/lib/cookie.c
index 1231882ed..0374f94c1 100644
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -1400,11 +1400,11 @@ static int cookie_output(struct CookieInfo *c, const char *dumphere)
     fclose(out);
 
   return 0;
 }
 
-struct curl_slist *Curl_cookie_list(struct Curl_easy *data)
+static struct curl_slist *cookie_list(struct Curl_easy *data)
 {
   struct curl_slist *list = NULL;
   struct curl_slist *beg;
   struct Cookie *c;
   char *line;
@@ -1431,10 +1431,19 @@ struct curl_slist *Curl_cookie_list(struct Curl_easy *data)
   }
 
   return list;
 }
 
+struct curl_slist *Curl_cookie_list(struct Curl_easy *data)
+{
+  struct curl_slist *list;
+  Curl_share_lock(data, CURL_LOCK_DATA_COOKIE, CURL_LOCK_ACCESS_SINGLE);
+  list = cookie_list(data);
+  Curl_share_unlock(data, CURL_LOCK_DATA_COOKIE);
+  return list;
+}
+
 void Curl_flush_cookies(struct Curl_easy *data, int cleanup)
 {
   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
Member

bagder commented Sep 19, 2017

Personally I think I prefer doing it like this to avoid the risk of missing an unlock:

diff --git a/lib/cookie.c b/lib/cookie.c
index 1231882ed..0374f94c1 100644
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -1400,11 +1400,11 @@ static int cookie_output(struct CookieInfo *c, const char *dumphere)
     fclose(out);
 
   return 0;
 }
 
-struct curl_slist *Curl_cookie_list(struct Curl_easy *data)
+static struct curl_slist *cookie_list(struct Curl_easy *data)
 {
   struct curl_slist *list = NULL;
   struct curl_slist *beg;
   struct Cookie *c;
   char *line;
@@ -1431,10 +1431,19 @@ struct curl_slist *Curl_cookie_list(struct Curl_easy *data)
   }
 
   return list;
 }
 
+struct curl_slist *Curl_cookie_list(struct Curl_easy *data)
+{
+  struct curl_slist *list;
+  Curl_share_lock(data, CURL_LOCK_DATA_COOKIE, CURL_LOCK_ACCESS_SINGLE);
+  list = cookie_list(data);
+  Curl_share_unlock(data, CURL_LOCK_DATA_COOKIE);
+  return list;
+}
+
 void Curl_flush_cookies(struct Curl_easy *data, int cleanup)
 {
   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
@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Sep 19, 2017

Member

I've fixed test 506 locally to work with this change.

Member

bagder commented Sep 19, 2017

I've fixed test 506 locally to work with this change.

@bagder bagder closed this in 5fe8558 Sep 20, 2017

@bagder

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Sep 20, 2017

Member

Thanks!

Member

bagder commented Sep 20, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment