Skip to content

Commit 200aace

Browse files
ethomsonethomson
authored
Merge pull request #7165 from ambv/tcp-keepalive
fix: prevent SSH timeout infinite loop and enable TCP keepalive
2 parents 16cb9c5 + 2528c88 commit 200aace

4 files changed

Lines changed: 287 additions & 8 deletions

File tree

‎ci/test.sh‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -216,18 +216,13 @@ if should_run "SSH_TESTS"; then
216216
cat >"${SSHD_DIR}/sshd_config" <<-EOF
217217
Port 2222
218218
ListenAddress 0.0.0.0
219-
Protocol 2
220219
HostKey ${SSHD_DIR}/id_${GITTEST_SSH_KEYTYPE}
221220
PidFile ${SSHD_DIR}/pid
222221
AuthorizedKeysFile ${HOME}/.ssh/authorized_keys
223222
LogLevel DEBUG
224-
RSAAuthentication yes
225223
PasswordAuthentication yes
226224
PubkeyAuthentication yes
227-
ChallengeResponseAuthentication no
228225
StrictModes no
229-
HostCertificate ${SSHD_DIR}/id_${GITTEST_SSH_KEYTYPE}.pub
230-
HostKey ${SSHD_DIR}/id_${GITTEST_SSH_KEYTYPE}
231226
# Required here as sshd will simply close connection otherwise
232227
UsePAM no
233228
EOF

‎src/libgit2/streams/socket.c‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,10 @@
2020
# include <netdb.h>
2121
# include <netinet/in.h>
2222
# include <arpa/inet.h>
23+
# include <netinet/tcp.h>
24+
# ifdef __APPLE__
25+
# include <sys/socket.h>
26+
# endif
2327
#else
2428
# include <winsock2.h>
2529
# include <ws2tcpip.h>
@@ -188,6 +192,32 @@ static int socket_connect(git_stream *stream)
188192
for (p = info; p != NULL; p = p->ai_next) {
189193
s = socket(p->ai_family, p->ai_socktype | SOCK_CLOEXEC, p->ai_protocol);
190194

195+
/* Enable TCP keepalive to detect dead connections */
196+
if (s != INVALID_SOCKET && p->ai_family == AF_INET) {
197+
int keepalive = 1;
198+
if (setsockopt(s, SOL_SOCKET, SO_KEEPALIVE,
199+
(const char *)&keepalive, sizeof(keepalive)) == 0) {
200+
#ifdef __APPLE__
201+
/* macOS: Set idle time to 60 seconds */
202+
int keepidle = 60;
203+
setsockopt(s, IPPROTO_TCP, TCP_KEEPALIVE,
204+
&keepidle, sizeof(keepidle));
205+
/* Note: TCP_CONNECTIONTIMEOUT (0x20) could also be set */
206+
#elif defined(__linux__)
207+
/* Linux: Set idle, interval, and count */
208+
int keepidle = 60; /* Start probes after 60 seconds */
209+
int keepintvl = 10; /* 10 seconds between probes */
210+
int keepcnt = 3; /* 3 probes before giving up */
211+
setsockopt(s, IPPROTO_TCP, TCP_KEEPIDLE,
212+
&keepidle, sizeof(keepidle));
213+
setsockopt(s, IPPROTO_TCP, TCP_KEEPINTVL,
214+
&keepintvl, sizeof(keepintvl));
215+
setsockopt(s, IPPROTO_TCP, TCP_KEEPCNT,
216+
&keepcnt, sizeof(keepcnt));
217+
#endif
218+
}
219+
}
220+
191221
if (s == INVALID_SOCKET)
192222
continue;
193223

‎src/libgit2/transports/ssh_libssh2.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,7 @@ static int _git_ssh_authenticate_session(
366366
default:
367367
rc = LIBSSH2_ERROR_AUTHENTICATION_FAILED;
368368
}
369-
} while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc);
369+
} while (LIBSSH2_ERROR_EAGAIN == rc);
370370

