Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1915?usp=email
to review the following change.
Change subject: socket: let an optional UDP socket give up on a failed bind
......................................................................
socket: let an optional UDP socket give up on a failed bind
create_socket_udp_configured()'s "optional" argument covers only the
socket() call: socket_bind() is fatal, so a caller that can do without
the socket still dies if the address is unavailable.
That is a problem for --server-probe, which binds a probe socket for
every address family in use, while a connection binds only the family of
the remote it is about to try. A --lport whose IPv6 port happens to be
taken by another process therefore kills a client that would have
connected over IPv4.
Give socket_bind() a msglevel and a return value, and let "optional"
apply to the bind as well: the socket is closed and SOCKET_UNDEFINED is
returned, so the caller can skip that address family. Every existing
caller passes M_FATAL and keeps its behaviour.
Change-Id: I24e5925a72aaede53ef9e863fe96fcd7363df402
Signed-off-by: Lev Stipakov <[email protected]>
---
M src/openvpn/manage.c
M src/openvpn/socket.c
M src/openvpn/socket.h
3 files changed, 29 insertions(+), 14 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/15/1915/1
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index e121b38..46b0abe 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -2044,7 +2044,7 @@
{
man->connection.sd_top = create_socket_tcp(man->settings.local);
socket_bind(man->connection.sd_top, man->settings.local,
man->settings.local->ai_family,
- "MANAGEMENT", false);
+ "MANAGEMENT", false, M_FATAL);
}
/*
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 41c8bac..4c0aab5 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -627,12 +627,13 @@
{
if (sock->socks_proxy && sock->info.proto == PROTO_UDP)
{
- socket_bind(sock->ctrl_sd, sock->info.lsa->bind_local,
sock->info.af, "SOCKS", false);
+ socket_bind(sock->ctrl_sd, sock->info.lsa->bind_local,
sock->info.af, "SOCKS", false,
+ M_FATAL);
}
else
{
socket_bind(sock->sd, sock->info.lsa->bind_local, sock->info.af,
"TCP/UDP",
- sock->info.bind_ipv6_only);
+ sock->info.bind_ipv6_only, M_FATAL);
}
}
}
@@ -678,9 +679,11 @@
}
socket_apply_options(sd, sbs, mark, bind_dev);
- if (bind_addr)
+ if (bind_addr
+ && !socket_bind(sd, bind_addr, af, "TCP/UDP", bind_ipv6_only, optional
? D_LOW : M_FATAL))
{
- socket_bind(sd, bind_addr, af, "TCP/UDP", bind_ipv6_only);
+ openvpn_close_socket(sd);
+ return SOCKET_UNDEFINED;
}
return sd;
}
@@ -914,9 +917,9 @@
return new_sd;
}
-void
+bool
socket_bind(socket_descriptor_t sd, struct addrinfo *local, int ai_family,
const char *prefix,
- bool ipv6only)
+ bool ipv6only, msglvl_t msglevel)
{
struct gc_arena gc = gc_new();
@@ -940,8 +943,10 @@
}
if (!cur)
{
- msg(M_FATAL, "%s: Socket bind failed: Addr to bind has no %s record",
prefix,
+ msg(msglevel, "%s: Socket bind failed: Addr to bind has no %s record",
prefix,
addr_family_name(ai_family));
+ gc_free(&gc);
+ return false;
}
if (ai_family == AF_INET6)
@@ -956,10 +961,13 @@
}
if (openvpn_bind(sd, cur->ai_addr, cur->ai_addrlen))
{
- msg(M_FATAL | M_ERRNO, "%s: Socket bind failed on local address %s",
prefix,
+ msg(msglevel | M_ERRNO, "%s: Socket bind failed on local address %s",
prefix,
print_sockaddr_ex(local->ai_addr, ":", PS_SHOW_PORT, &gc));
+ gc_free(&gc);
+ return false;
}
gc_free(&gc);
+ return true;
}
int
diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h
index 46fa29c..20c10c2 100644
--- a/src/openvpn/socket.h
+++ b/src/openvpn/socket.h
@@ -348,8 +348,15 @@
struct link_socket *link_socket_new(void);
-void socket_bind(socket_descriptor_t sd, struct addrinfo *local, int
af_family, const char *prefix,
- bool ipv6only);
+/**
+ * Bind @p sd to the first @p af_family address of @p local. A failure is
+ * reported at @p msglevel, so a caller that can do without the bind passes a
+ * non-fatal level and checks the result.
+ *
+ * @return whether the socket was bound
+ */
+bool socket_bind(socket_descriptor_t sd, struct addrinfo *local, int
af_family, const char *prefix,
+ bool ipv6only, msglvl_t msglevel);
int openvpn_connect(socket_descriptor_t sd, const struct sockaddr *remote, int
connect_timeout,
volatile int *signal_received);
@@ -405,9 +412,9 @@
* Create a UDP socket for @p af set up the way a link socket is:
--sndbuf/--rcvbuf,
* --mark, --bind-dev and, when @p bind_addr is given, the local bind
(IPV6_V6ONLY
* per @p bind_ipv6_only). Shared by create_socket() and the --server-probe
sockets,
- * so a probe socket is the connection socket it may become. A failed bind is
- * fatal; with @p optional a socket the host cannot create is not, so
- * --server-probe can skip that address family (SOCKET_UNDEFINED is returned).
+ * so a probe socket is the connection socket it may become. With @p optional a
+ * socket the host cannot create or bind is not fatal, so --server-probe can
skip
+ * that address family (SOCKET_UNDEFINED is returned).
*/
socket_descriptor_t create_socket_udp_configured(sa_family_t af, unsigned int
sockflags,
const struct
socket_buffer_size *sbs, int mark,
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1915?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I24e5925a72aaede53ef9e863fe96fcd7363df402
Gerrit-Change-Number: 1915
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
