[pacman-dev] [PATCH 1/2] lib/dload: enforce usage of TCP keepalives
This is particularly important in the case of FTP control connections, which may be closed by rogue NAT/firewall devices detecting idle connections on larger transfers which may take 5-10+ minutes. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- This is basically on the advice of Daniel Stenberg, who mentions that it's a common problem that he's aware of. I have no idea if this is actually the fix we're looking for and I'm still unable to reproduce, but it seems plausible solution. Also, fuck FTP. configure.ac | 7 +++-- lib/libalpm/dload.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 6941054..c8ba03f 100644 --- a/configure.ac +++ b/configure.ac @@ -174,9 +174,10 @@ AM_CONDITIONAL([HAVE_LIBGPGME], [test "x$with_gpgme" = "xyes"]) # Checks for header files. AC_CHECK_HEADERS([fcntl.h float.h glob.h libintl.h limits.h locale.h \ - mntent.h stddef.h string.h sys/ioctl.h sys/mount.h \ - sys/param.h sys/statvfs.h sys/time.h sys/types.h \ - sys/ucred.h syslog.h termios.h wchar.h]) + mntent.h netinet/in.h netinet/tcp.h stddef.h string.h \ + sys/ioctl.h sys/mount.h sys/param.h sys/statvfs.h \ + sys/time.h sys/types.h sys/ucred.h syslog.h termios.h \ + wchar.h]) # Checks for typedefs, structures, and compiler characteristics. AC_C_INLINE diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index a3980d6..790080c 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -23,11 +23,19 @@ #include <errno.h> #include <string.h> #include <unistd.h> +#include <sys/socket.h> /* setsockopt, SO_KEEPALIVE */ #include <sys/time.h> #include <sys/types.h> #include <sys/stat.h> #include <signal.h> +#ifdef _HAVE_NETINET_IN_H +#include <netinet/in.h> /* IPPROTO_TCP */ +#endif +#ifdef _HAVE_NETINET_TCP_H +#include <netinet/tcp.h> /* TCP_KEEPINTVL, TCP_KEEPIDLE */ +#endif + #ifdef HAVE_LIBCURL #include <curl/curl.h> #endif @@ -215,6 +223,53 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) return realsize; } +static int curl_sockopt_cb(void *userdata, curl_socket_t curlfd, + curlsocktype purpose) +{ + alpm_handle_t *handle = userdata; + + int onoff = 1; + +#ifdef TCP_KEEPIDLE + int keepidle = TCP_KEEPIDLE; +#endif +#ifdef TCP_KEEPINTVL + int keepintvl = TCP_KEEPINTVL; +#endif + + switch(purpose) { + case CURLSOCKTYPE_IPCXN: + if(setsockopt(curlfd, SOL_SOCKET, SO_KEEPALIVE, (void *)&onoff, + sizeof(onoff)) < 0) { + /* don't abort operation, just log to debug */ + _alpm_log(handle, ALPM_LOG_DEBUG, "Failed to set SO_KEEPALIVE on fd %d\n", + curlfd); + } + else { +#ifdef TCP_KEEPIDLE + if(setsockopt(curlfd, IPPROTO_TCP, TCP_KEEPIDLE, (void *)&keepidle, + sizeof(keepidle)) < 0) { + /* don't abort operation, just log to debug */ + _alpm_log(handle, ALPM_LOG_DEBUG, "Failed to set TCP_KEEPIDLE on fd %d\n", + curlfd); + } +#endif +#ifdef TCP_KEEPINTVL + if(setsockopt(curlfd, IPPROTO_TCP, TCP_KEEPINTVL, (void *)&keepintvl, + sizeof(keepintvl)) < 0) { + /* don't abort operation, just log to debug */ + _alpm_log(handle, ALPM_LOG_DEBUG, "Failed to set TCP_KEEPINTVL on fd %d\n", + curlfd); + } +#endif + } + default: + break; + } + + return 0; +} + static void curl_set_handle_opts(struct dload_payload *payload, CURL *curl, char *error_buffer) { @@ -239,6 +294,8 @@ static void curl_set_handle_opts(struct dload_payload *payload, curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, parse_headers); curl_easy_setopt(curl, CURLOPT_WRITEHEADER, (void *)payload); curl_easy_setopt(curl, CURLOPT_NETRC, CURL_NETRC_OPTIONAL); + curl_easy_setopt(curl, CURLOPT_SOCKOPTFUNCTION, curl_sockopt_cb); + curl_easy_setopt(curl, CURLOPT_SOCKOPTDATA, (void *)handle); _alpm_log(handle, ALPM_LOG_DEBUG, "url: %s\n", payload->fileurl); @@ -388,6 +445,8 @@ static int curl_download_internal(struct dload_payload *payload, /* perform transfer */ payload->curlerr = curl_easy_perform(curl); + _alpm_log(handle, ALPM_LOG_DEBUG, "curl returned error %d from transfer\n", + payload->curlerr); /* disconnect relationships from the curl handle for things that might go out * of scope, but could still be touched on connection teardown. This really -- 1.7.8.4
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 790080c..c1be528 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -90,7 +90,7 @@ static void inthandler(int UNUSED signum) dload_interrupted = ABORT_SIGINT; } -static int curl_progress(void *file, double dltotal, double dlnow, +static int curl_progress_cb(void *file, double dltotal, double dlnow, double UNUSED ultotal, double UNUSED ulnow) { struct dload_payload *payload = (struct dload_payload *)file; @@ -192,7 +192,7 @@ static mode_t _getumask(void) return mask; } -static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) +static size_t curl_parseheader_cb(void *ptr, size_t size, size_t nmemb, void *user) { size_t realsize = size * nmemb; const char *fptr, *endptr = NULL; @@ -287,11 +287,11 @@ static void curl_set_handle_opts(struct dload_payload *payload, curl_easy_setopt(curl, CURLOPT_FILETIME, 1L); curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0L); curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L); - curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, curl_progress); + curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, curl_progress_cb); curl_easy_setopt(curl, CURLOPT_PROGRESSDATA, (void *)payload); curl_easy_setopt(curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, 10L); - curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, parse_headers); + curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, curl_parseheader_cb); curl_easy_setopt(curl, CURLOPT_WRITEHEADER, (void *)payload); curl_easy_setopt(curl, CURLOPT_NETRC, CURL_NETRC_OPTIONAL); curl_easy_setopt(curl, CURLOPT_SOCKOPTFUNCTION, curl_sockopt_cb); -- 1.7.8.4
On Sun, Jan 22, 2012 at 7:31 PM, Dave Reisner <d@falconindy.com> wrote:
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 790080c..c1be528 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -90,7 +90,7 @@ static void inthandler(int UNUSED signum) dload_interrupted = ABORT_SIGINT; }
-static int curl_progress(void *file, double dltotal, double dlnow, +static int curl_progress_cb(void *file, double dltotal, double dlnow, double UNUSED ultotal, double UNUSED ulnow) { struct dload_payload *payload = (struct dload_payload *)file; @@ -192,7 +192,7 @@ static mode_t _getumask(void) return mask; }
-static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) +static size_t curl_parseheader_cb(void *ptr, size_t size, size_t nmemb, void *user) Wouldn't it make more sense to not put our functions in the curl_* "namespace" and just use dload_parseheader_cb, etc. instead?
{ size_t realsize = size * nmemb; const char *fptr, *endptr = NULL; @@ -287,11 +287,11 @@ static void curl_set_handle_opts(struct dload_payload *payload, curl_easy_setopt(curl, CURLOPT_FILETIME, 1L); curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0L); curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L); - curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, curl_progress); + curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, curl_progress_cb); curl_easy_setopt(curl, CURLOPT_PROGRESSDATA, (void *)payload); curl_easy_setopt(curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, 10L); - curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, parse_headers); + curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, curl_parseheader_cb); curl_easy_setopt(curl, CURLOPT_WRITEHEADER, (void *)payload); curl_easy_setopt(curl, CURLOPT_NETRC, CURL_NETRC_OPTIONAL); curl_easy_setopt(curl, CURLOPT_SOCKOPTFUNCTION, curl_sockopt_cb); -- 1.7.8.4
On Mon, Jan 23, 2012 at 09:25:18AM -0600, Dan McGee wrote:
On Sun, Jan 22, 2012 at 7:31 PM, Dave Reisner <d@falconindy.com> wrote:
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 790080c..c1be528 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -90,7 +90,7 @@ static void inthandler(int UNUSED signum) dload_interrupted = ABORT_SIGINT; }
-static int curl_progress(void *file, double dltotal, double dlnow, +static int curl_progress_cb(void *file, double dltotal, double dlnow, double UNUSED ultotal, double UNUSED ulnow) { struct dload_payload *payload = (struct dload_payload *)file; @@ -192,7 +192,7 @@ static mode_t _getumask(void) return mask; }
-static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) +static size_t curl_parseheader_cb(void *ptr, size_t size, size_t nmemb, void *user) Wouldn't it make more sense to not put our functions in the curl_* "namespace" and just use dload_parseheader_cb, etc. instead?
Yeah, I'll buy this.
{ size_t realsize = size * nmemb; const char *fptr, *endptr = NULL; @@ -287,11 +287,11 @@ static void curl_set_handle_opts(struct dload_payload *payload, curl_easy_setopt(curl, CURLOPT_FILETIME, 1L); curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0L); curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L); - curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, curl_progress); + curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, curl_progress_cb); curl_easy_setopt(curl, CURLOPT_PROGRESSDATA, (void *)payload); curl_easy_setopt(curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, 10L); - curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, parse_headers); + curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, curl_parseheader_cb); curl_easy_setopt(curl, CURLOPT_WRITEHEADER, (void *)payload); curl_easy_setopt(curl, CURLOPT_NETRC, CURL_NETRC_OPTIONAL); curl_easy_setopt(curl, CURLOPT_SOCKOPTFUNCTION, curl_sockopt_cb); -- 1.7.8.4
On Sun, Jan 22, 2012 at 08:31:51PM -0500, Dave Reisner wrote:
Also, fuck FTP.
relevant [1], for those who don't know it. =) cheers! mar77i [1] http://mywiki.wooledge.org/FtpMustDie
On Sun, Jan 22, 2012 at 7:31 PM, Dave Reisner <d@falconindy.com> wrote:
This is particularly important in the case of FTP control connections, which may be closed by rogue NAT/firewall devices detecting idle connections on larger transfers which may take 5-10+ minutes.
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- This is basically on the advice of Daniel Stenberg, who mentions that it's a common problem that he's aware of. I have no idea if this is actually the fix we're looking for and I'm still unable to reproduce, but it seems plausible solution.
Also, fuck FTP.
configure.ac | 7 +++-- lib/libalpm/dload.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-)
diff --git a/configure.ac b/configure.ac index 6941054..c8ba03f 100644 --- a/configure.ac +++ b/configure.ac @@ -174,9 +174,10 @@ AM_CONDITIONAL([HAVE_LIBGPGME], [test "x$with_gpgme" = "xyes"])
# Checks for header files. AC_CHECK_HEADERS([fcntl.h float.h glob.h libintl.h limits.h locale.h \ - mntent.h stddef.h string.h sys/ioctl.h sys/mount.h \ - sys/param.h sys/statvfs.h sys/time.h sys/types.h \ - sys/ucred.h syslog.h termios.h wchar.h]) + mntent.h netinet/in.h netinet/tcp.h stddef.h string.h \ + sys/ioctl.h sys/mount.h sys/param.h sys/statvfs.h \ + sys/time.h sys/types.h sys/ucred.h syslog.h termios.h \ + wchar.h])
# Checks for typedefs, structures, and compiler characteristics. AC_C_INLINE diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index a3980d6..790080c 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -23,11 +23,19 @@ #include <errno.h> #include <string.h> #include <unistd.h> +#include <sys/socket.h> /* setsockopt, SO_KEEPALIVE */ #include <sys/time.h> #include <sys/types.h> #include <sys/stat.h> #include <signal.h>
+#ifdef _HAVE_NETINET_IN_H +#include <netinet/in.h> /* IPPROTO_TCP */ +#endif +#ifdef _HAVE_NETINET_TCP_H +#include <netinet/tcp.h> /* TCP_KEEPINTVL, TCP_KEEPIDLE */ +#endif + #ifdef HAVE_LIBCURL #include <curl/curl.h> #endif @@ -215,6 +223,53 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) return realsize; }
+static int curl_sockopt_cb(void *userdata, curl_socket_t curlfd, + curlsocktype purpose) +{ + alpm_handle_t *handle = userdata; + + int onoff = 1; + +#ifdef TCP_KEEPIDLE + int keepidle = TCP_KEEPIDLE; +#endif +#ifdef TCP_KEEPINTVL + int keepintvl = TCP_KEEPINTVL; +#endif + + switch(purpose) { + case CURLSOCKTYPE_IPCXN: + if(setsockopt(curlfd, SOL_SOCKET, SO_KEEPALIVE, (void *)&onoff, + sizeof(onoff)) < 0) { + /* don't abort operation, just log to debug */ + _alpm_log(handle, ALPM_LOG_DEBUG, "Failed to set SO_KEEPALIVE on fd %d\n", + curlfd); This makes sense...
+ } + else { +#ifdef TCP_KEEPIDLE + if(setsockopt(curlfd, IPPROTO_TCP, TCP_KEEPIDLE, (void *)&keepidle, + sizeof(keepidle)) < 0) { + /* don't abort operation, just log to debug */ + _alpm_log(handle, ALPM_LOG_DEBUG, "Failed to set TCP_KEEPIDLE on fd %d\n", + curlfd); + } +#endif +#ifdef TCP_KEEPINTVL + if(setsockopt(curlfd, IPPROTO_TCP, TCP_KEEPINTVL, (void *)&keepintvl, + sizeof(keepintvl)) < 0) { + /* don't abort operation, just log to debug */ + _alpm_log(handle, ALPM_LOG_DEBUG, "Failed to set TCP_KEEPINTVL on fd %d\n", + curlfd); + } +#endif These, however, seem a bit suspect.
1) Linux-specific (although you guarded it fine, this is quite ugly). 2) Looks like the default keepidle value is 7200 secs; aka 2 hours. This is far too long, but do we need to be messing with keepintvl? That defaults to 75 seconds which seems relatively sane. 3) Why are you using TCP_KEEPIDLE and TCP_KEEPINTVL for both the option_name AND option_value? Passing a logical value rather than the arbitrary constant values (4 and 5) would seem to make more sense in any case. Probably something like 60 seconds for both would work.
+ } + default: + break; + } + + return 0; +} + static void curl_set_handle_opts(struct dload_payload *payload, CURL *curl, char *error_buffer) { @@ -239,6 +294,8 @@ static void curl_set_handle_opts(struct dload_payload *payload, curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, parse_headers); curl_easy_setopt(curl, CURLOPT_WRITEHEADER, (void *)payload); curl_easy_setopt(curl, CURLOPT_NETRC, CURL_NETRC_OPTIONAL); + curl_easy_setopt(curl, CURLOPT_SOCKOPTFUNCTION, curl_sockopt_cb); + curl_easy_setopt(curl, CURLOPT_SOCKOPTDATA, (void *)handle);
_alpm_log(handle, ALPM_LOG_DEBUG, "url: %s\n", payload->fileurl);
@@ -388,6 +445,8 @@ static int curl_download_internal(struct dload_payload *payload,
/* perform transfer */ payload->curlerr = curl_easy_perform(curl); + _alpm_log(handle, ALPM_LOG_DEBUG, "curl returned error %d from transfer\n", + payload->curlerr);
/* disconnect relationships from the curl handle for things that might go out * of scope, but could still be touched on connection teardown. This really -- 1.7.8.4
participants (3)
-
Dan McGee
-
Dave Reisner
-
Martti Kühne