Reported in #1561470 (http://curl.haxx.se/bug/view.cgi?id=1561470), libcurl

would crash if a bad function sequence was used when shutting down after
using the multi interface (i.e using easy_cleanup after multi_cleanup) so
precautions have been added to make sure it doesn't any more - test case 529
was added to verify.

10 files changed, 138 insertions, 35 deletions

diff --git a/CHANGES b/CHANGES
index 6718fb804..a277a3e23 100644
--- a/CHANGES
+++ b/CHANGES
@@ -6,6 +6,18 @@
 
                                   Changelog
 
+Daniel (28 September 2006)
+- Reported in #1561470 (http://curl.haxx.se/bug/view.cgi?id=1561470), libcurl
+  would crash if a bad function sequence was used when shutting down after
+  using the multi interface (i.e using easy_cleanup after multi_cleanup) so
+  precautions have been added to make sure it doesn't any more - test case 529
+  was added to verify.
+
+Daniel (27 September 2006)
+- The URL in the cookie jar file is now changed since it was giving a 404.
+  Reported by Timothy Stone. The new URL will take the visitor to a curl web
+  site mirror with the document.
+
 Daniel (24 September 2006)
 - Bernard Leak fixed configure --with-gssapi-libs.
 
diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index 70a33b5bf..11330d30d 100644
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -11,10 +11,11 @@ Curl and libcurl 7.16.0
 
 This release includes the following changes:
 
+ o curl_multi_socket() and family are suitable to start using
  o uses WSAPoll() on Windows Vista
  o (FTP) --ftp-ssl-control was added
  o CURLOPT_SSL_SESSIONID_CACHE and --no-sessionid added
- o CURLMOPT_PIPELINING added for enabling pipelined transfers
+ o CURLMOPT_PIPELINING added for enabling HTTP pipelined transfers
  o multi handles now have a shared connection cache
  o Added support for other MS-DOS compilers (besides djgpp)
  o CURLOPT_SOCKOPTFUNCTION and CURLOPT_SOCKOPTDATA were added
@@ -23,6 +24,9 @@ This release includes the following changes:
 
 This release includes the following bugfixes:
 
+ o multi interface crash if bad function call order was used for cleanup
+ o put a new URL in saved cookie jar files
+ o configure --with-gssapi-libs
  o SOCKS proxy connection fixes
  o (FTP) a failed upload does not invalidate the control connection
  o proxy URL with user name and empty password or no password at all now work
@@ -55,6 +59,6 @@ advice from friends like these:
  Domenico Andreoli, Armel Asselin, Gisle Vanem, Yang Tse, Andrew Biggs,
  Peter Sylvester, David McCreedy, Dmitriy Sergeyev, Dmitry Rechkin,
  Jari Sundell, Ravi Pratap, Michele Bini, Jeff Pohlmeyer, Michael Wallner,
- Mike Protts, Cory Nelson
+ Mike Protts, Cory Nelson, Bernard Leak
 
         Thanks! (and sorry if I forgot to mention someone)
diff --git a/lib/multi.c b/lib/multi.c
index 244994cfe..9d596401a 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -172,6 +172,8 @@ static bool multi_conn_using(struct Curl_multi *multi,
                              struct SessionHandle *data);
 static void singlesocket(struct Curl_multi *multi,
                          struct Curl_one_easy *easy);
+static void add_closure(struct Curl_multi *multi,
+                        struct SessionHandle *data);
 
 /* always use this function to change state, to make debugging easier */
 static void multistate(struct Curl_one_easy *easy, CURLMstate state)
@@ -539,17 +541,28 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle,
        we need to add this handle to the list of "easy handles kept around for
        nice connection closures".
      */
-    if(multi_conn_using(multi, easy->easy_handle))
+    if(multi_conn_using(multi, easy->easy_handle)) {
       /* There's at least one connection using this handle so we must keep
          this handle around. We also keep the connection cache pointer
          pointing to the shared one since that will be used on close as
          well. */
       easy->easy_handle->state.shared_conn = multi;
-    else
-      if(easy->easy_handle->state.connc->type == CONNCACHE_MULTI)
-        /* if this was using the shared connection cache we clear the pointer
-           to that */
-        easy->easy_handle->state.connc = NULL;
+
+      /* this handle is still being used by a shared connection cache and
+         thus we leave it around for now */
+      add_closure(multi, easy->easy_handle);
+    }
+
+    if(easy->easy_handle->state.connc->type == CONNCACHE_MULTI) {
+      /* if this was using the shared connection cache we clear the pointer
+         to that since we're not part of that handle anymore */
+      easy->easy_handle->state.connc = NULL;
+
+      /* and modify the connectindex since this handle can't point to the
+         connection cache anymore */
+      if(easy->easy_conn)
+        easy->easy_conn->connectindex = -1;
+    }
 
     /* change state without using multistate(), only to make singlesocket() do
        what we want */
