cookies: Improved OOM handling in cookies

This fixes the test 506 torture test. The internal cookie API really
ought to be improved to separate cookie parsing errors (which may be
ignored) with OOM errors (which should be fatal).

diff --git a/lib/cookie.c b/lib/cookie.c
index 17c18e2df980382fbceab9a3ee4827b77f67653c..0b9c8d337c8b8c53f795bf679103ca8a455d47f3 100644 (file)
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -262,7 +262,8 @@ static char *sanitize_cookie_path(const char *cookie_path)
 
 /*
  * Load cookies from all given cookie files (CURLOPT_COOKIEFILE).
- * NOTE: failures are ignored
+ *
+ * NOTE: OOM or cookie parsing failures are ignored.
  */
 void Curl_cookie_loadfiles(struct SessionHandle *data)
 {
@@ -270,10 +271,17 @@ void Curl_cookie_loadfiles(struct SessionHandle *data)
   if(list) {
     Curl_share_lock(data, CURL_LOCK_DATA_COOKIE, CURL_LOCK_ACCESS_SINGLE);
     while(list) {
-      data->cookies = Curl_cookie_init(data,
-                                       list->data,
-                                       data->cookies,
-                                       data->set.cookiesession);
+      struct CookieInfo *newcookies = Curl_cookie_init(data,
+                                        list->data,
+                                        data->cookies,
+                                        data->set.cookiesession);
+      if(!newcookies)
+        /* Failure may be due to OOM or a bad cookie; both are ignored
+         * but only the first should be
+         */
+        infof(data, "ignoring failed cookie_init for %s\n", list->data);
+      else
+        data->cookies = newcookies;
       list = list->next;
     }
     curl_slist_free_all(data->change.cookielist); /* clean up list */
@@ -355,6 +363,8 @@ static bool isip(const char *domain)
  * Be aware that sometimes we get an IP-only host name, and that might also be
  * a numerical IPv6 address.
  *
+ * Returns NULL on out of memory or invalid cookie. This is suboptimal,
+ * as they should be treated separately.
  ***************************************************************************/
 
 struct Cookie *
@@ -886,6 +896,7 @@ Curl_cookie_add(struct SessionHandle *data,
  *
  * If 'newsession' is TRUE, discard all "session cookies" on read from file.
  *
+ * Returns NULL on out of memory. Invalid cookies are ignored.
  ****************************************************************************/
 struct CookieInfo *Curl_cookie_init(struct SessionHandle *data,
                                     const char *file,
@@ -893,8 +904,9 @@ struct CookieInfo *Curl_cookie_init(struct SessionHandle *data,
                                     bool newsession)
 {
   struct CookieInfo *c;
-  FILE *fp;
+  FILE *fp = NULL;
   bool fromfile=TRUE;
+  char *line = NULL;
 
   if(NULL == inc) {
     /* we didn't get a struct, create one */
@@ -902,6 +914,8 @@ struct CookieInfo *Curl_cookie_init(struct SessionHandle *data,
     if(!c)
       return NULL; /* failed to get memory */
     c->filename = strdup(file?file:"none"); /* copy the name just in case */
+    if(!c->filename)
+      goto fail; /* failed to get memory */
   }
   else {
     /* we got an already existing one, use that */
@@ -926,25 +940,26 @@ struct CookieInfo *Curl_cookie_init(struct SessionHandle *data,
     char *lineptr;
     bool headerline;
 
-    char *line = malloc(MAX_COOKIE_LINE);
-    if(line) {
-      while(fgets(line, MAX_COOKIE_LINE, fp)) {
-        if(checkprefix("Set-Cookie:", line)) {
-          /* This is a cookie line, get it! */
-          lineptr=&line[11];
-          headerline=TRUE;
-        }
-        else {
-          lineptr=line;
-          headerline=FALSE;
-        }
-        while(*lineptr && ISBLANK(*lineptr))
-          lineptr++;
-
-        Curl_cookie_add(data, c, headerline, lineptr, NULL, NULL);
+    line = malloc(MAX_COOKIE_LINE);
+    if(!line)
+      goto fail;
+    while(fgets(line, MAX_COOKIE_LINE, fp)) {
+      if(checkprefix("Set-Cookie:", line)) {
+        /* This is a cookie line, get it! */
+        lineptr=&line[11];
+        headerline=TRUE;
       }
-      free(line); /* free the line buffer */
+      else {
+        lineptr=line;
+        headerline=FALSE;
+      }
+      while(*lineptr && ISBLANK(*lineptr))
+        lineptr++;
+
+      Curl_cookie_add(data, c, headerline, lineptr, NULL, NULL);
     }
+    free(line); /* free the line buffer */
+
     if(fromfile)
       fclose(fp);
   }
@@ -952,6 +967,16 @@ struct CookieInfo *Curl_cookie_init(struct SessionHandle *data,
   c->running = TRUE;          /* now, we're running */
 
   return c;
+
+fail:
+  Curl_safefree(line);
+  if(!inc)
+    /* Only clean up if we allocated it here, as the original could still be in
+     * use by a share handle */
+    Curl_cookie_cleanup(c);
+  if(fromfile && fp)
+    fclose(fp);
+  return NULL; /* out of memory */
 }
 
 /* sort this so that the longest path gets before the shorter path */

diff --git a/lib/url.c b/lib/url.c
index 9996dd52767735d74581eb06098a2bcbd73e2f8b..dd3118d3e2cd96b31b44c896543b70234ce2b0cb 100644 (file)
--- a/lib/url.c
+++ b/lib/url.c
@@ -1164,6 +1164,8 @@ CURLcode Curl_setopt(struct SessionHandle *data, CURLoption option,
     /*
      * Set cookie file name to dump all cookies to when we're done.
      */
+  {
+    struct CookieInfo *newcookies;
     result = setstropt(&data->set.str[STRING_COOKIEJAR],
                        va_arg(param, char *));
 
@@ -1171,8 +1173,12 @@ CURLcode Curl_setopt(struct SessionHandle *data, CURLoption option,
      * Activate the cookie parser. This may or may not already
      * have been made.
      */
-    data->cookies = Curl_cookie_init(data, NULL, data->cookies,
-                                     data->set.cookiesession);
+    newcookies = Curl_cookie_init(data, NULL, data->cookies,
+                                  data->set.cookiesession);
+    if(!newcookies)
+      result = CURLE_OUT_OF_MEMORY;
+    data->cookies = newcookies;
+  }
     break;
 
   case CURLOPT_COOKIESESSION:
@@ -1227,8 +1233,9 @@ CURLcode Curl_setopt(struct SessionHandle *data, CURLoption option,
         data->cookies = Curl_cookie_init(data, NULL, NULL, TRUE);
 
       argptr = strdup(argptr);
-      if(!argptr) {
+      if(!argptr || !data->cookies) {
         result = CURLE_OUT_OF_MEMORY;
+        Curl_safefree(argptr);
       }
       else {
         Curl_share_lock(data, CURL_LOCK_DATA_COOKIE, CURL_LOCK_ACCESS_SINGLE);

diff --git a/tests/libtest/lib506.c b/tests/libtest/lib506.c
index 2fbe38fe92bbc1a277b7791aac8031bc990d50b7..4dad0d98a012c03b6ccb67445c66b2b98ec05301 100644 (file)
--- a/tests/libtest/lib506.c
+++ b/tests/libtest/lib506.c
@@ -328,17 +328,15 @@ int test(char *URL)
   if ( code != CURLE_OK )
   {
     fprintf(stderr, "curl_easy_getinfo() failed\n");
-    curl_share_cleanup(share);
-    curl_global_cleanup();
-    return TEST_ERR_MAJOR_BAD;
+    res = TEST_ERR_MAJOR_BAD;
+    goto test_cleanup;
   }
   printf("loaded cookies:\n");
   if ( !cookies )
   {
     fprintf(stderr, "  reloading cookies from '%s' failed\n", JAR);
-    curl_share_cleanup(share);
-    curl_global_cleanup();
-    return TEST_ERR_MAJOR_BAD;
+    res = TEST_ERR_MAJOR_BAD;
+    goto test_cleanup;
   }
   printf("-----------------\n");
   next_cookie = cookies;