Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cookies: lock when using CURLINFO_COOKIELIST/Curl_cookie_list #1896

Closed
wants to merge 1 commit into from

Conversation

pps83
Copy link
Contributor

@pps83 pps83 commented Sep 18, 2017

No description provided.

@pps83
Copy link
Contributor Author

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
Copy link
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
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pps83
Copy link
Contributor Author

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 lock cookies when using CURLINFO_COOKIELIST cookies: lock when using CURLINFO_COOKIELIST/Curl_cookie_list Sep 19, 2017
@bagder
Copy link
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
Copy link
Member

bagder commented Sep 19, 2017

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

@bagder
Copy link
Member

bagder commented Sep 20, 2017

Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants