From b2954e66e87be7414a4508f8167ca531e653bea8 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Sun, 4 Nov 2012 18:22:48 +0100 Subject: [PATCH] FTP: prevent the multi interface from blocking As pointed out in Bug report #3579064, curl_multi_perform() would wrongly use a blocking mechanism internally for some commands which could lead to for example a very long block if the LIST response never showed. The solution was to make sure to properly continue to use the multi interface non-blocking state machine. The new test 1501 verifies the fix. Bug: http://curl.haxx.se/bug/view.cgi?id=3579064 Reported by: Guido Berhoerster --- lib/ftp.c | 56 ++++++++++++----- lib/pingpong.c | 5 +- tests/data/Makefile.am | 2 +- tests/data/test1501 | 53 ++++++++++++++++ tests/data/test591 | 3 +- tests/data/test592 | 5 +- tests/libtest/.gitignore | 2 +- tests/libtest/Makefile.inc | 4 +- tests/libtest/lib1501.c | 126 +++++++++++++++++++++++++++++++++++++ 9 files changed, 234 insertions(+), 22 deletions(-) create mode 100644 tests/data/test1501 create mode 100644 tests/libtest/lib1501.c diff --git a/lib/ftp.c b/lib/ftp.c index 02c671a96..bb7d7a496 100644 --- a/lib/ftp.c +++ b/lib/ftp.c @@ -666,11 +666,18 @@ static CURLcode ftp_readresp(curl_socket_t sockfd, if(ftpcode) *ftpcode = code; - if(421 == code) + if(421 == code) { /* 421 means "Service not available, closing control connection." and FTP * servers use it to signal that idle session timeout has been exceeded. - * If we ignored the response, it could end up hanging in some cases. */ + * If we ignored the response, it could end up hanging in some cases. + * + * This response code can come at any point so having it treated + * generically is a good idea. + */ + infof(data, "We got a 421 - timeout!\n"); + state(conn, FTP_STOP); return CURLE_OPERATION_TIMEDOUT; + } return result; } @@ -2394,6 +2401,7 @@ static CURLcode ftp_state_stor_resp(struct connectdata *conn, if(ftpcode>=400) { failf(data, "Failed FTP upload: %0d", ftpcode); + state(conn, FTP_STOP); /* oops, we never close the sockets! */ return CURLE_UPLOAD_FAILED; } @@ -2411,9 +2419,6 @@ static CURLcode ftp_state_stor_resp(struct connectdata *conn, if(!connected) { struct ftp_conn *ftpc = &conn->proto.ftpc; infof(data, "Data conn was not available immediately\n"); - /* as there's not necessarily an immediate action on the control - connection now, we halt the state machine */ - state(conn, FTP_STOP); ftpc->wait_data_conn = TRUE; } @@ -3663,6 +3668,8 @@ static CURLcode ftp_do_more(struct connectdata *conn, bool *complete) /* the ftp struct is inited in ftp_connect() */ struct FTP *ftp = data->state.proto.ftp; + *complete = FALSE; + /* if the second connection isn't done yet, wait for it */ if(!conn->bits.tcpconnect[SECONDARYSOCKET]) { result = Curl_is_connected(conn, SECONDARYSOCKET, &connected); @@ -3675,6 +3682,18 @@ static CURLcode ftp_do_more(struct connectdata *conn, bool *complete) return result; } + if((data->state.used_interface == Curl_if_multi) && + ftpc->state) { + /* multi interface and already in a state so skip the intial commands. + They are only done to kickstart the do_more state */ + result = ftp_multi_statemach(conn, complete); + + /* if we got an error or if we don't wait for a data connection return + immediately */ + if(result || (ftpc->wait_data_conn != TRUE)) + return result; + } + if(ftp->transfer <= FTPTRANSFER_INFO) { /* a transfer is about to take place, or if not a file name was given so we'll do a SIZE on it later and then we need the right TYPE first */ @@ -3728,7 +3747,13 @@ static CURLcode ftp_do_more(struct connectdata *conn, bool *complete) return result; } } - result = ftp_easy_statemach(conn); + if(data->state.used_interface == Curl_if_multi) { + result = ftp_multi_statemach(conn, complete); + + return result; + } + else + result = ftp_easy_statemach(conn); } if((result == CURLE_OK) && (ftp->transfer != FTPTRANSFER_BODY)) @@ -4402,20 +4427,21 @@ CURLcode ftp_parse_url_path(struct connectdata *conn) static CURLcode ftp_dophase_done(struct connectdata *conn, bool connected) { - CURLcode result = CURLE_OK; struct FTP *ftp = conn->data->state.proto.ftp; struct ftp_conn *ftpc = &conn->proto.ftpc; if(connected) { bool completed; - result = ftp_do_more(conn, &completed); - } + CURLcode result = ftp_do_more(conn, &completed); - if(result && (conn->sock[SECONDARYSOCKET] != CURL_SOCKET_BAD)) { - /* Failure detected, close the second socket if it was created already */ - Curl_closesocket(conn, conn->sock[SECONDARYSOCKET]); - conn->sock[SECONDARYSOCKET] = CURL_SOCKET_BAD; - return result; + if(result) { + if(conn->sock[SECONDARYSOCKET] != CURL_SOCKET_BAD) { + /* close the second socket if it was created already */ + Curl_closesocket(conn, conn->sock[SECONDARYSOCKET]); + conn->sock[SECONDARYSOCKET] = CURL_SOCKET_BAD; + } + return result; + } } if(ftp->transfer != FTPTRANSFER_BODY) @@ -4427,7 +4453,7 @@ static CURLcode ftp_dophase_done(struct connectdata *conn, ftpc->ctl_valid = TRUE; /* seems good */ - return result; + return CURLE_OK; } /* called from multi.c while DOing */ diff --git a/lib/pingpong.c b/lib/pingpong.c index c10894654..85a7a45af 100644 --- a/lib/pingpong.c +++ b/lib/pingpong.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2011, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2012, 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 @@ -424,6 +424,9 @@ CURLcode Curl_pp_readresp(curl_socket_t sockfd, it may actually contain another end of response already! */ clipamount = gotbytes - i; restart = TRUE; + DEBUGF(infof(data, "Curl_pp_readresp_ %d bytes of trailing " + "server response left\n", + (int)clipamount)); } else if(keepon) { diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index d188d1909..382e4b105 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -93,7 +93,7 @@ test1379 test1380 test1381 test1382 test1383 test1384 test1385 test1386 \ test1387 test1388 test1389 test1390 test1391 test1392 test1393 \ test1400 test1401 test1402 test1403 test1404 test1405 test1406 test1407 \ test1408 test1409 test1410 test1411 \ -test1500 \ +test1500 test1501 \ test2000 test2001 test2002 test2003 test2004 test2005 test2006 test2007 \ test2008 test2009 test2010 test2011 test2012 test2013 test2014 test2015 \ test2016 test2017 test2018 test2019 test2020 test2021 test2022 \ diff --git a/tests/data/test1501 b/tests/data/test1501 new file mode 100644 index 000000000..8c7e13af7 --- /dev/null +++ b/tests/data/test1501 @@ -0,0 +1,53 @@ + + + +FTP +RETR +multi +LIST + + + +# Server-side + + + + +DELAY LIST 2 +DELAY TYPE 2 + + + +# Client-side + + +ftp + + +lib1501 + + +FTP with multi interface and slow LIST response + + +ftp://%HOSTIP:%FTPPORT/1501/ + + +# Verify data after the test has been "shot" + + +0 + + +USER anonymous +PASS ftp@example.com +PWD +CWD 1501 +EPSV +TYPE A +LIST +QUIT + + + + diff --git a/tests/data/test591 b/tests/data/test591 index e04ae5bcc..42a227112 100644 --- a/tests/data/test591 +++ b/tests/data/test591 @@ -63,8 +63,9 @@ TYPE I STOR 591 QUIT +# CURLE_UPLOAD_FAILED = 25 -10 +25 diff --git a/tests/data/test592 b/tests/data/test592 index 487290dd4..23aa6c465 100644 --- a/tests/data/test592 +++ b/tests/data/test592 @@ -52,6 +52,7 @@ Moooooooooooo for 592 s/^PORT (.*)/PORT/ s/^EPRT \|1\|(.*)/EPRT \|1\|/ +# a 421 response must prevent further commands from being sent USER anonymous PASS ftp@example.com @@ -61,10 +62,10 @@ EPRT |1| PORT TYPE I STOR 592 -QUIT +# 28 == CURLE_OPERATION_TIMEDOUT -10 +28 diff --git a/tests/libtest/.gitignore b/tests/libtest/.gitignore index 80be891d6..28e83c08d 100644 --- a/tests/libtest/.gitignore +++ b/tests/libtest/.gitignore @@ -1,5 +1,5 @@ chkhostname lib5[0-9][0-9] -lib1500 +lib150[0-9] libauthretry libntlmconnect diff --git a/tests/libtest/Makefile.inc b/tests/libtest/Makefile.inc index fa0c69cb5..e0ebcb4c7 100644 --- a/tests/libtest/Makefile.inc +++ b/tests/libtest/Makefile.inc @@ -20,7 +20,7 @@ noinst_PROGRAMS = chkhostname \ lib556 lib539 lib557 lib560 lib562 lib564 lib565 lib566 lib567 lib568 \ lib569 lib570 lib571 lib572 lib573 lib582 lib583 lib585 lib586 lib587 \ lib590 lib591 lib597 lib598 lib599 libauthretry libntlmconnect \ - lib1500 + lib1500 lib1501 chkhostname_SOURCES = chkhostname.c $(top_srcdir)/lib/curl_gethostname.c chkhostname_LDADD = @CURL_NETWORK_LIBS@ @@ -189,6 +189,8 @@ lib599_SOURCES = lib599.c $(SUPPORTFILES) lib1500_SOURCES = lib1500.c $(SUPPORTFILES) $(TESTUTIL) +lib1501_SOURCES = lib1501.c $(SUPPORTFILES) $(TESTUTIL) + libauthretry_SOURCES = libauthretry.c $(SUPPORTFILES) libntlmconnect_SOURCES = libntlmconnect.c $(SUPPORTFILES) $(TESTUTIL) diff --git a/tests/libtest/lib1501.c b/tests/libtest/lib1501.c new file mode 100644 index 000000000..8d1a40571 --- /dev/null +++ b/tests/libtest/lib1501.c @@ -0,0 +1,126 @@ +/*************************************************************************** + * _ _ ____ _ + * Project ___| | | | _ \| | + * / __| | | | |_) | | + * | (__| |_| | _ <| |___ + * \___|\___/|_| \_\_____| + * + * Copyright (C) 1998 - 2012, 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 http://curl.haxx.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. + * + ***************************************************************************/ +#include "test.h" + +#include + +#include "testutil.h" +#include "warnless.h" +#include "memdebug.h" + +#define TEST_HANG_TIMEOUT 30 * 1000 + +/* 500 milliseconds allowed. An extreme number but lets be really conservative + to allow old and slow machines to run this test too */ +#define MAX_BLOCKED_TIME_US 500000 + +/* return the number of microseconds between two time stamps */ +static int elapsed(struct timeval *before, + struct timeval *after) +{ + ssize_t result; + + result = (after->tv_sec - before->tv_sec) * 1000000 + + after->tv_usec - before->tv_usec; + if (result < 0) + result = 0; + + return curlx_sztosi(result); +} + + +int test(char *URL) +{ + CURL *handle = NULL; + CURLM *mhandle = NULL; + int res = 0; + int still_running = 0; + + start_test_timing(); + + global_init(CURL_GLOBAL_ALL); + + easy_init(handle); + + easy_setopt(handle, CURLOPT_URL, URL); + easy_setopt(handle, CURLOPT_VERBOSE, 1L); + + multi_init(mhandle); + + multi_add_handle(mhandle, handle); + + multi_perform(mhandle, &still_running); + + abort_on_test_timeout(); + + while(still_running) { + struct timeval timeout; + fd_set fdread; + fd_set fdwrite; + fd_set fdexcep; + int maxfd = -99; + struct timeval before; + struct timeval after; + int e; + + timeout.tv_sec = 0; + timeout.tv_usec = 100000L; /* 100 ms */ + + FD_ZERO(&fdread); + FD_ZERO(&fdwrite); + FD_ZERO(&fdexcep); + + multi_fdset(mhandle, &fdread, &fdwrite, &fdexcep, &maxfd); + + /* At this point, maxfd is guaranteed to be greater or equal than -1. */ + + select_test(maxfd+1, &fdread, &fdwrite, &fdexcep, &timeout); + + abort_on_test_timeout(); + + fprintf(stderr, "ping\n"); + gettimeofday(&before, 0); + + multi_perform(mhandle, &still_running); + + abort_on_test_timeout(); + + gettimeofday(&after, 0); + e = elapsed(&before, &after); + fprintf(stderr, "pong = %d\n", e); + + if(e > MAX_BLOCKED_TIME_US) { + res = 100; + break; + } + } + +test_cleanup: + + /* undocumented cleanup sequence - type UA */ + + curl_multi_cleanup(mhandle); + curl_easy_cleanup(handle); + curl_global_cleanup(); + + return res; +}