From ae13c93b7db9f9c68eaf95150ed551b3b649d8c4 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Thu, 28 Sep 2006 21:26:06 +0000 Subject: [PATCH] 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. --- CHANGES | 12 +++++++++++ RELEASE-NOTES | 8 +++++-- lib/multi.c | 38 +++++++++++++++++++++++--------- lib/multiif.h | 3 --- lib/url.c | 30 ++++++++++++++------------ lib/urldata.h | 3 +++ tests/data/Makefile.am | 4 +--- tests/data/test529 | 55 +++++++++++++++++++++++++++++++++++++++++++++++ tests/libtest/Makefile.am | 12 ++++++++--- tests/libtest/lib525.c | 8 +++++++ 10 files changed, 138 insertions(+), 35 deletions(-) create mode 100644 tests/data/test529 diff --git a/CHANGES b/CHANGES index 6718fb8..a277a3e 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 70a33b5..11330d3 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 244994c..9d59640 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 45b3d20..800aa0f 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 5039010..9684210 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 d90c6cd..875dd00 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 a5eec95..067499c 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 0000000..d5c657d --- /dev/null +++ b/tests/data/test529 @@ -0,0 +1,55 @@ + + +FTP +PORT +STOR + + +# Server-side + + + + + +# Client-side + + +ftp + + +lib529 + + +FTP PORT upload using multi interface (weird cleanup function sequence) + + +ftp://%HOSTIP:%FTPPORT/path/529 log/upload529 + + +Moooooooooooo + upload this + + + +# Verify data after the test has been "shot" + + +^PORT .* +^EPRT .* +^LPRT .* + + +USER anonymous +PASS curl_by_daniel@haxx.se +PWD +CWD path +PORT 127,0,0,1,5,109 +TYPE I +STOR 529 +QUIT + + +Moooooooooooo + upload this + + diff --git a/tests/libtest/Makefile.am b/tests/libtest/Makefile.am index 877532d..4b52a5f 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 77e5c20..2406280 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 */ -- 2.7.4