From: Stefan Eissing Date: Fri, 13 Jun 2025 07:43:40 +0000 (+0200) Subject: lib: avoid reusing unclean connection X-Git-Tag: curl-8_15_0~265 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=66d35ee5d4c184fff90d545426ca4525c6ad966d;p=thirdparty%2Fcurl.git lib: avoid reusing unclean connection When `curl_easy_cleanup()` is invoked while still being part of a multi handle, the code will auto-remove it. But since the connection was detached first, the code in `curl_multi_remove_handle()` that invalidates dirty connections did not bite. Attach the connection *after* the possible remove from a multi handle, so that connection reuse can be prevented. Add test753 to reproduce and verify the fix. This required adding the new debug env var CURL_FTP_PWD_STOP, to have a transfer return from multi_perform() early with a connection that does not show and pending input. Reported-by: Brian Harris Fixes https://github.com/curl/curl/issues/17578 Closes https://github.com/curl/curl/pull/17607 --- diff --git a/docs/libcurl/libcurl-env-dbg.md b/docs/libcurl/libcurl-env-dbg.md index ce4e3c006a..3fcc1935d5 100644 --- a/docs/libcurl/libcurl-env-dbg.md +++ b/docs/libcurl/libcurl-env-dbg.md @@ -83,6 +83,12 @@ When built with c-ares for name resolving, setting this environment variable to `[IP:port]` makes libcurl use that DNS server instead of the system default. This is used by the curl test suite. +## `CURL_FTP_PWD_STOP` + +When set, the first transfer - when using ftp: - returns before sending +the `PWD` command and stop any further progress. This is used to test +an edge case + ## `CURL_GETHOSTNAME` Fake the local machine's unqualified hostname for NTLM and SMTP. diff --git a/lib/ftp.c b/lib/ftp.c index 6246b8d3ac..413505e6f2 100644 --- a/lib/ftp.c +++ b/lib/ftp.c @@ -765,7 +765,12 @@ static CURLcode ftp_state_user(struct Curl_easy *data, static CURLcode ftp_state_pwd(struct Curl_easy *data, struct ftp_conn *ftpc) { - CURLcode result = Curl_pp_sendf(data, &ftpc->pp, "%s", "PWD"); + CURLcode result; +#ifdef DEBUGBUILD + if(!data->id && getenv("CURL_FTP_PWD_STOP")) + return CURLE_OK; +#endif + result = Curl_pp_sendf(data, &ftpc->pp, "%s", "PWD"); if(!result) ftp_state(data, ftpc, FTP_PWD); diff --git a/lib/url.c b/lib/url.c index 3597c5cb45..b7e58e1004 100644 --- a/lib/url.c +++ b/lib/url.c @@ -234,22 +234,24 @@ CURLcode Curl_close(struct Curl_easy **datap) data = *datap; *datap = NULL; - /* Detach connection if any is left. This should not be normal, but can be - the case for example with CONNECT_ONLY + recv/send (test 556) */ - Curl_detach_connection(data); - if(!data->state.internal) { - if(data->multi) - /* This handle is still part of a multi handle, take care of this first - and detach this handle from there. */ - curl_multi_remove_handle(data->multi, data); - - if(data->multi_easy) { + if(!data->state.internal && data->multi) { + /* This handle is still part of a multi handle, take care of this first + and detach this handle from there. + This detaches the connection. */ + curl_multi_remove_handle(data->multi, data); + } + else { + /* Detach connection if any is left. This should not be normal, but can be + the case for example with CONNECT_ONLY + recv/send (test 556) */ + Curl_detach_connection(data); + if(!data->state.internal && data->multi_easy) { /* when curl_easy_perform() is used, it creates its own multi handle to use and this is the one */ curl_multi_cleanup(data->multi_easy); data->multi_easy = NULL; } } + DEBUGASSERT(!data->conn || data->state.internal); Curl_expire_clear(data); /* shut off any timers left */ diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index ab21e0e220..8fc0b3480a 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -107,7 +107,7 @@ test709 test710 test711 test712 test713 test714 test715 test716 test717 \ test718 test719 test720 test721 test722 test723 test724 test725 test726 \ test727 test728 test729 test730 test731 test732 test733 test734 test735 \ test736 test737 test738 test739 test740 test741 test742 test743 test744 \ -test745 test746 test747 test748 test749 test750 test751 test752 \ +test745 test746 test747 test748 test749 test750 test751 test752 test753 \ \ test780 test781 test782 test783 test784 test785 test786 test787 test788 \ test789 test790 test791 \ diff --git a/tests/data/test753 b/tests/data/test753 new file mode 100644 index 0000000000..3913567205 --- /dev/null +++ b/tests/data/test753 @@ -0,0 +1,45 @@ + + + +FTP + + + +# Client-side + + +Debug +ftp + + +ftp + + +lib%TESTNUMBER + + +cleanup easy without multi_remove_handle + + +ftp://%HOSTIP:%FTPPORT/%TESTNUMBER + + +CURL_FTP_PWD_STOP=1 + + + +# Verify data after the test has been "shot" + + +USER anonymous +PASS ftp@example.com +USER anonymous +PASS ftp@example.com +PWD +EPSV +TYPE I +SIZE 753 +QUIT + + + diff --git a/tests/libtest/Makefile.inc b/tests/libtest/Makefile.inc index 41c9441d11..9939c94d45 100644 --- a/tests/libtest/Makefile.inc +++ b/tests/libtest/Makefile.inc @@ -59,7 +59,7 @@ TESTFILES = \ lib643.c lib650.c lib651.c lib652.c lib653.c lib654.c lib655.c lib658.c \ lib659.c lib661.c lib666.c lib667.c lib668.c \ lib670.c lib674.c lib676.c lib677.c lib678.c lib694.c lib695.c \ - lib751.c \ + lib751.c lib753.c \ lib1156.c \ lib1301.c \ lib1308.c \ diff --git a/tests/libtest/lib753.c b/tests/libtest/lib753.c new file mode 100644 index 0000000000..6f12069f87 --- /dev/null +++ b/tests/libtest/lib753.c @@ -0,0 +1,186 @@ +/*************************************************************************** + * _ _ ____ _ + * Project ___| | | | _ \| | + * / __| | | | |_) | | + * | (__| |_| | _ <| |___ + * \___|\___/|_| \_\_____| + * + * Copyright (C) Daniel Stenberg, , et al. + * + * This software is licensed as described in the file COPYING, which + * you should have received as part of this distribution. The terms + * are also available at https://curl.se/docs/copyright.html. + * + * You may opt to use, copy, modify, merge, publish, distribute and/or sell + * copies of the Software, and permit persons to whom the Software is + * furnished to do so, under the terms of the COPYING file. + * + * This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY + * KIND, either express or implied. + * + * SPDX-License-Identifier: curl + * + ***************************************************************************/ +#include "test.h" + +#include "testutil.h" +#include "testtrace.h" +#include "warnless.h" +#include "memdebug.h" + +struct t753_transfer_status { + CURL *easy; + const char *name; + bool pause; + bool is_paused; + bool seen_welcome; +}; + +static size_t t753_write_cb(char *ptr, size_t size, size_t nmemb, void *userp) +{ + struct t753_transfer_status *st = userp; + size_t len = size * nmemb; + (void)ptr; + if(st->pause) { + curl_mfprintf(stderr, "[%s] write_cb(len=%zu), PAUSE\n", st->name, len); + st->is_paused = TRUE; + return CURL_READFUNC_PAUSE; + } + curl_mfprintf(stderr, "[%s] write_cb(len=%zu), CONSUME\n", st->name, len); + st->is_paused = FALSE; + return len; +} + +static size_t t753_hd_cb(char *ptr, size_t size, size_t nmemb, void *userp) +{ + struct t753_transfer_status *st = userp; + size_t len = size * nmemb; + curl_mfprintf(stderr, "[%s] hd_cb '%.*s'\n", st->name, (int)len, ptr); + if(!strcmp("230 Welcome you silly person\r\n", ptr)) { + st->seen_welcome = TRUE; + st->easy = NULL; + } + return len; +} + +static bool t753_setup(char *URL, const char *name, + CURL **peasy, + struct t753_transfer_status *st) +{ + CURL *easy = NULL; + CURLcode res = CURLE_OK; + + *peasy = NULL; + memset(st, 0, sizeof(*st)); + st->name = name; + st->easy = easy; + st->pause = TRUE; + + easy_init(easy); + + easy_setopt(easy, CURLOPT_URL, URL); + easy_setopt(easy, CURLOPT_WRITEFUNCTION, t753_write_cb); + easy_setopt(easy, CURLOPT_WRITEDATA, st); + easy_setopt(easy, CURLOPT_HEADERFUNCTION, t753_hd_cb); + easy_setopt(easy, CURLOPT_HEADERDATA, st); + + easy_setopt(easy, CURLOPT_NOPROGRESS, 1L); + easy_setopt(easy, CURLOPT_DEBUGDATA, &libtest_debug_config); + easy_setopt(easy, CURLOPT_DEBUGFUNCTION, libtest_debug_cb); + easy_setopt(easy, CURLOPT_VERBOSE, 1L); + + *peasy = easy; + return TRUE; + +test_cleanup: + if(easy) + curl_easy_cleanup(easy); + return FALSE; +} + +static CURLcode test_lib753(char *URL) +{ + CURL *easy1 = NULL, *easy2 = NULL; + CURLM *multi = NULL; + struct t753_transfer_status st1, st2; + CURLcode res = CURLE_OK; + CURLMcode mres; + int still_running; + + start_test_timing(); + + libtest_debug_config.nohex = 1; + libtest_debug_config.tracetime = 1; + + curl_global_init(CURL_GLOBAL_DEFAULT); + + curl_mfprintf(stderr, "init multi\n"); + multi = curl_multi_init(); + if(!multi) { + res = CURLE_OUT_OF_MEMORY; + goto test_cleanup; + } + + if(!t753_setup(URL, "EASY1", &easy1, &st1)) + goto test_cleanup; + + multi_add_handle(multi, easy1); + + multi_perform(multi, &still_running); + abort_on_test_timeout(); + curl_mfprintf(stderr, "multi_perform() -> %d running\n", still_running); + + while(still_running) { + int num; + + /* The purpose of this Test: + * 1. Violently cleanup EASY1 *without* removing it from the multi + * handle first. This MUST discard the connection that EASY1 holds, + * as EASY1 is not DONE at this point. + * With the env var CURL_FTP_PWD_STOP set, the connection will + * have no outstanding data at this point. This would allow + * reuse if the connection is not terminated by the cleanup. + * 2. Add EASY2 for the same URL and observe in the expected result + * that the connection is NOT reused, e.g. all FTP commands + * are sent again on the new connection. + */ + if(easy1 && st1.seen_welcome) { + curl_easy_cleanup(easy1); + easy1 = NULL; + if(!easy2) { + if(!t753_setup(URL, "EASY2", &easy2, &st2)) + goto test_cleanup; + st2.pause = FALSE; + multi_add_handle(multi, easy2); + } + } + + mres = curl_multi_wait(multi, NULL, 0, 1, &num); + if(mres != CURLM_OK) { + curl_mfprintf(stderr, "curl_multi_wait() returned %d\n", mres); + res = TEST_ERR_MAJOR_BAD; + goto test_cleanup; + } + + abort_on_test_timeout(); + + multi_perform(multi, &still_running); + curl_mfprintf(stderr, "multi_perform() -> %d running\n", still_running); + + abort_on_test_timeout(); + } + +test_cleanup: + + if(res) + curl_mfprintf(stderr, "ERROR: %s\n", curl_easy_strerror(res)); + + if(easy1) + curl_easy_cleanup(easy1); + if(easy2) + curl_easy_cleanup(easy2); + curl_multi_cleanup(multi); + curl_global_cleanup(); + + return res; +}