multi: fix memory leak when stopped during name resolve

When the application just started the transfer and then stops it while
the name resolve in the background thread hasn't completed, we need to
wait for the resolve to complete and then cleanup data accordingly.

Enabled test 1553 again and added test 1590 to also check when the host
name resolves successfully.

Detected by OSS-fuzz.
Closes #1968

diff --git a/lib/asyn-ares.c b/lib/asyn-ares.c
index aa581a49a0a2dbf4805cdcc2772068b99c3138e2..00fe1adb580cdd51a90cdbc30c50a4260cff6bfd 100644 (file)
--- a/lib/asyn-ares.c
+++ b/lib/asyn-ares.c
@@ -5,7 +5,7 @@
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 1998 - 2017, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
@@ -312,22 +312,25 @@ CURLcode Curl_resolver_is_resolved(struct connectdata *conn,
     conn->async.os_specific;
   CURLcode result = CURLE_OK;
 
-  *dns = NULL;
+  if(dns)
+    *dns = NULL;
 
   waitperform(conn, 0);
 
   if(res && !res->num_pending) {
-    (void)Curl_addrinfo_callback(conn, res->last_status, res->temp_ai);
-    /* temp_ai ownership is moved to the connection, so we need not free-up
-       them */
-    res->temp_ai = NULL;
+    if(dns) {
+      (void)Curl_addrinfo_callback(conn, res->last_status, res->temp_ai);
+      /* temp_ai ownership is moved to the connection, so we need not free-up
+         them */
+      res->temp_ai = NULL;
+    }
     if(!conn->async.dns) {
       failf(data, "Could not resolve: %s (%s)",
             conn->async.hostname, ares_strerror(conn->async.status));
       result = conn->bits.proxy?CURLE_COULDNT_RESOLVE_PROXY:
         CURLE_COULDNT_RESOLVE_HOST;
     }
-    else
+    else if(dns)
       *dns = conn->async.dns;
 
     destroy_async_data(&conn->async);
@@ -390,7 +393,7 @@ CURLcode Curl_resolver_wait_resolv(struct connectdata *conn,
       timeout_ms = 1000;
 
     waitperform(conn, timeout_ms);
-    result = Curl_resolver_is_resolved(conn, &temp_entry);
+    result = Curl_resolver_is_resolved(conn, entry?&temp_entry:NULL);
 
     if(result || conn->async.done)
       break;

diff --git a/lib/asyn-thread.c b/lib/asyn-thread.c
index b11fab24684a55ceeff564a27140be0a996efc02..894ca459b54718e27793000d85ab5496ac62bd54 100644 (file)
--- a/lib/asyn-thread.c
+++ b/lib/asyn-thread.c
@@ -481,8 +481,10 @@ CURLcode Curl_resolver_wait_resolv(struct connectdata *conn,
   DEBUGASSERT(conn && td);
 
   /* wait for the thread to resolve the name */
-  if(Curl_thread_join(&td->thread_hnd))
-    result = getaddrinfo_complete(conn);
+  if(Curl_thread_join(&td->thread_hnd)) {
+    if(entry)
+      result = getaddrinfo_complete(conn);
+  }
   else
     DEBUGASSERT(0);
 

diff --git a/lib/multi.c b/lib/multi.c
index 9cad600e2b6963240bfcf5cdd22ed31e26b12e1b..e30737dd27a79066ba9e296093830eee60c266b8 100644 (file)
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -69,8 +69,8 @@
 #define GOOD_MULTI_HANDLE(x) \
   ((x) && (x)->type == CURL_MULTI_HANDLE)
 
-static void singlesocket(struct Curl_multi *multi,
-                         struct Curl_easy *data);
+static CURLMcode singlesocket(struct Curl_multi *multi,
+                              struct Curl_easy *data);
 static int update_timer(struct Curl_multi *multi);
 
 static CURLMcode add_next_timeout(struct curltime now,
@@ -515,6 +515,11 @@ static CURLcode multi_done(struct connectdata **connp,
     /* Stop if multi_done() has already been called */
     return CURLE_OK;
 
+  if(data->mstate == CURLM_STATE_WAITRESOLVE) {
+    /* still waiting for the resolve to complete */
+    (void)Curl_resolver_wait_resolv(conn, NULL);
+  }
+
   Curl_getoff_all_pipelines(data, conn);
 
   /* Cleanup possible redirect junk */
@@ -2304,8 +2309,8 @@ CURLMsg *curl_multi_info_read(struct Curl_multi *multi, int *msgs_in_queue)
  * and if we have a different state in any of those sockets from last time we
  * call the callback accordingly.
  */
-static void singlesocket(struct Curl_multi *multi,
-                         struct Curl_easy *data)
+static CURLMcode singlesocket(struct Curl_multi *multi,
+                              struct Curl_easy *data)
 {
   curl_socket_t socks[MAX_SOCKSPEREASYHANDLE];
   int i;
@@ -2352,7 +2357,7 @@ static void singlesocket(struct Curl_multi *multi,
       entry = sh_addentry(&multi->sockhash, s, data);
       if(!entry)
         /* fatal */
-        return;
+        return CURLM_OUT_OF_MEMORY;
     }
 
     /* we know (entry != NULL) at this point, see the logic above */
@@ -2440,6 +2445,7 @@ static void singlesocket(struct Curl_multi *multi,
 
   memcpy(data->sockets, socks, num*sizeof(curl_socket_t));
   data->numsocks = num;
+  return CURLM_OK;
 }
 
 void Curl_updatesocket(struct Curl_easy *data)
@@ -2553,8 +2559,8 @@ static CURLMcode multi_socket(struct Curl_multi *multi,
        and callbacks */
     if(result != CURLM_BAD_HANDLE) {
       data = multi->easyp;
-      while(data) {
-        singlesocket(multi, data);
+      while(data && !result) {
+        result = singlesocket(multi, data);
         data = data->next;
       }
     }
@@ -2608,10 +2614,13 @@ static CURLMcode multi_socket(struct Curl_multi *multi,
         /* clear the bitmask only if not locked */
         data->easy_conn->cselect_bits = 0;
 
-      if(CURLM_OK >= result)
+      if(CURLM_OK >= result) {
         /* get the socket(s) and check if the state has been changed since
            last */
-        singlesocket(multi, data);
+        result = singlesocket(multi, data);
+        if(result)
+          return result;
+      }
 
       /* Now we fall-through and do the timer-based stuff, since we don't want
          to force the user to have to deal with timeouts as long as at least
@@ -2645,10 +2654,13 @@ static CURLMcode multi_socket(struct Curl_multi *multi,
       result = multi_runsingle(multi, now, data);
       sigpipe_restore(&pipe_st);
 
-      if(CURLM_OK >= result)
+      if(CURLM_OK >= result) {
         /* get the socket(s) and check if the state has been changed since
            last */
-        singlesocket(multi, data);
+        result = singlesocket(multi, data);
+        if(result)
+          return result;
+      }
     }
 
     /* Check if there's one (more) expired timer to deal with! This function

diff --git a/tests/data/DISABLED b/tests/data/DISABLED
index fcbf0ffc4d47ce9415429873a10e0809d2c6655b..11d54b4637777f411c2d771d7edc100c0b77b140 100644 (file)
--- a/tests/data/DISABLED
+++ b/tests/data/DISABLED
@@ -18,5 +18,3 @@
 1510
 # Pipelining test that is causing false positives a little too often
 1903
-# causes memory leaks for now:
-1553

diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc
index c7dcaaa274222f16acf9343a453273b69eac2141..9d0781f0e4703232c4db05590f7d463aeead8a0b 100644 (file)
--- a/tests/data/Makefile.inc
+++ b/tests/data/Makefile.inc
@@ -176,6 +176,8 @@ test1525 test1526 test1527 test1528 test1529 test1530 test1531 test1532 \
 test1533 test1534 test1535 test1536 test1537 test1538 \
 test1540 \
 test1550 test1551 test1552 test1553 test1554 test1555 test1556 \
+\
+test1590 \
 test1600 test1601 test1602 test1603 test1604 test1605 test1606 test1607 \
 test1608 test1609 \
 \

diff --git a/tests/data/test1553 b/tests/data/test1553
index c2fb9cc3212ecb760c0cbe3ed800f18666470397..5e793b391964258eba462a183b1957e3a949f117 100644 (file)
--- a/tests/data/test1553
+++ b/tests/data/test1553
@@ -38,7 +38,9 @@ IMAP cleanup before a connection was created
 <tool>
 lib1553
 </tool>
- <command>
+
+# this MUST use a host name that doesn't resolve
+<command>
 imap://non-existing-host.haxx.se:%IMAPPORT/1553
 </command>
 </client>

diff --git a/tests/data/test1590 b/tests/data/test1590
new file mode 100644 (file)
index 0000000..b605022
--- /dev/null
+++ b/tests/data/test1590
@@ -0,0 +1,54 @@
+<testcase>
+<info>
+<keywords>
+IMAP
+Clear Text
+FETCH
+</keywords>
+</info>
+
+#
+# Server-side
+<reply>
+<data>
+From: me@somewhere\r
+To: fake@nowhere\r
+\r
+body\r
+\r
+--\r
+  yours sincerely\r
+</data>
+<datacheck>
+</datacheck>
+<servercmd>
+</servercmd>
+</reply>
+
+#
+# Client-side
+<client>
+<server>
+imap
+</server>
+ <name>
+IMAP cleanup before a connection was created
+ </name>
+# tool is what to use instead of 'curl'
+<tool>
+lib1553
+</tool>
+
+# it is important this uses a host name that resolves successfully
+<command>
+imap://localhost:%IMAPPORT/1590
+</command>
+</client>
+
+#
+# Verify data after the test has been "shot"
+<verify>
+<protocol>
+</protocol>
+</verify>
+</testcase>