Skip to content

Commit 074e642

Browse files
Sean-DerSean-Der
committed
Fix TCP error handling in conn_poll
Don't kill the entire connection because TCP has errored
1 parent f007d8c commit 074e642

File tree

9 files changed

+115
-21
lines changed

9 files changed

+115
-21
lines changed

‎src/agent.c‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -866,11 +866,11 @@ void agent_register_entry_for_candidate_pair(juice_agent_t *agent, ice_candidate
866866
agent_translate_host_candidate_entry(agent, entry);
867867
}
868868

869-
void agent_tcp_conn_connected(juice_agent_t *agent) {
869+
void agent_tcp_conn_connected(juice_agent_t *agent, bool success) {
870870
for (int i = 0; i < agent->entries_count; ++i) {
871871
agent_stun_entry_t *entry = agent->entries + i;
872872
if (entry->pair->remote->transport != ICE_CANDIDATE_TRANSPORT_UDP) {
873-
entry->pair->tcp_connected = true;
873+
entry->pair->tcp_connect_called = true;
874874
entry->next_transmission = current_timestamp();
875875
}
876876
}
@@ -889,9 +889,21 @@ int agent_bookkeeping(juice_agent_t *agent, timestamp_t *next_timestamp) {
889889
for (int i = 0; i < agent->entries_count; ++i) {
890890
agent_stun_entry_t *entry = agent->entries + i;
891891

892-
if (entry->pair && entry->pair->remote->transport != ICE_CANDIDATE_TRANSPORT_UDP && entry->pair->tcp_connected == false) {
892+
if (entry->pair && entry->pair->remote->transport != ICE_CANDIDATE_TRANSPORT_UDP && entry->pair->tcp_connect_called == false) {
893+
if (entry->next_transmission > now) {
894+
continue;
895+
} else if (entry->retransmissions <= 0) {
896+
JLOG_DEBUG("ice-tcp failed to connected, marking as failed");
897+
entry->pair->state = ICE_CANDIDATE_PAIR_STATE_FAILED;
898+
entry->state = AGENT_STUN_ENTRY_STATE_FAILED;
899+
entry->next_transmission = 0;
900+
continue;
901+
}
902+
893903
conn_tcp_connect(agent, &entry->pair->remote->resolved, agent_tcp_conn_connected);
894904
entry->next_transmission = now + LAST_STUN_RETRANSMISSION_TIMEOUT;
905+
entry->retransmissions--;
906+
895907
} else if (entry->state == AGENT_STUN_ENTRY_STATE_PENDING) { // STUN requests transmission or retransmission
896908
if (entry->next_transmission > now)
897909
continue;

‎src/conn.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ int conn_send(juice_agent_t *agent, const addr_record_t *dst, const char *data,
253253
return get_agent_mode_entry(agent)->send_func(agent, dst, data, size, ds);
254254
}
255255

256-
void conn_tcp_connect(juice_agent_t *agent, const addr_record_t *dst, void (*callback)(juice_agent_t*)) {
256+
void conn_tcp_connect(juice_agent_t *agent, const addr_record_t *dst, void (*callback)(juice_agent_t*, bool)) {
257257
if (!agent->conn_impl)
258258
return;
259259

‎src/conn.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ typedef struct conn_registry {
3232
int agents_count;
3333
} conn_registry_t;
3434

35-
typedef void tcp_connect_func(juice_agent_t *agent, const addr_record_t *dst, void (*callback)(juice_agent_t*));
35+
typedef void tcp_connect_func(juice_agent_t *agent, const addr_record_t *dst, void (*callback)(juice_agent_t*, bool));
3636

3737
typedef struct conn_mode_entry {
3838
int (*registry_init_func)(conn_registry_t *registry, udp_socket_config_t *config);
@@ -64,7 +64,7 @@ void conn_unlock(juice_agent_t *agent);
6464
int conn_interrupt(juice_agent_t *agent);
6565
int conn_send(juice_agent_t *agent, const addr_record_t *dst, const char *data, size_t size,
6666
int ds);
67-
void conn_tcp_connect(juice_agent_t *agent, const addr_record_t *dst, void (*callback)(juice_agent_t*));
67+
void conn_tcp_connect(juice_agent_t *agent, const addr_record_t *dst, void (*callback)(juice_agent_t*, bool));
6868
int conn_get_addrs(juice_agent_t *agent, addr_record_t *records, size_t size);
6969

7070
#endif

‎src/conn_poll.c‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ typedef struct conn_impl {
3636
conn_state_t state;
3737
socket_t udp_sock;
3838
socket_t tcp_sock;
39-
void (*tcp_sock_connected)(juice_agent_t*);
39+
void (*tcp_sock_callback)(juice_agent_t*, bool);
4040
addr_record_t tcp_dst;
4141
uint16_t ice_tcp_len;
4242
mutex_t send_mutex;
@@ -200,7 +200,7 @@ int conn_poll_prepare(conn_registry_t *registry, pfds_record_t *pfds, timestamp_
200200
if (conn_impl->tcp_sock != INVALID_SOCKET) {
201201
pfds->pfds[i].fd = conn_impl->tcp_sock;
202202
pfds->pfds[i].events = POLLIN;
203-
if (conn_impl->tcp_sock_connected != NULL) {
203+
if (conn_impl->tcp_sock_callback != NULL) {
204204
pfds->pfds[i].events |= POLLOUT;
205205
}
206206
i++;
@@ -284,21 +284,28 @@ void conn_poll_process_udp(juice_agent_t *agent, conn_impl_t *conn_impl, struct
284284

285285
}
286286

287+
void conn_poll_call_tcp_callback(juice_agent_t *agent, conn_impl_t *conn_impl, bool status) {
288+
if (conn_impl->tcp_sock_callback) {
289+
conn_impl->tcp_sock_callback(agent, status);
290+
conn_impl->tcp_sock_callback = NULL;
291+
}
292+
}
293+
287294
void conn_poll_process_tcp(juice_agent_t *agent, conn_impl_t *conn_impl, struct pollfd *pfd) {
288295
if (pfd->revents & POLLNVAL) {
289296
JLOG_WARN("Error when polling socket");
290297
return;
291298
}
292299

293300
if (pfd->revents & POLLHUP || pfd->revents & POLLERR) {
294-
agent_conn_fail(agent);
295-
conn_impl->state = CONN_STATE_FINISHED;
301+
JLOG_WARN("ice-tcp poll returned error, closing socket");
302+
closesocket(conn_impl->tcp_sock);
303+
conn_impl->tcp_sock = INVALID_SOCKET;
296304
return;
297305
}
298306

299-
if (pfd->revents & POLLOUT && conn_impl->tcp_sock_connected) {
300-
conn_impl->tcp_sock_connected(agent);
301-
conn_impl->tcp_sock_connected = NULL;
307+
if (pfd->revents & POLLOUT && conn_impl->tcp_sock_callback) {
308+
conn_poll_call_tcp_callback(agent, conn_impl, true);
302309
}
303310

304311
if (pfd->revents & POLLIN) {
@@ -321,8 +328,9 @@ void conn_poll_process_tcp(juice_agent_t *agent, conn_impl_t *conn_impl, struct
321328
return;
322329

323330
if (ret < 0) {
324-
agent_conn_fail(agent);
325-
conn_impl->state = CONN_STATE_FINISHED;
331+
JLOG_WARN("ice-tcp read returned error, closing socket");
332+
closesocket(conn_impl->tcp_sock);
333+
conn_impl->tcp_sock = INVALID_SOCKET;
326334
return;
327335
}
328336

@@ -447,7 +455,7 @@ int conn_poll_init(juice_agent_t *agent, conn_registry_t *registry, udp_socket_c
447455
mutex_init(&conn_impl->send_mutex, 0);
448456
conn_impl->registry = registry;
449457
conn_impl->tcp_sock = INVALID_SOCKET;
450-
conn_impl->tcp_sock_connected = NULL;
458+
conn_impl->tcp_sock_callback = NULL;
451459

452460
agent->conn_impl = conn_impl;
453461
return 0;
@@ -542,15 +550,15 @@ int conn_poll_send(juice_agent_t *agent, const addr_record_t *dst, const char *d
542550
return ret;
543551
}
544552

545-
void conn_poll_tcp_connect_func(juice_agent_t *agent, const addr_record_t *dst, void (*callback)(juice_agent_t *)) {
553+
void conn_poll_tcp_connect_func(juice_agent_t *agent, const addr_record_t *dst, void (*callback)(juice_agent_t *, bool)) {
546554
conn_impl_t *conn_impl = agent->conn_impl;
547555

548556
mutex_lock(&conn_impl->registry->mutex);
549557
mutex_lock(&conn_impl->send_mutex);
550-
if (conn_impl->tcp_sock == INVALID_SOCKET) {
558+
if (conn_impl->tcp_sock == INVALID_SOCKET && conn_impl->tcp_sock_callback == NULL) {
551559
conn_impl->tcp_sock = tcp_create_socket(dst);
552560
memcpy(&conn_impl->tcp_dst, dst, sizeof(conn_impl->tcp_dst));
553-
conn_impl->tcp_sock_connected = callback;
561+
conn_impl->tcp_sock_callback = callback;
554562
}
555563
mutex_unlock(&conn_impl->send_mutex);
556564
mutex_unlock(&conn_impl->registry->mutex);

‎src/conn_poll.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ void conn_poll_unlock(juice_agent_t *agent);
2727
int conn_poll_interrupt(juice_agent_t *agent);
2828
int conn_poll_send(juice_agent_t *agent, const addr_record_t *dst, const char *data, size_t size,
2929
int ds);
30-
void conn_poll_tcp_connect_func(juice_agent_t *agent, const addr_record_t *dst, void (*callback)(juice_agent_t *)) ;
30+
void conn_poll_tcp_connect_func(juice_agent_t *agent, const addr_record_t *dst, void (*callback)(juice_agent_t *, bool)) ;
3131
int conn_poll_get_addrs(juice_agent_t *agent, addr_record_t *records, size_t size);
3232

3333
#endif

‎src/ice.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ typedef struct ice_candidate_pair {
7676
ice_candidate_pair_state_t state;
7777
bool nominated;
7878
bool nomination_requested;
79-
bool tcp_connected;
79+
bool tcp_connect_called;
8080
timestamp_t consent_expiry;
8181
} ice_candidate_pair_t;
8282

‎src/udp.c‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,7 @@ socket_t udp_create_socket(const udp_socket_config_t *config) {
179179
int udp_recvfrom(socket_t sock, char *buffer, size_t size, addr_record_t *src) {
180180
while (true) {
181181
src->len = sizeof(src->addr);
182+
src->socktype = SOCK_DGRAM;
182183
int len =
183184
recvfrom(sock, buffer, (socklen_t)size, 0, (struct sockaddr *)&src->addr, &src->len);
184185
if (len >= 0) {

‎test/main.c‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ int test_stun_unhandled_multiple(void);
2727
int test_stun_unhandled_no_host(void);
2828
int test_stun_unhandled_unhandle(void);
2929
int test_tcp(void);
30+
int test_tcp_bad_candidate(void);
3031

3132
#ifndef NO_SERVER
3233
int test_server(void);
@@ -115,6 +116,13 @@ int main(int argc, char **argv) {
115116
return -2;
116117
}
117118

119+
printf("\nRunning TCP Bad Candidate test...\n");
120+
if (test_tcp_bad_candidate()) {
121+
fprintf(stderr, "TCP bad candidate test failed\n");
122+
return -2;
123+
}
124+
125+
118126
#ifndef _WIN32
119127
// windows fails to read STUN message from listen socket:
120128
// udp.c:196: Ignoring ECONNRESET returned by recvfrom

‎test/tcp.c‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,3 +171,68 @@ int test_tcp() {
171171
}
172172
}
173173

174+
static juice_agent_t *agent1;
175+
static juice_agent_t *agent2;
176+
177+
static void on_candidate_bad_tcp_1(juice_agent_t *agent, const char *sdp, void *user_ptr) {
178+
juice_add_remote_candidate(agent2, sdp);
179+
}
180+
181+
static void on_candidate_bad_tcp_2(juice_agent_t *agent, const char *sdp, void *user_ptr) {
182+
juice_add_remote_candidate(agent1, sdp);
183+
}
184+
185+
int test_tcp_bad_candidate() {
186+
juice_set_log_level(JUICE_LOG_LEVEL_DEBUG);
187+
188+
juice_config_t config1;
189+
memset(&config1, 0, sizeof(config1));
190+
191+
config1.cb_candidate = on_candidate_bad_tcp_1;
192+
config1.user_ptr = NULL;
193+
194+
agent1 = juice_create(&config1);
195+
196+
juice_config_t config2;
197+
memset(&config2, 0, sizeof(config2));
198+
199+
config1.cb_candidate = on_candidate_bad_tcp_2;
200+
config2.user_ptr = NULL;
201+
202+
agent2 = juice_create(&config2);
203+
204+
if (juice_set_ice_tcp_mode(agent1, JUICE_ICE_TCP_MODE_ACTIVE) || juice_set_ice_tcp_mode(agent2, JUICE_ICE_TCP_MODE_ACTIVE)) {
205+
printf("Failure\n");
206+
return -1;
207+
}
208+
209+
char sdp1[JUICE_MAX_SDP_STRING_LEN];
210+
juice_get_local_description(agent1, sdp1, JUICE_MAX_SDP_STRING_LEN);
211+
juice_set_remote_description(agent2, sdp1);
212+
213+
char sdp2[JUICE_MAX_SDP_STRING_LEN];
214+
juice_get_local_description(agent2, sdp2, JUICE_MAX_SDP_STRING_LEN);
215+
juice_set_remote_description(agent1, sdp2);
216+
217+
// Candidate that doesn't exist
218+
juice_add_remote_candidate(agent1, "a=candidate:1 1 TCP 2122316799 127.0.0.1 1337 typ host tcptype passive");
219+
220+
juice_gather_candidates(agent1);
221+
sleep(2);
222+
223+
juice_gather_candidates(agent2);
224+
sleep(2);
225+
226+
bool success = (juice_get_state(agent1) == JUICE_STATE_COMPLETED && juice_get_state(agent2) == JUICE_STATE_COMPLETED);
227+
228+
juice_destroy(agent1);
229+
juice_destroy(agent2);
230+
231+
if (success) {
232+
printf("Success\n");
233+
return 0;
234+
} else {
235+
printf("Failure\n");
236+
return -1;
237+
}
238+
}

0 commit comments

Comments
 (0)