@@ -1320,15 +1333,20 @@ CURLMcode curl_multi_cleanup(CURLM *multi_handle)
     /* go over all connections that have close actions */
     for(i=0; i< multi->connc->num; i++) {
       if(multi->connc->connects[i] &&
-         multi->connc->connects[i]->protocol & PROT_CLOSEACTION)
+         multi->connc->connects[i]->protocol & PROT_CLOSEACTION) {
         Curl_disconnect(multi->connc->connects[i]);
+        multi->connc->connects[i] = NULL;
+      }
     }
     /* now walk through the list of handles we kept around only to be
        able to close connections "properly" */
     cl = multi->closure;
     while(cl) {
       cl->easy_handle->state.shared_conn = NULL; /* no more shared */
-      Curl_close(cl->easy_handle); /* close handle */
+      if(cl->easy_handle->state.closed)
+        /* close handle only if curl_easy_cleanup() already has been called
+           for this easy handle */
+        Curl_close(cl->easy_handle);
       n = cl->next;
       free(cl);
       cl= n;
@@ -1780,8 +1798,8 @@ static bool multi_conn_using(struct Curl_multi *multi,
 
 /* add the given data pointer to the list of 'closure handles' that are
    kept around only to be able to close some connections nicely */
-void Curl_multi_add_closure(struct Curl_multi *multi,
-                            struct SessionHandle *data)
+static void add_closure(struct Curl_multi *multi,
+                        struct SessionHandle *data)
 {
   int i;
   struct closure *cl = (struct closure *)calloc(sizeof(struct closure), 1);
diff --git a/lib/multiif.h b/lib/multiif.h
index 45b3d2032..800aa0f5d 100644
--- a/lib/multiif.h
+++ b/lib/multiif.h
@@ -32,9 +32,6 @@ void Curl_multi_rmeasy(void *multi, CURL *data);
 
 bool Curl_multi_canPipeline(struct Curl_multi* multi);
 
-void Curl_multi_add_closure(struct Curl_multi *multi,
-                            struct SessionHandle *data);
-
 /* the write bits start at bit 16 for the *getsock() bitmap */
 #define GETSOCK_WRITEBITSTART 16
 
diff --git a/lib/url.c b/lib/url.c
index 50390108f..968421079 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -269,29 +269,31 @@ CURLcode Curl_close(struct SessionHandle *data)
                       the multi handle, since that function uses the magic
                       field! */
 
-  if(data->state.connc && (data->state.connc->type == CONNCACHE_PRIVATE)) {
-    /* close all connections still alive that are in the private connection
-       cache, as we no longer have the pointer left to the shared one. */
-    close_connections(data);
+  if(data->state.connc) {
 
-    /* free the connection cache if allocated privately */
-    Curl_rm_connc(data->state.connc);
-  }
+    if(data->state.connc->type == CONNCACHE_PRIVATE) {
+      /* close all connections still alive that are in the private connection
+         cache, as we no longer have the pointer left to the shared one. */
+      close_connections(data);
 
-  if ( ! (data->share && data->share->hostcache) ) {
-    if ( !Curl_global_host_cache_use(data)) {
-      Curl_hash_destroy(data->dns.hostcache);
+      /* free the connection cache if allocated privately */
+      Curl_rm_connc(data->state.connc);
     }
   }
 
   if(data->state.shared_conn) {
-    /* this handle is still being used by a shared connection cache and thus
-       we leave it around for now */
-    Curl_multi_add_closure(data->state.shared_conn, data);
-
+    /* marked to be used by a pending connection so we can't kill this handle
+       just yet */
+    data->state.closed = TRUE;
     return CURLE_OK;
   }
 
+  if ( ! (data->share && data->share->hostcache) ) {
+    if ( !Curl_global_host_cache_use(data)) {
+      Curl_hash_destroy(data->dns.hostcache);
+    }
+  }
+
   /* Free the pathbuffer */
   Curl_safefree(data->reqdata.pathbuffer);
   Curl_safefree(data->reqdata.proto.generic);
diff --git a/lib/urldata.h b/lib/urldata.h
index d90c6cd08..875dd0032 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -1053,6 +1053,9 @@ struct UrlState {
      must keep it around and add it to the list of handles to kill once all
      its connections are gone */
   void *shared_conn;
+  bool closed; /* set to TRUE when curl_easy_cleanup() has been called on this
+                  handle, but it is kept around as mentioned for
+                  shared_conn */
 };
 
 
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index a5eec9545..067499c6f 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -35,6 +35,4 @@ EXTRA_DIST = test1 test108 test117 test127 test20 test27 test34 test46	   \
  test256 test257 test258 test259 test260 test261 test262 test263 test264   \
  test265 test266 test267 test268 test269 test270 test271 test272 test273   \
  test274 test275 test524 test525 test276 test277 test526 test527 test528   \
- test530 DISABLED test278 test279 test531 test280
-
-
+ test530 DISABLED test278 test279 test531 test280 test529
diff --git a/tests/data/test529 b/tests/data/test529
new file mode 100644
index 000000000..d5c657d30
--- /dev/null
+++ b/tests/data/test529
@@ -0,0 +1,55 @@
+<info>
+<keywords>
+FTP
+PORT
+STOR
+</keywords>
+</info>
+# Server-side
+<reply>
+<data>
+</data>
+</reply>
+
+# Client-side
+<client>
+<server>
+ftp
+</server>
+<tool>
+lib529
+</tool>
+ <name>
+FTP PORT upload using multi interface (weird cleanup function sequence)
+ </name>
+ <command>
+ftp://%HOSTIP:%FTPPORT/path/529 log/upload529
+</command>
+<file name="log/upload529">
+Moooooooooooo
+ upload this
+</file>
+</client>
+
+# Verify data after the test has been "shot"
+<verify>
+<strip>
+^PORT .*
+^EPRT .*
+^LPRT .*
+</strip>
+<protocol>
+USER anonymous

+PASS curl_by_daniel@haxx.se

+PWD

+CWD path

+PORT 127,0,0,1,5,109

+TYPE I

+STOR 529

+QUIT

+</protocol>
+<upload>
+Moooooooooooo
+ upload this
+</upload>
+</verify>
diff --git a/tests/libtest/Makefile.am b/tests/libtest/Makefile.am
index 877532dd9..4b52a5f9a 100644
--- a/tests/libtest/Makefile.am
+++ b/tests/libtest/Makefile.am
@@ -39,9 +39,10 @@ LIBDIR = $(top_builddir)/lib
 SUPPORTFILES = first.c test.h
 
 # These are all libcurl test programs
-noinst_PROGRAMS = lib500 lib501 lib502 lib503 lib504 lib505 lib506 lib507 \
-  lib508 lib509 lib510 lib511 lib512 lib513 lib514 lib515 lib516 lib517 \
-  lib518 lib519 lib520 lib521 lib523 lib524 lib525 lib526 lib527 lib530
+noinst_PROGRAMS = lib500 lib501 lib502 lib503 lib504 lib505 lib506	\
+  lib507 lib508 lib509 lib510 lib511 lib512 lib513 lib514 lib515 lib516	\
+  lib517 lib518 lib519 lib520 lib521 lib523 lib524 lib525 lib526 lib527	\
+  lib529 lib530
 
 lib500_SOURCES = lib500.c $(SUPPORTFILES)
 lib500_LDADD = $(LIBDIR)/libcurl.la
@@ -152,6 +153,11 @@ lib527_CFLAGS = -DLIB527
 lib527_LDADD = $(LIBDIR)/libcurl.la
 lib527_DEPENDENCIES = $(LIBDIR)/libcurl.la
 
+lib529_SOURCES = lib525.c $(SUPPORTFILES)
+lib529_CFLAGS = -DLIB529
+lib529_LDADD = $(LIBDIR)/libcurl.la
+lib529_DEPENDENCIES = $(LIBDIR)/libcurl.la
+
 lib530_SOURCES = lib530.c $(SUPPORTFILES)
 lib530_CFLAGS = -DLIB530
 lib530_LDADD = $(LIBDIR)/libcurl.la
diff --git a/tests/libtest/lib525.c b/tests/libtest/lib525.c
index 77e5c203a..240628028 100644
--- a/tests/libtest/lib525.c
+++ b/tests/libtest/lib525.c
@@ -123,9 +123,17 @@ int test(char *URL)
     res = CURLM_CALL_MULTI_PERFORM;
   }
 
+#ifdef LIB529
+  /* test 529 */
+  curl_multi_remove_handle(m, curl);
+  curl_multi_cleanup(m);
+  curl_easy_cleanup(curl);
+#else
+  /* test 525 */
   curl_multi_remove_handle(m, curl);
   curl_easy_cleanup(curl);
   curl_multi_cleanup(m);
+#endif
 
   fclose(hd_src); /* close the local file */