From fad90428970e332153027773b517a1606c0efa1f Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Fri, 12 May 2023 20:48:30 +0100 Subject: streams: sockets are non-blocking and can timeout Make socket I/O non-blocking and add optional timeouts. Users may now set `GIT_OPT_SET_SERVER_CONNECT_TIMEOUT` to set a shorter connection timeout. (The connect timeout cannot be longer than the operating system default.) Users may also now configure the socket read and write timeouts with `GIT_OPT_SET_SERVER_TIMEOUT`. By default, connects still timeout based on the operating system defaults (typically 75 seconds) and socket read and writes block. Add a test against our custom testing git server that ensures that we can timeout reads against a slow server. --- ci/test.sh | 4 + include/git2/common.h | 27 +++++- include/git2/errors.h | 3 +- include/git2/sys/stream.h | 18 +++- src/libgit2/libgit2.c | 36 +++++++ src/libgit2/streams/socket.c | 198 ++++++++++++++++++++++++++++++++++----- src/libgit2/streams/stransport.c | 17 +++- tests/libgit2/CMakeLists.txt | 2 +- tests/libgit2/online/clone.c | 60 ++++++++++++ 9 files changed, 335 insertions(+), 30 deletions(-) diff --git a/ci/test.sh b/ci/test.sh index a6735231f..ee6801a79 100755 --- a/ci/test.sh +++ b/ci/test.sh @@ -271,9 +271,13 @@ if [ -z "$SKIP_ONLINE_TESTS" ]; then export GITTEST_REMOTE_REDIRECT_INITIAL="http://localhost:9000/initial-redirect/libgit2/TestGitRepository" export GITTEST_REMOTE_REDIRECT_SUBSEQUENT="http://localhost:9000/subsequent-redirect/libgit2/TestGitRepository" + export GITTEST_REMOTE_SPEED_SLOW="http://localhost:9000/speed-9600/test.git" + export GITTEST_REMOTE_SPEED_TIMESOUT="http://localhost:9000/speed-0.5/test.git" run_test online unset GITTEST_REMOTE_REDIRECT_INITIAL unset GITTEST_REMOTE_REDIRECT_SUBSEQUENT + unset GITTEST_REMOTE_SPEED_SLOW + unset GITTEST_REMOTE_SPEED_TIMESOUT # Run the online tests that immutably change global state separately # to avoid polluting the test environment. diff --git a/include/git2/common.h b/include/git2/common.h index f968deb23..ab6bc1333 100644 --- a/include/git2/common.h +++ b/include/git2/common.h @@ -224,7 +224,11 @@ typedef enum { GIT_OPT_GET_OWNER_VALIDATION, GIT_OPT_SET_OWNER_VALIDATION, GIT_OPT_GET_HOMEDIR, - GIT_OPT_SET_HOMEDIR + GIT_OPT_SET_HOMEDIR, + GIT_OPT_SET_SERVER_CONNECT_TIMEOUT, + GIT_OPT_GET_SERVER_CONNECT_TIMEOUT, + GIT_OPT_SET_SERVER_TIMEOUT, + GIT_OPT_GET_SERVER_TIMEOUT } git_libgit2_opt_t; /** @@ -480,6 +484,27 @@ typedef enum { * > * > - `path` directory of home directory. * + * opts(GIT_OPT_GET_SERVER_CONNECT_TIMEOUT, int *timeout) + * > Gets the timeout (in milliseconds) to attempt connections to + * > a remote server. + * + * opts(GIT_OPT_SET_SERVER_CONNECT_TIMEOUT, int timeout) + * > Sets the timeout (in milliseconds) to attempt connections to + * > a remote server. This is supported only for HTTP(S) connections + * > and is not supported by SSH. Set to 0 to use the system default. + * > Note that this may not be able to be configured longer than the + * > system default, typically 75 seconds. + * + * opts(GIT_OPT_GET_SERVER_TIMEOUT, int *timeout) + * > Gets the timeout (in milliseconds) for reading from and writing + * > to a remote server. + * + * opts(GIT_OPT_SET_SERVER_TIMEOUT, int timeout) + * > Sets the timeout (in milliseconds) for reading from and writing + * > to a remote server. This is supported only for HTTP(S) + * > connections and is not supported by SSH. Set to 0 to use the + * > system default. + * * @param option Option key * @param ... value to set the option * @return 0 on success, <0 on failure diff --git a/include/git2/errors.h b/include/git2/errors.h index e634a97c1..7180852f9 100644 --- a/include/git2/errors.h +++ b/include/git2/errors.h @@ -58,7 +58,8 @@ typedef enum { GIT_EMISMATCH = -33, /**< Hashsum mismatch in object */ GIT_EINDEXDIRTY = -34, /**< Unsaved changes in the index would be overwritten */ GIT_EAPPLYFAIL = -35, /**< Patch application failed */ - GIT_EOWNER = -36 /**< The object is not owned by the current user */ + GIT_EOWNER = -36, /**< The object is not owned by the current user */ + GIT_TIMEOUT = -37 /**< The operation timed out */ } git_error_code; /** diff --git a/include/git2/sys/stream.h b/include/git2/sys/stream.h index e0e03a2d7..3d28d09b3 100644 --- a/include/git2/sys/stream.h +++ b/include/git2/sys/stream.h @@ -29,8 +29,22 @@ GIT_BEGIN_DECL typedef struct git_stream { int version; - int encrypted; - int proxy_support; + int encrypted : 1, + proxy_support : 1; + + /** + * Timeout for read and write operations; can be set to `0` to + * block indefinitely. + */ + int timeout; + + /** + * Timeout to connect to the remote server; can be set to `0` + * to use the system defaults. This can be shorter than the + * system default - often 75 seconds - but cannot be longer. + */ + int connect_timeout; + int GIT_CALLBACK(connect)(struct git_stream *); int GIT_CALLBACK(certificate)(git_cert **, struct git_stream *); int GIT_CALLBACK(set_proxy)(struct git_stream *, const git_proxy_options *proxy_opts); diff --git a/src/libgit2/libgit2.c b/src/libgit2/libgit2.c index 178880c9e..ce287147a 100644 --- a/src/libgit2/libgit2.c +++ b/src/libgit2/libgit2.c @@ -48,6 +48,8 @@ extern size_t git_indexer__max_objects; extern bool git_disable_pack_keep_file_checks; extern int git_odb__packed_priority; extern int git_odb__loose_priority; +extern int git_socket_stream__connect_timeout; +extern int git_socket_stream__timeout; char *git__user_agent; char *git__ssl_ciphers; @@ -436,6 +438,40 @@ int git_libgit2_opts(int key, ...) error = git_sysdir_set(GIT_SYSDIR_HOME, va_arg(ap, const char *)); break; + case GIT_OPT_GET_SERVER_CONNECT_TIMEOUT: + *(va_arg(ap, int *)) = git_socket_stream__connect_timeout; + break; + + case GIT_OPT_SET_SERVER_CONNECT_TIMEOUT: + { + int timeout = va_arg(ap, int); + + if (timeout < 0) { + git_error_set(GIT_ERROR_INVALID, "invalid connect timeout"); + error = -1; + } else { + git_socket_stream__connect_timeout = timeout; + } + } + break; + + case GIT_OPT_GET_SERVER_TIMEOUT: + *(va_arg(ap, int *)) = git_socket_stream__timeout; + break; + + case GIT_OPT_SET_SERVER_TIMEOUT: + { + int timeout = va_arg(ap, int); + + if (timeout < 0) { + git_error_set(GIT_ERROR_INVALID, "invalid timeout"); + error = -1; + } else { + git_socket_stream__timeout = timeout; + } + } + break; + default: git_error_set(GIT_ERROR_INVALID, "invalid option key"); error = -1; diff --git a/src/libgit2/streams/socket.c b/src/libgit2/streams/socket.c index 6994d58f2..0e0aa6934 100644 --- a/src/libgit2/streams/socket.c +++ b/src/libgit2/streams/socket.c @@ -20,6 +20,7 @@ # include # include # include +# include #else # include # include @@ -28,6 +29,9 @@ # endif #endif +int git_socket_stream__connect_timeout = 0; +int git_socket_stream__timeout = 0; + #ifdef GIT_WIN32 static void net_set_error(const char *str) { @@ -66,21 +70,105 @@ static int close_socket(GIT_SOCKET s) } +static int set_nonblocking(GIT_SOCKET s) +{ +#ifdef GIT_WIN32 + unsigned long nonblocking = 1; + + if (ioctlsocket(s, FIONBIO, &nonblocking) != 0) { + net_set_error("could not set socket non-blocking"); + return -1; + } +#else + int flags; + + if ((flags = fcntl(s, F_GETFL, 0)) == -1) { + net_set_error("could not query socket flags"); + return -1; + } + + flags |= O_NONBLOCK; + + if (fcntl(s, F_SETFL, flags) != 0) { + net_set_error("could not set socket non-blocking"); + return -1; + } +#endif + + return 0; +} + +/* Promote a sockerr to an errno for our error handling routines */ +static int handle_sockerr(GIT_SOCKET socket) +{ + int sockerr; + socklen_t errlen = sizeof(sockerr); + + if (getsockopt(socket, SOL_SOCKET, SO_ERROR, &sockerr, &errlen) < 0) + return -1; + + if (sockerr == ETIMEDOUT) + return GIT_TIMEOUT; + + errno = sockerr; + return -1; +} + +static int connect_with_timeout( + GIT_SOCKET socket, + const struct sockaddr *address, + socklen_t address_len, + int timeout) +{ + struct pollfd fd; + int error; + + if (timeout && (error = set_nonblocking(socket)) < 0) + return error; + + error = connect(socket, address, address_len); + + if (error == 0 || (error == -1 && errno != EINPROGRESS)) + return error; + + fd.fd = socket; + fd.events = POLLOUT; + fd.revents = 0; + + error = poll(&fd, 1, timeout); + + if (error == 0) { + return GIT_TIMEOUT; + } else if (error != 1) { + return -1; + } else if ((fd.revents & (POLLHUP | POLLERR))) { + return handle_sockerr(socket); + } else if ((fd.revents & POLLOUT) != POLLOUT) { + git_error_set(GIT_ERROR_NET, + "unknown error while polling for connect: %d", + fd.revents); + return -1; + } + + return 0; +} + static int socket_connect(git_stream *stream) { - struct addrinfo *info = NULL, *p; - struct addrinfo hints; git_socket_stream *st = (git_socket_stream *) stream; GIT_SOCKET s = INVALID_SOCKET; - int ret; + struct addrinfo *info = NULL, *p; + struct addrinfo hints; + int error; memset(&hints, 0x0, sizeof(struct addrinfo)); hints.ai_socktype = SOCK_STREAM; hints.ai_family = AF_UNSPEC; - if ((ret = p_getaddrinfo(st->host, st->port, &hints, &info)) != 0) { + if ((error = p_getaddrinfo(st->host, st->port, &hints, &info)) != 0) { git_error_set(GIT_ERROR_NET, - "failed to resolve address for %s: %s", st->host, p_gai_strerror(ret)); + "failed to resolve address for %s: %s", + st->host, p_gai_strerror(error)); return -1; } @@ -90,51 +178,115 @@ static int socket_connect(git_stream *stream) if (s == INVALID_SOCKET) continue; - if (connect(s, p->ai_addr, (socklen_t)p->ai_addrlen) == 0) + error = connect_with_timeout(s, p->ai_addr, + (socklen_t)p->ai_addrlen, + st->parent.connect_timeout); + + if (error == 0) break; /* If we can't connect, try the next one */ close_socket(s); s = INVALID_SOCKET; + + if (error == GIT_TIMEOUT) + break; } /* Oops, we couldn't connect to any address */ - if (s == INVALID_SOCKET && p == NULL) { - git_error_set(GIT_ERROR_OS, "failed to connect to %s", st->host); - p_freeaddrinfo(info); - return -1; + if (s == INVALID_SOCKET) { + if (error == GIT_TIMEOUT) + git_error_set(GIT_ERROR_NET, "failed to connect to %s: Operation timed out", st->host); + else + git_error_set(GIT_ERROR_OS, "failed to connect to %s", st->host); + error = -1; + goto done; } + if (st->parent.timeout && !st->parent.connect_timeout && + (error = set_nonblocking(s)) < 0) + return error; + st->s = s; + error = 0; + +done: p_freeaddrinfo(info); - return 0; + return error; } -static ssize_t socket_write(git_stream *stream, const char *data, size_t len, int flags) +static ssize_t socket_write( + git_stream *stream, + const char *data, + size_t len, + int flags) { git_socket_stream *st = (git_socket_stream *) stream; - ssize_t written; + struct pollfd fd; + ssize_t ret; GIT_ASSERT(flags == 0); GIT_UNUSED(flags); - errno = 0; + ret = p_send(st->s, data, len, 0); + + if (st->parent.timeout && ret < 0 && + (errno == EAGAIN || errno != EWOULDBLOCK)) { + fd.fd = st->s; + fd.events = POLLOUT; + fd.revents = 0; + + ret = poll(&fd, 1, st->parent.timeout); - if ((written = p_send(st->s, data, len, 0)) < 0) { - net_set_error("error sending data"); + if (ret == 1) { + ret = p_send(st->s, data, len, 0); + } else if (ret == 0) { + git_error_set(GIT_ERROR_NET, + "could not write to socket: timed out"); + return GIT_TIMEOUT; + } + } + + if (ret < 0) { + net_set_error("error receiving data from socket"); return -1; } - return written; + return ret; } -static ssize_t socket_read(git_stream *stream, void *data, size_t len) +static ssize_t socket_read( + git_stream *stream, + void *data, + size_t len) { - ssize_t ret; git_socket_stream *st = (git_socket_stream *) stream; + struct pollfd fd; + ssize_t ret; + + ret = p_recv(st->s, data, len, 0); + + if (st->parent.timeout && ret < 0 && + (errno == EAGAIN || errno != EWOULDBLOCK)) { + fd.fd = st->s; + fd.events = POLLIN; + fd.revents = 0; - if ((ret = p_recv(st->s, data, len, 0)) < 0) - net_set_error("error receiving socket data"); + ret = poll(&fd, 1, st->parent.timeout); + + if (ret == 1) { + ret = p_recv(st->s, data, len, 0); + } else if (ret == 0) { + git_error_set(GIT_ERROR_NET, + "could not read from socket: timed out"); + return GIT_TIMEOUT; + } + } + + if (ret < 0) { + net_set_error("error receiving data from socket"); + return -1; + } return ret; } @@ -182,6 +334,8 @@ static int default_socket_stream_new( } st->parent.version = GIT_STREAM_VERSION; + st->parent.timeout = git_socket_stream__timeout; + st->parent.connect_timeout = git_socket_stream__connect_timeout; st->parent.connect = socket_connect; st->parent.write = socket_write; st->parent.read = socket_read; @@ -248,7 +402,7 @@ int git_socket_stream_global_init(void) return git_runtime_shutdown_register(socket_stream_global_shutdown); } - + #else #include "stream.h" diff --git a/src/libgit2/streams/stransport.c b/src/libgit2/streams/stransport.c index 74ee0d1ee..d956df84d 100644 --- a/src/libgit2/streams/stransport.c +++ b/src/libgit2/streams/stransport.c @@ -161,7 +161,9 @@ static OSStatus write_cb(SSLConnectionRef conn, const void *data, size_t *len) if (ret < 0) { st->error = ret; - return -36; /* ioErr */ + return (ret == GIT_TIMEOUT) ? + errSSLNetworkTimeout : + -36 /* ioErr */; } return noErr; @@ -176,8 +178,12 @@ static ssize_t stransport_write(git_stream *stream, const char *data, size_t len GIT_UNUSED(flags); data_len = min(len, SSIZE_MAX); - if ((ret = SSLWrite(st->ctx, data, data_len, &processed)) != noErr) + if ((ret = SSLWrite(st->ctx, data, data_len, &processed)) != noErr) { + if (st->error == GIT_TIMEOUT) + return GIT_TIMEOUT; + return stransport_error(ret); + } GIT_ASSERT(processed < SSIZE_MAX); return (ssize_t)processed; @@ -207,7 +213,9 @@ static OSStatus read_cb(SSLConnectionRef conn, void *data, size_t *len) if (ret < 0) { st->error = ret; - error = -36; /* ioErr */ + error = (ret == GIT_TIMEOUT) ? + errSSLNetworkTimeout : + -36 /* ioErr */; break; } else if (ret == 0) { error = errSSLClosedGraceful; @@ -228,6 +236,9 @@ static ssize_t stransport_read(git_stream *stream, void *data, size_t len) OSStatus ret; if ((ret = SSLRead(st->ctx, data, len, &processed)) != noErr) { + if (st->error == GIT_TIMEOUT) + return GIT_TIMEOUT; + return stransport_error(ret); } diff --git a/tests/libgit2/CMakeLists.txt b/tests/libgit2/CMakeLists.txt index 866122d1c..af70f55a7 100644 --- a/tests/libgit2/CMakeLists.txt +++ b/tests/libgit2/CMakeLists.txt @@ -65,7 +65,7 @@ endif() include(AddClarTest) add_clar_test(libgit2_tests offline -v -xonline) -add_clar_test(libgit2_tests invasive -v -sfilter::stream::bigfile -sodb::largefiles -siterator::workdir::filesystem_gunk -srepo::init -srepo::init::at_filesystem_root) +add_clar_test(libgit2_tests invasive -v -sfilter::stream::bigfile -sodb::largefiles -siterator::workdir::filesystem_gunk -srepo::init -srepo::init::at_filesystem_root -sonline::clone::connect_timeout_default) add_clar_test(libgit2_tests online -v -sonline -xonline::customcert) add_clar_test(libgit2_tests online_customcert -v -sonline::customcert) add_clar_test(libgit2_tests gitdaemon -v -sonline::push) diff --git a/tests/libgit2/online/clone.c b/tests/libgit2/online/clone.c index b635739b6..92fa4d351 100644 --- a/tests/libgit2/online/clone.c +++ b/tests/libgit2/online/clone.c @@ -35,6 +35,8 @@ static char *_remote_proxy_selfsigned = NULL; static char *_remote_expectcontinue = NULL; static char *_remote_redirect_initial = NULL; static char *_remote_redirect_subsequent = NULL; +static char *_remote_speed_timesout = NULL; +static char *_remote_speed_slow = NULL; static char *_github_ssh_pubkey = NULL; static char *_github_ssh_privkey = NULL; @@ -89,6 +91,8 @@ void test_online_clone__initialize(void) _remote_expectcontinue = cl_getenv("GITTEST_REMOTE_EXPECTCONTINUE"); _remote_redirect_initial = cl_getenv("GITTEST_REMOTE_REDIRECT_INITIAL"); _remote_redirect_subsequent = cl_getenv("GITTEST_REMOTE_REDIRECT_SUBSEQUENT"); + _remote_speed_timesout = cl_getenv("GITTEST_REMOTE_SPEED_TIMESOUT"); + _remote_speed_slow = cl_getenv("GITTEST_REMOTE_SPEED_SLOW"); _github_ssh_pubkey = cl_getenv("GITTEST_GITHUB_SSH_PUBKEY"); _github_ssh_privkey = cl_getenv("GITTEST_GITHUB_SSH_KEY"); @@ -128,6 +132,8 @@ void test_online_clone__cleanup(void) git__free(_remote_expectcontinue); git__free(_remote_redirect_initial); git__free(_remote_redirect_subsequent); + git__free(_remote_speed_timesout); + git__free(_remote_speed_slow); git__free(_github_ssh_pubkey); git__free(_github_ssh_privkey); @@ -145,6 +151,8 @@ void test_online_clone__cleanup(void) } git_libgit2_opts(GIT_OPT_SET_SSL_CERT_LOCATIONS, NULL, NULL); + git_libgit2_opts(GIT_OPT_SET_SERVER_TIMEOUT, 0); + git_libgit2_opts(GIT_OPT_SET_SERVER_CONNECT_TIMEOUT, 0); } void test_online_clone__network_full(void) @@ -1207,3 +1215,55 @@ void test_online_clone__sha256(void) git_reference_free(head); #endif } + +void test_online_clone__connect_timeout_configurable(void) +{ + uint64_t start, finish; + + start = git_time_monotonic(); + + cl_git_pass(git_libgit2_opts(GIT_OPT_SET_SERVER_CONNECT_TIMEOUT, 1)); + cl_git_fail(git_clone(&g_repo, "http://www.google.com:8000/", "./timedout", NULL)); + cl_assert(git_error_last() && strstr(git_error_last()->message, "timed out")); + + finish = git_time_monotonic(); + + cl_assert(finish - start < 1000); +} + +void test_online_clone__connect_timeout_default(void) +{ + /* This test takes ~ 75 seconds on Unix. */ + if (!cl_is_env_set("GITTEST_INVASIVE_SPEED")) + cl_skip(); + + /* + * Use a host/port pair that blackholes packets and does not + * send an RST. + */ + cl_git_fail_with(GIT_TIMEOUT, git_clone(&g_repo, "http://www.google.com:8000/", "./refused", NULL)); + cl_assert(git_error_last() && strstr(git_error_last()->message, "timed out")); +} + +void test_online_clone__timeout_configurable_times_out(void) +{ + git_repository *failed_repo; + + if (!_remote_speed_timesout) + cl_skip(); + + cl_git_pass(git_libgit2_opts(GIT_OPT_SET_SERVER_TIMEOUT, 1000)); + + cl_git_fail_with(GIT_TIMEOUT, git_clone(&failed_repo, _remote_speed_timesout, "./timedout", NULL)); + cl_assert(git_error_last() && strstr(git_error_last()->message, "timed out")); +} + +void test_online_clone__timeout_configurable_succeeds_slowly(void) +{ + if (!_remote_speed_slow) + cl_skip(); + + cl_git_pass(git_libgit2_opts(GIT_OPT_SET_SERVER_TIMEOUT, 1000)); + + cl_git_pass(git_clone(&g_repo, _remote_speed_slow, "./slow-but-successful", NULL)); +} -- cgit v1.2.1