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
Conversation
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 |
Thanks! I agree with the fix, but I think I'd prefer to get the locks done within |
There was a problem hiding this 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.
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): For example, 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). |
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 |
I've fixed test 506 locally to work with this change. |
Thanks! |
No description provided.