From 7bd11cf01a07d26215c4ad51a3bc4521f980188b Mon Sep 17 00:00:00 2001 From: Fabrice Bellet Date: Fri, 1 Oct 2021 17:35:44 +0200 Subject: conncheck: don't ignore local socket errors With this patch we ensure that local socket errors during connection establishment are properly transmitted to the connection check layer, so the related pair can be put in state failed when needed. The local socket errors we are interested in, are those occuring when the local source address cannot be bound anymore, because the underlying interface vanished for example. In particular, we don't ignore errors coming from g_socket_bind() with tcp active sockets, that perform a bind to *whatever* local address is available, in case it cannot bind to the specified address. This behaviour was demonstrated with the test-tcp example, that tried to bind to the IPv4 loopback address on a socket initially created for the IPv6 loopback. --- agent/conncheck.c | 10 ++++++++-- socket/tcp-active.c | 54 ++++++++++++++++++++++++++++++++++++++++------------- socket/udp-bsd.c | 2 +- socket/udp-turn.c | 13 ++++++++++--- tests/test-tcp.c | 2 +- 5 files changed, 61 insertions(+), 20 deletions(-) diff --git a/agent/conncheck.c b/agent/conncheck.c index 53faba3..cd8f9cf 100644 --- a/agent/conncheck.c +++ b/agent/conncheck.c @@ -2975,12 +2975,18 @@ int conn_check_send (NiceAgent *agent, CandidateCheckPair *pair) component2); nice_component_attach_socket (component2, new_socket); + } else { + priv_remove_stun_transaction (pair, stun, component); + return -1; } } } /* send the conncheck */ - agent_socket_send (pair->sockptr, &pair->remote->addr, - buffer_len, (gchar *)stun->buffer); + if (agent_socket_send (pair->sockptr, &pair->remote->addr, + buffer_len, (gchar *)stun->buffer) < 0) { + priv_remove_stun_transaction (pair, stun, component); + return -1; + } if (agent->compatibility == NICE_COMPATIBILITY_OC2007R2) ms_ice2_legacy_conncheck_send (&stun->message, pair->sockptr, diff --git a/socket/tcp-active.c b/socket/tcp-active.c index eb7cd7f..558b303 100644 --- a/socket/tcp-active.c +++ b/socket/tcp-active.c @@ -41,6 +41,7 @@ #include "socket.h" #include "tcp-active.h" +#include "agent-priv.h" #include #include @@ -192,7 +193,14 @@ nice_tcp_active_socket_connect (NiceSocket *sock, NiceAddress *addr) gboolean gret = FALSE; GSocketAddress *gaddr; NiceAddress local_addr; + NiceAddress remote_addr; NiceSocket *new_socket = NULL; + union { + struct sockaddr_storage ss; + struct sockaddr sa; + } sa; + char remote_addr_str[INET6_ADDRSTRLEN]; + char local_addr_str[INET6_ADDRSTRLEN]; if (addr == NULL) { /* We can't connect a tcp socket with no destination address */ @@ -234,29 +242,27 @@ nice_tcp_active_socket_connect (NiceSocket *sock, NiceAddress *addr) /* setting TCP_NODELAY to TRUE in order to avoid packet batching */ g_socket_set_option (gsock, IPPROTO_TCP, TCP_NODELAY, TRUE, NULL); - /* Allow g_socket_bind to fail */ - g_socket_bind (gsock, priv->local_addr, FALSE, NULL); + gret = g_socket_bind (gsock, priv->local_addr, FALSE, &gerr); + + if (gret == FALSE) { + if (g_error_matches (gerr, G_IO_ERROR, G_IO_ERROR_PENDING) == FALSE) + goto error; + g_clear_error (&gerr); + } gret = g_socket_connect (gsock, gaddr, NULL, &gerr); g_object_unref (gaddr); if (gret == FALSE) { - if (g_error_matches (gerr, G_IO_ERROR, G_IO_ERROR_PENDING) == FALSE) { - g_error_free (gerr); - g_socket_close (gsock, NULL); - g_object_unref (gsock); - return NULL; - } + if (g_error_matches (gerr, G_IO_ERROR, G_IO_ERROR_PENDING) == FALSE) + goto error; g_error_free (gerr); } gaddr = g_socket_get_local_address (gsock, NULL); if (gaddr == NULL || - !g_socket_address_to_native (gaddr, &name.addr, sizeof (name), NULL)) { - g_socket_close (gsock, NULL); - g_object_unref (gsock); - return NULL; - } + !g_socket_address_to_native (gaddr, &name.addr, sizeof (name), NULL)) + goto error2; g_object_unref (gaddr); nice_address_set_from_sockaddr (&local_addr, &name.addr); @@ -266,4 +272,26 @@ nice_tcp_active_socket_connect (NiceSocket *sock, NiceAddress *addr) g_object_unref (gsock); return new_socket; + +error: + g_socket_address_to_native (gaddr, &sa, sizeof (sa), NULL); + nice_address_set_from_sockaddr (&remote_addr, &sa.sa); + nice_address_to_string (&remote_addr, remote_addr_str); + + g_socket_address_to_native (priv->local_addr, &sa, sizeof (sa), NULL); + nice_address_set_from_sockaddr (&local_addr, &sa.sa); + nice_address_to_string (&local_addr, local_addr_str); + + nice_debug ("%s: tcp-active socket %p %s:%u -> %s:%u: error: %s", + G_STRFUNC, sock, + local_addr_str, nice_address_get_port (&local_addr), + remote_addr_str, nice_address_get_port (&remote_addr), + gerr->message); + + g_error_free (gerr); + +error2: + g_socket_close (gsock, NULL); + g_object_unref (gsock); + return NULL; } diff --git a/socket/udp-bsd.c b/socket/udp-bsd.c index d1c8513..7064408 100644 --- a/socket/udp-bsd.c +++ b/socket/udp-bsd.c @@ -335,7 +335,7 @@ socket_send_messages (NiceSocket *sock, const NiceAddress *to, nice_address_to_string (&local_addr, local_addr_str); g_object_unref (gsocket); - nice_debug_verbose ("%s: udp-bsd socket %p %s:%u -> %s:%u: error: %s", + nice_debug ("%s: udp-bsd socket %p %s:%u -> %s:%u: error: %s", G_STRFUNC, sock, local_addr_str, nice_address_get_port (&local_addr), remote_addr_str, nice_address_get_port (&remote_addr), diff --git a/socket/udp-turn.c b/socket/udp-turn.c index 1e86a0a..c1dac62 100644 --- a/socket/udp-turn.c +++ b/socket/udp-turn.c @@ -948,7 +948,8 @@ socket_send_message (NiceSocket *sock, const NiceAddress *to, if (priv->compatibility == NICE_TURN_SOCKET_COMPATIBILITY_RFC5766 && !priv_has_permission_for_peer (priv, to)) { if (!priv_has_sent_permission_for_peer (priv, to)) { - priv_send_create_permission (priv, to); + if (!priv_send_create_permission (priv, to)) + goto error; } /* enque data */ @@ -2012,7 +2013,7 @@ priv_send_create_permission(UdpTurnPriv *priv, const NiceAddress *peer) { guint msg_buf_len; - gboolean res = FALSE; + gssize res = 0; TURNMessage *msg = g_new0 (TURNMessage, 1); union { struct sockaddr_storage storage; @@ -2051,6 +2052,11 @@ priv_send_create_permission(UdpTurnPriv *priv, msg_buf_len, (gchar *) msg->buffer, FALSE); } + if (res < 0) { + g_free(msg); + goto done; + } + if (nice_socket_is_reliable (priv->base_socket)) { stun_timer_start_reliable (&msg->timer, STUN_TIMER_DEFAULT_RELIABLE_TIMEOUT); @@ -2065,7 +2071,8 @@ priv_send_create_permission(UdpTurnPriv *priv, g_free(msg); } - return res; +done: + return (res > 0); } static gboolean diff --git a/tests/test-tcp.c b/tests/test-tcp.c index f94ac88..fb38943 100644 --- a/tests/test-tcp.c +++ b/tests/test-tcp.c @@ -93,7 +93,7 @@ main (void) mainloop = g_main_loop_new (NULL, FALSE); nice_address_init (&active_bind_addr); - g_assert_true (nice_address_set_from_string (&active_bind_addr, "::1")); + g_assert_true (nice_address_set_from_string (&active_bind_addr, "127.0.0.1")); nice_address_init (&passive_bind_addr); g_assert_true (nice_address_set_from_string (&passive_bind_addr, "127.0.0.1")); -- cgit v1.2.1