371371
if (rc == LIBSSH2_ERROR_PASSWORD_EXPIRED ||
372372
rc == LIBSSH2_ERROR_AUTHENTICATION_FAILED ||
@@ -556,7 +556,7 @@ static int _git_ssh_session_create(
556556
if (git_str_len(&prefs) > 0) {
557557
do {
558558
rc = libssh2_session_method_pref(s, LIBSSH2_METHOD_HOSTKEY, git_str_cstr(&prefs));
559-
} while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc);
559+
} while (LIBSSH2_ERROR_EAGAIN == rc);
560560
if (rc != LIBSSH2_ERROR_NONE) {
561561
ssh_error(s, "failed to set hostkey preference");
562562
goto on_error;
@@ -566,7 +566,7 @@ static int _git_ssh_session_create(
566566

567567
do {
568568
rc = libssh2_session_handshake(s, socket->s);
569-
} while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc);
569+
} while (LIBSSH2_ERROR_EAGAIN == rc);
570570

571571
if (rc != LIBSSH2_ERROR_NONE) {
572572
ssh_error(s, "failed to start SSH session");

‎tests/libgit2/online/ssh_timeout.c‎

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,254 @@
1+
#include "clar_libgit2.h"
2+
#include "git2/sys/transport.h"
3+
#include "thread.h"
4+
5+
#ifndef _WIN32
6+
# include <sys/socket.h>
7+
# include <netinet/in.h>
8+
# include <arpa/inet.h>
9+
# include <pthread.h>
10+
# include <unistd.h>
11+
# include <time.h>
12+
#else
13+
# include <winsock2.h>
14+
# include <ws2tcpip.h>
15+
#endif
16+
17+
extern int git_socket_stream__timeout;
18+
19+
#ifdef GIT_SSH_LIBSSH2
20+
21+
#ifdef _WIN32
22+
static SOCKET server_socket = INVALID_SOCKET;
23+
#else
24+
static int server_socket = -1;
25+
#endif
26+
static int server_port = 0;
27+
#ifndef _WIN32
28+
static pthread_t server_thread;
29+
#else
30+
static HANDLE server_thread;
31+
#endif
32+
static git_atomic32 server_running;
33+
34+
/* Black hole server: accepts connections but never responds */
35+
#ifdef _WIN32
36+
static DWORD WINAPI blackhole_server(LPVOID param)
37+
{
38+
SOCKET client_socket;
39+
struct sockaddr_in client_addr;
40+
int client_len = sizeof(client_addr);
41+
42+
GIT_UNUSED(param);
43+
44+
git_atomic32_set(&server_running, 1);
45+
46+
while (git_atomic32_get(&server_running)) {
47+
client_socket = accept(server_socket,
48+
(struct sockaddr *)&client_addr, &client_len);
49+
if (client_socket == INVALID_SOCKET)
50+
break;
51+
52+
/* Accept the connection but never send data - this will
53+
* cause SSH handshake to timeout */
54+
Sleep(10000); /* 10 seconds */
55+
closesocket(client_socket);
56+
}
57+
58+
return 0;
59+
}
60+
#else
61+
static void *blackhole_server(void *param)
62+
{
63+
int client_socket;
64+
struct sockaddr_in client_addr;
65+
socklen_t client_len = sizeof(client_addr);
66+
67+
GIT_UNUSED(param);
68+
69+
git_atomic32_set(&server_running, 1);
70+
71+
while (git_atomic32_get(&server_running)) {
72+
client_socket = accept(server_socket,
73+
(struct sockaddr *)&client_addr, &client_len);
74+
if (client_socket < 0)
75+
break;
76+
77+
/* Accept the connection but never send data - this will
78+
* cause SSH handshake to timeout */
79+
sleep(10); /* 10 seconds */
80+
close(client_socket);
81+
}
82+
83+
return NULL;
84+
}
85+
#endif
86+
87+
static int start_blackhole_server(void)
88+
{
89+
struct sockaddr_in addr;
90+
#ifdef _WIN32
91+
int addr_len = sizeof(addr);
92+
#else
93+
socklen_t addr_len = sizeof(addr);
94+
#endif
95+
int opt = 1;
96+
97+
#ifdef _WIN32
98+
WSADATA wsa_data;
99+
WSAStartup(MAKEWORD(2, 2), &wsa_data);
100+
#endif
101+
102+
server_socket = socket(AF_INET, SOCK_STREAM, 0);
103+
#ifdef _WIN32
104+
if (server_socket == INVALID_SOCKET)
105+
return -1;
106+
#else
107+
if (server_socket < 0)
108+
return -1;
109+
#endif
110+
111+
#ifdef _WIN32
112+
setsockopt(server_socket, SOL_SOCKET, SO_REUSEADDR,
113+
(const char *)&opt, sizeof(opt));
114+
#else
115+
setsockopt(server_socket, SOL_SOCKET, SO_REUSEADDR,
116+
&opt, sizeof(opt));
117+
#endif
118+
119+
memset(&addr, 0, sizeof(addr));
120+
addr.sin_family = AF_INET;
121+
addr.sin_addr.s_addr = inet_addr("127.0.0.1");
122+
addr.sin_port = 0; /* Let OS choose port */
123+
124+
if (bind(server_socket, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
125+
#ifdef _WIN32
126+
closesocket(server_socket);
127+
#else
128+
close(server_socket);
129+
#endif
130+
return -1;
131+
}
132+
133+
if (listen(server_socket, 5) < 0) {
134+
#ifdef _WIN32
135+
closesocket(server_socket);
136+
#else
137+
close(server_socket);
138+
#endif
139+
return -1;
140+
}
141+
142+
/* Get the actual port assigned */
143+
if (getsockname(server_socket, (struct sockaddr *)&addr, &addr_len) < 0) {
144+
#ifdef _WIN32
145+
closesocket(server_socket);
146+
#else
147+
close(server_socket);
148+
#endif
149+
return -1;
150+
}
151+
server_port = ntohs(addr.sin_port);
152+
153+
/* Start server thread */
154+
#ifdef _WIN32
155+
server_thread = CreateThread(NULL, 0, blackhole_server, NULL, 0, NULL);
156+
if (server_thread == NULL) {
157+
closesocket(server_socket);
158+
return -1;
159+
}
160+
#else
161+
if (pthread_create(&server_thread, NULL, blackhole_server, NULL) != 0) {
162+
close(server_socket);
163+
return -1;
164+
}
165+
#endif
166+
167+
return 0;
168+
}
169+
170+
static void stop_blackhole_server(void)
171+
{
172+
git_atomic32_set(&server_running, 0);
173+
174+
#ifdef _WIN32
175+
if (server_socket != INVALID_SOCKET) {
176+
closesocket(server_socket);
177+
if (server_thread)
178+
WaitForSingleObject(server_thread, INFINITE);
179+
server_socket = INVALID_SOCKET;
180+
}
181+
WSACleanup();
182+
#else
183+
if (server_socket >= 0) {
184+
close(server_socket);
185+
pthread_join(server_thread, NULL);
186+
server_socket = -1;
187+
}
188+
#endif
189+
}
190+
191+
#endif /* GIT_SSH_LIBSSH2 */
192+
193+
/*
194+
* Test that SSH connection timeout doesn't cause infinite retry loop.
195+
*
196+
* This test creates a TCP server that accepts connections but never
197+
* responds to SSH handshake, causing libssh2 to timeout.
198+
*
199+
* Before the fix: The code would retry indefinitely on LIBSSH2_ERROR_TIMEOUT
200+
* After the fix: The code properly returns an error after first timeout
201+
*/
202+
void test_online_ssh_timeout__no_infinite_loop(void)
203+
{
204+
#ifndef GIT_SSH_LIBSSH2
205+
cl_skip();
206+
#else
207+
git_remote *remote = NULL;
208+
git_repository *repo = NULL;
209+
git_transport *transport = NULL;
210+
git_remote_connect_options opts = GIT_REMOTE_CONNECT_OPTIONS_INIT;
211+
char url[256];
212+
int old_timeout;
213+
clock_t start, end;
214+
double elapsed_ms;
215+
216+
/* Start black hole server */
217+
cl_git_pass(start_blackhole_server());
218+
219+
/* Create URL to our black hole server */
220+
sprintf(url, "ssh://localhost:%d/test.git", server_port);
221+
222+
/* Set a short timeout (100ms) */
223+
old_timeout = git_socket_stream__timeout;
224+
git_socket_stream__timeout = 100;
225+
226+
cl_git_pass(git_repository_init(&repo, "./transport-timeout", 0));
227+
cl_git_pass(git_remote_create(&remote, repo, "test", url));
228+
229+
/* Get transport */
230+
cl_git_pass(git_transport_new(&transport, remote, url));
231+
232+
/* Attempt connection - should fail due to timeout */
233+
start = clock();
234+
cl_git_fail(transport->connect(transport, url,
235+
GIT_SERVICE_UPLOADPACK_LS, &opts));
236+
end = clock();
237+
238+
/* Calculate elapsed time in milliseconds */
239+
elapsed_ms = ((double)(end - start) / CLOCKS_PER_SEC) * 1000.0;
240+
241+
/* With the fix, this should fail relatively quickly (within 2 seconds).
242+
* Without the fix, it would loop many times and take much longer.
243+
* We use a generous timeout of 5 seconds to avoid flakiness. */
244+
cl_assert(elapsed_ms < 5000);
245+
246+
/* Cleanup */
247+
transport->free(transport);
248+
git_remote_free(remote);
249+
git_repository_free(repo);
250+
git_socket_stream__timeout = old_timeout;
251+
252+
stop_blackhole_server();
253+
#endif
254+
}

0 commit comments

Comments
 (0)