From d7934b8bd49114cbb54f7401742f7fb088e2f796 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Fri, 21 Oct 2011 23:36:54 +0200 Subject: curl_multi_fdset: correct fdset with FTP PORT use After a PORT has been issued, and the multi handle would switch to the CURLM_STATE_DO_MORE state (which is unique for FTP), libcurl would return the wrong fdset to wait for when curl_multi_fdset() is called. The code would blindly assume that it was waiting for a connect of the second connection, while that isn't true immediately after the PORT command. Also, the function multi.c:domore_getsock() was highly FTP-centric and therefore ugly to keep in protocol-agnostic code. I solved this problem by introducing a new function pointer in the Curl_handler struct called domore_getsock() which is only called during the DOMORE state for protocols that set that pointer. The new ftp.c:ftp_domore_getsock() function now returns fdset info about the control connection's command/response handling while such a state is in use, and goes over to waiting for a writable second connection first once the commands are done. The original problem could be seen by running test 525 and checking the time stamps in the FTP server log. I can verify that this fix at least fixes this problem. Bug: http://curl.haxx.se/mail/lib-2011-10/0250.html Reported by: Gokhan Sengun --- lib/curl_rtmp.c | 6 ++++++ lib/dict.c | 1 + lib/file.c | 1 + lib/ftp.c | 40 ++++++++++++++++++++++++++++++++++++++-- lib/gopher.c | 1 + lib/http.c | 2 ++ lib/imap.c | 4 ++++ lib/ldap.c | 2 ++ lib/multi.c | 16 ++++------------ lib/openldap.c | 2 ++ lib/pop3.c | 4 ++++ lib/rtsp.c | 1 + lib/smtp.c | 4 ++++ lib/ssh.c | 2 ++ lib/telnet.c | 1 + lib/tftp.c | 1 + lib/url.c | 1 + lib/urldata.h | 6 ++++++ 18 files changed, 81 insertions(+), 14 deletions(-) (limited to 'lib') diff --git a/lib/curl_rtmp.c b/lib/curl_rtmp.c index 44b7e1524..725b8f995 100644 --- a/lib/curl_rtmp.c +++ b/lib/curl_rtmp.c @@ -71,6 +71,7 @@ const struct Curl_handler Curl_handler_rtmp = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ rtmp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -90,6 +91,7 @@ const struct Curl_handler Curl_handler_rtmpt = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ rtmp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -109,6 +111,7 @@ const struct Curl_handler Curl_handler_rtmpe = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ rtmp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -128,6 +131,7 @@ const struct Curl_handler Curl_handler_rtmpte = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ rtmp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -147,6 +151,7 @@ const struct Curl_handler Curl_handler_rtmps = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ rtmp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -166,6 +171,7 @@ const struct Curl_handler Curl_handler_rtmpts = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ rtmp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/dict.c b/lib/dict.c index 36bb28923..b326054d1 100644 --- a/lib/dict.c +++ b/lib/dict.c @@ -92,6 +92,7 @@ const struct Curl_handler Curl_handler_dict = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/file.c b/lib/file.c index d4ca3e5ef..00d5fc09b 100644 --- a/lib/file.c +++ b/lib/file.c @@ -113,6 +113,7 @@ const struct Curl_handler Curl_handler_file = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ file_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/ftp.c b/lib/ftp.c index a078c5e71..12b41ecc5 100644 --- a/lib/ftp.c +++ b/lib/ftp.c @@ -134,9 +134,10 @@ static CURLcode ftp_connect(struct connectdata *conn, bool *done); static CURLcode ftp_disconnect(struct connectdata *conn, bool dead_connection); static CURLcode ftp_nextconnect(struct connectdata *conn); static CURLcode ftp_multi_statemach(struct connectdata *conn, bool *done); -static int ftp_getsock(struct connectdata *conn, - curl_socket_t *socks, +static int ftp_getsock(struct connectdata *conn, curl_socket_t *socks, int numsocks); +static int ftp_domore_getsock(struct connectdata *conn, curl_socket_t *socks, + int numsocks); static CURLcode ftp_doing(struct connectdata *conn, bool *dophase_done); static CURLcode ftp_setup_connection(struct connectdata * conn); @@ -171,6 +172,7 @@ const struct Curl_handler Curl_handler_ftp = { ftp_doing, /* doing */ ftp_getsock, /* proto_getsock */ ftp_getsock, /* doing_getsock */ + ftp_domore_getsock, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ftp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -196,6 +198,7 @@ const struct Curl_handler Curl_handler_ftps = { ftp_doing, /* doing */ ftp_getsock, /* proto_getsock */ ftp_getsock, /* doing_getsock */ + ftp_domore_getsock, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ftp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -222,6 +225,7 @@ static const struct Curl_handler Curl_handler_ftp_proxy = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -247,6 +251,7 @@ static const struct Curl_handler Curl_handler_ftps_proxy = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -634,6 +639,37 @@ static int ftp_getsock(struct connectdata *conn, return Curl_pp_getsock(&conn->proto.ftpc.pp, socks, numsocks); } +/* For the FTP "DO_MORE" phase only */ +static int ftp_domore_getsock(struct connectdata *conn, curl_socket_t *socks, + int numsocks) +{ + struct ftp_conn *ftpc = &conn->proto.ftpc; + + if(!numsocks) + return GETSOCK_BLANK; + + /* When in DO_MORE state, we could be either waiting for us to connect to a + remote site, or we could wait for that site to connect to us. Or just + handle ordinary commands. + + When waiting for a connect, we will be in FTP_STOP state and then we wait + for the secondary socket to become writeable. If we're in another state, + we're still handling commands on the control (primary) connection. + + */ + + switch(ftpc->state) { + case FTP_STOP: + break; + default: + return Curl_pp_getsock(&conn->proto.ftpc.pp, socks, numsocks); + } + + socks[0] = conn->sock[SECONDARYSOCKET]; + + return GETSOCK_READSOCK(0); +} + /* This is called after the FTP_QUOTE state is passed. ftp_state_cwd() sends the range of CWD commands to the server to change to diff --git a/lib/gopher.c b/lib/gopher.c index 38f1f64a9..b4efae8cc 100644 --- a/lib/gopher.c +++ b/lib/gopher.c @@ -97,6 +97,7 @@ const struct Curl_handler Curl_handler_gopher = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/http.c b/lib/http.c index e41cb7af3..f85cd6c55 100644 --- a/lib/http.c +++ b/lib/http.c @@ -118,6 +118,7 @@ const struct Curl_handler Curl_handler_http = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ http_getsock_do, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -141,6 +142,7 @@ const struct Curl_handler Curl_handler_https = { ZERO_NULL, /* doing */ https_getsock, /* proto_getsock */ http_getsock_do, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/imap.c b/lib/imap.c index 37c0d0e73..986b79a56 100644 --- a/lib/imap.c +++ b/lib/imap.c @@ -119,6 +119,7 @@ const struct Curl_handler Curl_handler_imap = { imap_doing, /* doing */ imap_getsock, /* proto_getsock */ imap_getsock, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ imap_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -144,6 +145,7 @@ const struct Curl_handler Curl_handler_imaps = { imap_doing, /* doing */ imap_getsock, /* proto_getsock */ imap_getsock, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ imap_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -169,6 +171,7 @@ static const struct Curl_handler Curl_handler_imap_proxy = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -194,6 +197,7 @@ static const struct Curl_handler Curl_handler_imaps_proxy = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/ldap.c b/lib/ldap.c index 11b9beeb7..737847649 100644 --- a/lib/ldap.c +++ b/lib/ldap.c @@ -130,6 +130,7 @@ const struct Curl_handler Curl_handler_ldap = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -154,6 +155,7 @@ const struct Curl_handler Curl_handler_ldaps = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/multi.c b/lib/multi.c index e53d387f0..6e4ec37a8 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -816,20 +816,12 @@ static int waitconnect_getsock(struct connectdata *conn, } static int domore_getsock(struct connectdata *conn, - curl_socket_t *sock, + curl_socket_t *socks, int numsocks) { - if(!numsocks) - return GETSOCK_BLANK; - - /* When in DO_MORE state, we could be either waiting for us - to connect to a remote site, or we could wait for that site - to connect to us. It makes a difference in the way: if we - connect to the site we wait for the socket to become writable, if - the site connects to us we wait for it to become readable */ - sock[0] = conn->sock[SECONDARYSOCKET]; - - return GETSOCK_WRITESOCK(0); + if(conn && conn->handler->domore_getsock) + return conn->handler->domore_getsock(conn, socks, numsocks); + return GETSOCK_BLANK; } /* returns bitmapped flags for this handle and its sockets */ diff --git a/lib/openldap.c b/lib/openldap.c index 070892524..e5a3369a1 100644 --- a/lib/openldap.c +++ b/lib/openldap.c @@ -83,6 +83,7 @@ const struct Curl_handler Curl_handler_ldap = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ldap_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -107,6 +108,7 @@ const struct Curl_handler Curl_handler_ldaps = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ldap_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/pop3.c b/lib/pop3.c index c5a0f5b9b..cb70679c5 100644 --- a/lib/pop3.c +++ b/lib/pop3.c @@ -119,6 +119,7 @@ const struct Curl_handler Curl_handler_pop3 = { pop3_doing, /* doing */ pop3_getsock, /* proto_getsock */ pop3_getsock, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ pop3_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -144,6 +145,7 @@ const struct Curl_handler Curl_handler_pop3s = { pop3_doing, /* doing */ pop3_getsock, /* proto_getsock */ pop3_getsock, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ pop3_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -169,6 +171,7 @@ static const struct Curl_handler Curl_handler_pop3_proxy = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -194,6 +197,7 @@ static const struct Curl_handler Curl_handler_pop3s_proxy = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/rtsp.c b/lib/rtsp.c index 198c25dc1..5d62ac7c7 100644 --- a/lib/rtsp.c +++ b/lib/rtsp.c @@ -113,6 +113,7 @@ const struct Curl_handler Curl_handler_rtsp = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ rtsp_getsock_do, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ rtsp_disconnect, /* disconnect */ rtsp_rtp_readwrite, /* readwrite */ diff --git a/lib/smtp.c b/lib/smtp.c index f85f9c433..8835b07a5 100644 --- a/lib/smtp.c +++ b/lib/smtp.c @@ -127,6 +127,7 @@ const struct Curl_handler Curl_handler_smtp = { smtp_doing, /* doing */ smtp_getsock, /* proto_getsock */ smtp_getsock, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ smtp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -151,6 +152,7 @@ const struct Curl_handler Curl_handler_smtps = { smtp_doing, /* doing */ smtp_getsock, /* proto_getsock */ smtp_getsock, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ smtp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -176,6 +178,7 @@ static const struct Curl_handler Curl_handler_smtp_proxy = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -200,6 +203,7 @@ static const struct Curl_handler Curl_handler_smtps_proxy = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/ssh.c b/lib/ssh.c index 089b76110..a9e4c56e4 100644 --- a/lib/ssh.c +++ b/lib/ssh.c @@ -165,6 +165,7 @@ const struct Curl_handler Curl_handler_scp = { scp_doing, /* doing */ ssh_getsock, /* proto_getsock */ ssh_getsock, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ssh_perform_getsock, /* perform_getsock */ scp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ @@ -189,6 +190,7 @@ const struct Curl_handler Curl_handler_sftp = { sftp_doing, /* doing */ ssh_getsock, /* proto_getsock */ ssh_getsock, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ssh_perform_getsock, /* perform_getsock */ sftp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/telnet.c b/lib/telnet.c index 890632214..59094b674 100644 --- a/lib/telnet.c +++ b/lib/telnet.c @@ -182,6 +182,7 @@ const struct Curl_handler Curl_handler_telnet = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/tftp.c b/lib/tftp.c index 8a85a1cdd..9b44a9b3d 100644 --- a/lib/tftp.c +++ b/lib/tftp.c @@ -184,6 +184,7 @@ const struct Curl_handler Curl_handler_tftp = { tftp_doing, /* doing */ tftp_getsock, /* proto_getsock */ tftp_getsock, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ tftp_disconnect, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/url.c b/lib/url.c index c3beaaa6f..40dcd1568 100644 --- a/lib/url.c +++ b/lib/url.c @@ -246,6 +246,7 @@ static const struct Curl_handler Curl_handler_dummy = { ZERO_NULL, /* doing */ ZERO_NULL, /* proto_getsock */ ZERO_NULL, /* doing_getsock */ + ZERO_NULL, /* domore_getsock */ ZERO_NULL, /* perform_getsock */ ZERO_NULL, /* disconnect */ ZERO_NULL, /* readwrite */ diff --git a/lib/urldata.h b/lib/urldata.h index 45ef2434e..5b3dc059e 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -671,6 +671,12 @@ struct Curl_handler { curl_socket_t *socks, int numsocks); + /* Called from the multi interface during the DO_MORE phase, and it should + then return a proper fd set */ + int (*domore_getsock)(struct connectdata *conn, + curl_socket_t *socks, + int numsocks); + /* Called from the multi interface during the DO_DONE, PERFORM and WAITPERFORM phases, and it should then return a proper fd set. Not setting this will make libcurl use the generic default one. */ -- cgit v1.2.3