[SCM] Socket Wrapper Repository - branch master updated
Andreas Schneider
asn at samba.org
Fri Feb 5 13:13:26 UTC 2021
The branch, master has been updated
via 13a6aca swrap: fix copy on write leak of ~38M for every fork.
via 92a4fce swrap: abort on mutex errors
via affaf42 swrap: fallback to libc_getpeername() when we get an empty sun_path from accept()
via e72898a swrap: make swrap_accept() more resilient against races related to already disconnected sockets
via c3f7465 swrap: add better logging to convert_un_in()
via fa7a9b7 tests/echo_srv: make the main server logic resilient to ECONNABORTED from accept()
from cd51d80 Bump version to 1.3.0
https://git.samba.org/?p=socket_wrapper.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit 13a6aca342120383776251247c72170c142bf017
Author: Stefan Metzmacher <metze at samba.org>
Date: Thu Feb 4 05:02:32 2021 +0100
swrap: fix copy on write leak of ~38M for every fork.
commit 0f8e90dd7e59c473be615dee08d445dca98fdab9
(src/socket_wrapper.c: fix mutex fork handling)
let us touch the whole sockets array on every fork,
because each element in the array has it's own mutex.
max_sockets=65535 * sizeof(struct socket_info_container)=592 = 38796720
This was designed for the use of robust shared mutexes
when moving the sockets array into a shared memory file.
Until we really move to shared memory, we can use a single
global mutex in order to avoid the copy on write leaking.
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
commit 92a4fce9df55e4c1a3b75325baa0f4b0988eeb6c
Author: Stefan Metzmacher <metze at samba.org>
Date: Fri Feb 5 11:50:17 2021 +0100
swrap: abort on mutex errors
There's no way to continue in a reliable way...
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
commit affaf4248c0cc2056081f56199109e4a94348b82
Author: Stefan Metzmacher <metze at samba.org>
Date: Fri Feb 5 13:13:44 2021 +0100
swrap: fallback to libc_getpeername() when we get an empty sun_path from accept()
This hopefully hides the strange behaviour of FreeBSD (at least 12.1)
for already disconnected AF_UNIX sockets.
The race is triggered when the following detects the usage of 'getpeername':
truss -o ./truss.out -f -H -a -e -D -s 160 ctest -V -R test_thread_echo_tcp_connect;
grep getpeername truss.out
In a simplified log the following is happening:
ECHO_SRV(parent): socket(PF_LOCAL,SOCK_STREAM,0) = 4 (0x4)
ECHO_SRV(parent): unlink("/tmp/w_E37bkf/T0A0007") ERR#2 'No such file or directory'
ECHO_SRV(parent): bind(4,{ AF_UNIX "/tmp/w_E37bkf/T0A0007" },106) = 0 (0x0)
ECHO_SRV(parent): listen(4,16) = 0 (0x0)
...
ECHO_SRV(parent): write(2,"SWRAP_ERROR[echo_srv (9792)] - swrap_accept: before accept(sa_socklen=106)\n",75) = 75 (0x4b)
ECHO_SRV(parent): accept4(0x4,0x7ffffffde158,0x7ffffffde150,0x0) = 5 (0x5)
ECHO_SRV(parent): write(2,"SWRAP_ERROR[echo_srv (9792)] - swrap_accept: after accept(sa_socklen=106, family=1)\n",84) = 84 (0x54)
ECHO_SRV(parent): getsockname(5,{ AF_UNIX "/tmp/w_E37bkf/T0A0007" },0x7ffffffde0c0) = 0 (0x0)
ECHO_SRV(parent): swrap_accept() returned a valid connection and a per connection child (pid=9793) handles it
TEST_THREAD: socket(PF_LOCAL,SOCK_STREAM,0) = 7 (0x7)
TEST_THREAD: bind(7,{ AF_UNIX "/tmp/w_E37bkf/T014D4F" },106) = 0 (0x0)
TEST_THREAD: connect(7,{ AF_UNIX "/tmp/w_E37bkf/T0A0007" },106) = 0 (0x0)
TEST_THREAD: close(7) = 0 (0x0)
ECHO_SRV(parent): wait4(-1,0x0,0x0,0x0) = 9793 (0x2641)
ECHO_SRV(parent): close(5) = 0 (0x0)
ECHO_SRV(parent): write(2,"SWRAP_ERROR[echo_srv (9792)] - swrap_accept: before accept(sa_socklen=106)\n",75) = 75 (0x4b)
ECHO_SRV(parent): accept4(0x4,0x7ffffffde158,0x7ffffffde150,0x0) = 5 (0x5)
TEST_THREAD: unlink("/tmp/w_E37bkf/T014D4F") = 0 (0x0)
ECHO_SRV(parent): write(2,"SWRAP_ERROR[echo_srv (9792)] - swrap_accept: after accept(sa_socklen=16, family=1)\n",83) = 83 (0x53)
ECHO_SRV(parent): getpeername(5,0x7ffffffde158,0x7ffffffde150) ERR#57 'Socket is not connected'
ECHO_SRV(parent): getsockname(5,{ AF_UNIX "/tmp/w_E37bkf/T0A0007" },0x7ffffffde0c0) = 0 (0x0)
ECHO_SRV(parent): getpeername(5,0x7ffffffde158,0x7ffffffde150) ERR#57 'Socket is not connected'
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
commit e72898ad92a52a595d4733210483e9689cb5d390
Author: Stefan Metzmacher <metze at samba.org>
Date: Fri Feb 5 12:13:12 2021 +0100
swrap: make swrap_accept() more resilient against races related to already disconnected sockets
Callers of accept() expect to get ECONNABORTED instead of a disconnected
socket.
Even on Linux we have a potential race calling libc_getsockname()
after accept(), so we map ENOTCONN to ECONNABORTED.
We should do all syscalls in order to have peer and sockname, before
doing in memory things like calling sockaddr_convert_from_un().
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
commit c3f7465f9cf453ff3bd53750db4024deaba02fb5
Author: Stefan Metzmacher <metze at samba.org>
Date: Fri Feb 5 13:13:07 2021 +0100
swrap: add better logging to convert_un_in()
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
commit fa7a9b7ab5edf3ca986979b9cdaa08deae3d9308
Author: Stefan Metzmacher <metze at samba.org>
Date: Fri Feb 5 12:22:47 2021 +0100
tests/echo_srv: make the main server logic resilient to ECONNABORTED from accept()
That should fix a race where the connect() directly followed by close()
in test_thread_echo_tcp_connect will cause the echo_srv to terminate
early, which results in connect() returning ECONNREFUSED in for other
threads.
This mainly happens on FreeBSD, but it can also happen on Linux.
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
-----------------------------------------------------------------------
Summary of changes:
src/socket_wrapper.c | 146 ++++++++++++++++++++++++++++++++++++---------------
tests/echo_srv.c | 3 ++
2 files changed, 107 insertions(+), 42 deletions(-)
Changeset truncated at 500 lines:
diff --git a/src/socket_wrapper.c b/src/socket_wrapper.c
index 14d0dda..43a5892 100644
--- a/src/socket_wrapper.c
+++ b/src/socket_wrapper.c
@@ -183,7 +183,6 @@ enum swrap_dbglvl_e {
/* Add new global locks here please */
# define SWRAP_REINIT_ALL do { \
- size_t __i; \
int ret; \
ret = socket_wrapper_init_mutex(&sockets_mutex); \
if (ret != 0) exit(-1); \
@@ -191,10 +190,8 @@ enum swrap_dbglvl_e {
if (ret != 0) exit(-1); \
ret = socket_wrapper_init_mutex(&first_free_mutex); \
if (ret != 0) exit(-1); \
- for (__i = 0; (sockets != NULL) && __i < socket_info_max; __i++) { \
- ret = socket_wrapper_init_mutex(&sockets[__i].meta.mutex); \
- if (ret != 0) exit(-1); \
- } \
+ ret = socket_wrapper_init_mutex(&sockets_si_global); \
+ if (ret != 0) exit(-1); \
ret = socket_wrapper_init_mutex(&autobind_start_mutex); \
if (ret != 0) exit(-1); \
ret = socket_wrapper_init_mutex(&pcap_dump_mutex); \
@@ -204,27 +201,20 @@ enum swrap_dbglvl_e {
} while(0)
# define SWRAP_LOCK_ALL do { \
- size_t __i; \
swrap_mutex_lock(&sockets_mutex); \
swrap_mutex_lock(&socket_reset_mutex); \
swrap_mutex_lock(&first_free_mutex); \
- for (__i = 0; (sockets != NULL) && __i < socket_info_max; __i++) { \
- swrap_mutex_lock(&sockets[__i].meta.mutex); \
- } \
+ swrap_mutex_lock(&sockets_si_global); \
swrap_mutex_lock(&autobind_start_mutex); \
swrap_mutex_lock(&pcap_dump_mutex); \
swrap_mutex_lock(&mtu_update_mutex); \
} while(0)
# define SWRAP_UNLOCK_ALL do { \
- size_t __s; \
swrap_mutex_unlock(&mtu_update_mutex); \
swrap_mutex_unlock(&pcap_dump_mutex); \
swrap_mutex_unlock(&autobind_start_mutex); \
- for (__s = 0; (sockets != NULL) && __s < socket_info_max; __s++) { \
- size_t __i = (socket_info_max - 1) - __s; \
- swrap_mutex_unlock(&sockets[__i].meta.mutex); \
- } \
+ swrap_mutex_unlock(&sockets_si_global); \
swrap_mutex_unlock(&first_free_mutex); \
swrap_mutex_unlock(&socket_reset_mutex); \
swrap_mutex_unlock(&sockets_mutex); \
@@ -235,12 +225,20 @@ enum swrap_dbglvl_e {
#define SWRAP_LOCK_SI(si) do { \
struct socket_info_container *sic = SOCKET_INFO_CONTAINER(si); \
- swrap_mutex_lock(&sic->meta.mutex); \
+ if (sic != NULL) { \
+ swrap_mutex_lock(&sockets_si_global); \
+ } else { \
+ abort(); \
+ } \
} while(0)
#define SWRAP_UNLOCK_SI(si) do { \
struct socket_info_container *sic = SOCKET_INFO_CONTAINER(si); \
- swrap_mutex_unlock(&sic->meta.mutex); \
+ if (sic != NULL) { \
+ swrap_mutex_unlock(&sockets_si_global); \
+ } else { \
+ abort(); \
+ } \
} while(0)
#if defined(HAVE_GETTIMEOFDAY_TZ) || defined(HAVE_GETTIMEOFDAY_TZ_VOID)
@@ -337,7 +335,13 @@ struct socket_info_meta
{
unsigned int refcount;
int next_free;
- pthread_mutex_t mutex;
+ /*
+ * As long as we don't use shared memory
+ * for the sockets array, we use
+ * sockets_si_global as a single mutex.
+ *
+ * pthread_mutex_t mutex;
+ */
};
struct socket_info_container
@@ -372,6 +376,14 @@ static pthread_mutex_t socket_reset_mutex = PTHREAD_MUTEX_INITIALIZER;
/* Mutex to synchronize access to first free index in socket_info array */
static pthread_mutex_t first_free_mutex = PTHREAD_MUTEX_INITIALIZER;
+/*
+ * Mutex to synchronize access to to socket_info structures
+ * We use a single global mutex in order to avoid leaking
+ * ~ 38M copy on write memory per fork.
+ * max_sockets=65535 * sizeof(struct socket_info_container)=592 = 38796720
+ */
+static pthread_mutex_t sockets_si_global = PTHREAD_MUTEX_INITIALIZER;
+
/* Mutex to synchronize access to packet capture dump file */
static pthread_mutex_t pcap_dump_mutex = PTHREAD_MUTEX_INITIALIZER;
@@ -755,6 +767,7 @@ static void _swrap_mutex_lock(pthread_mutex_t *mutex, const char *name, const ch
if (ret != 0) {
SWRAP_LOG(SWRAP_LOG_ERROR, "PID(%d):PPID(%d): %s(%u): Couldn't lock pthread mutex(%s) - %s",
getpid(), getppid(), caller, line, name, strerror(ret));
+ abort();
}
}
@@ -767,6 +780,7 @@ static void _swrap_mutex_unlock(pthread_mutex_t *mutex, const char *name, const
if (ret != 0) {
SWRAP_LOG(SWRAP_LOG_ERROR, "PID(%d):PPID(%d): %s(%u): Couldn't unlock pthread mutex(%s) - %s",
getpid(), getppid(), caller, line, name, strerror(ret));
+ abort();
}
}
@@ -1705,27 +1719,18 @@ static void socket_wrapper_init_sockets(void)
}
swrap_mutex_lock(&first_free_mutex);
+ swrap_mutex_lock(&sockets_si_global);
first_free = 0;
for (i = 0; i < max_sockets; i++) {
swrap_set_next_free(&sockets[i].info, i+1);
- sockets[i].meta.mutex = (pthread_mutex_t)PTHREAD_MUTEX_INITIALIZER;
- }
-
- for (i = 0; i < max_sockets; i++) {
- ret = socket_wrapper_init_mutex(&sockets[i].meta.mutex);
- if (ret != 0) {
- SWRAP_LOG(SWRAP_LOG_ERROR,
- "Failed to initialize pthread mutex i=%zu", i);
- goto done;
- }
}
/* mark the end of the free list */
swrap_set_next_free(&sockets[max_sockets-1].info, -1);
-done:
+ swrap_mutex_unlock(&sockets_si_global);
swrap_mutex_unlock(&first_free_mutex);
swrap_mutex_unlock(&sockets_mutex);
if (ret != 0) {
@@ -1885,31 +1890,40 @@ static int convert_un_in(const struct sockaddr_un *un, struct sockaddr *in, sock
if (p) p++; else p = un->sun_path;
if (sscanf(p, SOCKET_FORMAT, &type, &iface, &prt) != 3) {
+ SWRAP_LOG(SWRAP_LOG_ERROR, "sun_path[%s] p[%s]",
+ un->sun_path, p);
errno = EINVAL;
return -1;
}
- SWRAP_LOG(SWRAP_LOG_TRACE, "type %c iface %u port %u",
- type, iface, prt);
-
if (iface == 0 || iface > MAX_WRAPPED_INTERFACES) {
+ SWRAP_LOG(SWRAP_LOG_ERROR, "type %c iface %u port %u",
+ type, iface, prt);
errno = EINVAL;
return -1;
}
if (prt > 0xFFFF) {
+ SWRAP_LOG(SWRAP_LOG_ERROR, "type %c iface %u port %u",
+ type, iface, prt);
errno = EINVAL;
return -1;
}
+ SWRAP_LOG(SWRAP_LOG_TRACE, "type %c iface %u port %u",
+ type, iface, prt);
+
switch(type) {
case SOCKET_TYPE_CHAR_TCP:
case SOCKET_TYPE_CHAR_UDP: {
struct sockaddr_in *in2 = (struct sockaddr_in *)(void *)in;
if ((*len) < sizeof(*in2)) {
- errno = EINVAL;
- return -1;
+ SWRAP_LOG(SWRAP_LOG_ERROR,
+ "V4: *len(%zu) < sizeof(*in2)=%zu",
+ (size_t)*len, sizeof(*in2));
+ errno = EINVAL;
+ return -1;
}
memset(in2, 0, sizeof(*in2));
@@ -1926,6 +1940,10 @@ static int convert_un_in(const struct sockaddr_un *un, struct sockaddr *in, sock
struct sockaddr_in6 *in2 = (struct sockaddr_in6 *)(void *)in;
if ((*len) < sizeof(*in2)) {
+ SWRAP_LOG(SWRAP_LOG_ERROR,
+ "V6: *len(%zu) < sizeof(*in2)=%zu",
+ (size_t)*len, sizeof(*in2));
+ SWRAP_LOG(SWRAP_LOG_ERROR, "LINE:%d", __LINE__);
errno = EINVAL;
return -1;
}
@@ -1941,6 +1959,8 @@ static int convert_un_in(const struct sockaddr_un *un, struct sockaddr *in, sock
}
#endif
default:
+ SWRAP_LOG(SWRAP_LOG_ERROR, "type %c iface %u port %u",
+ type, iface, prt);
errno = EINVAL;
return -1;
}
@@ -3627,10 +3647,12 @@ static int swrap_accept(int s,
ret = libc_accept(s, &un_addr.sa.s, &un_addr.sa_socklen);
#endif
if (ret == -1) {
- if (errno == ENOTSOCK) {
+ int saved_errno = errno;
+ if (saved_errno == ENOTSOCK) {
/* Remove stale fds */
swrap_remove_stale(s);
}
+ errno = saved_errno;
return ret;
}
@@ -3639,6 +3661,50 @@ static int swrap_accept(int s,
/* Check if we have a stale fd and remove it */
swrap_remove_stale(fd);
+ if (un_addr.sa.un.sun_path[0] == '\0') {
+ /*
+ * FreeBSD seems to have a problem where
+ * accept4() on the unix socket doesn't
+ * ECONNABORTED for already disconnected connections.
+ *
+ * Let's try libc_getpeername() to get the peer address
+ * as a fallback, but it'll likely return ENOTCONN,
+ * which we have to map to ECONNABORTED.
+ */
+ un_addr.sa_socklen = sizeof(struct sockaddr_un),
+ ret = libc_getpeername(fd, &un_addr.sa.s, &un_addr.sa_socklen);
+ if (ret == -1) {
+ int saved_errno = errno;
+ libc_close(fd);
+ if (saved_errno == ENOTCONN) {
+ /*
+ * If the connection is already disconnected
+ * we should return ECONNABORTED.
+ */
+ saved_errno = ECONNABORTED;
+ }
+ errno = saved_errno;
+ return ret;
+ }
+ }
+
+ ret = libc_getsockname(fd,
+ &un_my_addr.sa.s,
+ &un_my_addr.sa_socklen);
+ if (ret == -1) {
+ int saved_errno = errno;
+ libc_close(fd);
+ if (saved_errno == ENOTCONN) {
+ /*
+ * If the connection is already disconnected
+ * we should return ECONNABORTED.
+ */
+ saved_errno = ECONNABORTED;
+ }
+ errno = saved_errno;
+ return ret;
+ }
+
SWRAP_LOCK_SI(parent_si);
ret = sockaddr_convert_from_un(parent_si,
@@ -3648,8 +3714,10 @@ static int swrap_accept(int s,
&in_addr.sa.s,
&in_addr.sa_socklen);
if (ret == -1) {
+ int saved_errno = errno;
SWRAP_UNLOCK_SI(parent_si);
libc_close(fd);
+ errno = saved_errno;
return ret;
}
@@ -3677,14 +3745,6 @@ static int swrap_accept(int s,
*addrlen = in_addr.sa_socklen;
}
- ret = libc_getsockname(fd,
- &un_my_addr.sa.s,
- &un_my_addr.sa_socklen);
- if (ret == -1) {
- libc_close(fd);
- return ret;
- }
-
ret = sockaddr_convert_from_un(child_si,
&un_my_addr.sa.un,
un_my_addr.sa_socklen,
@@ -3692,7 +3752,9 @@ static int swrap_accept(int s,
&in_my_addr.sa.s,
&in_my_addr.sa_socklen);
if (ret == -1) {
+ int saved_errno = errno;
libc_close(fd);
+ errno = saved_errno;
return ret;
}
diff --git a/tests/echo_srv.c b/tests/echo_srv.c
index 87c85f7..0aefa9a 100644
--- a/tests/echo_srv.c
+++ b/tests/echo_srv.c
@@ -538,6 +538,9 @@ static void echo_tcp(int sock)
while (1) {
s = accept(sock, &addr.sa.s, &addr.sa_socklen);
+ if (s == -1 && errno == ECONNABORTED) {
+ continue;
+ }
if (s == -1) {
perror("accept");
goto done;
--
Socket Wrapper Repository
More information about the samba-cvs
mailing list