The branch, master has been updated
via 5529ba0 swrap: Replace socket_fds linked list with an array
from 321833e tests: Fix resouce leak in echo_srv tcp handling
https://git.samba.org/?p=socket_wrapper.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit 5529ba060519017bb71de5de6e7b81e5b3d56ccf
Author: Anoop C S <anoo...@redhat.com>
Date: Sun Jun 10 13:19:14 2018 +0530
swrap: Replace socket_fds linked list with an array
This fixes the following bug:
As we are using a doubly linked list, we need a mutex which needs to be
locked when we are reading it that we do not end up with invalid
pointers.
The following can happen:
We are in swrap_close() which calls find_socket_info_fd() this locks the
mutex for the linked list. Now we get a singal SIGCHILD and the signal
handler is called. The signal handler calls swrap_write() and we try to
find out if the socket is managed by socket_wrapper calling
find_socket_info_fd() again -> DEADLOCK!
By moving to an array to handle the socket fds and using the fd as the
array access, we do not need a mutex for reading anymore. All we need is
a memory barrier.
This change also improves the performance as we move from the a linked
list to a hash table!
Pair-Programmed-With: Andreas Schneider <a...@samba.org>
Signed-off-by: Anoop C S <anoo...@redhat.com>
Signed-off-by: Andreas Schneider <a...@samba.org>
Reviewed-by: Stefan Metzmacher <me...@samba.org>
-----------------------------------------------------------------------
Summary of changes:
src/socket_wrapper.c | 314 ++++++++++++++++++++-------------------------------
1 file changed, 122 insertions(+), 192 deletions(-)
Changeset truncated at 500 lines:
diff --git a/src/socket_wrapper.c b/src/socket_wrapper.c
index 6c9ec51..a22f7dc 100644
--- a/src/socket_wrapper.c
+++ b/src/socket_wrapper.c
@@ -142,6 +142,10 @@ enum swrap_dbglvl_e {
} while(0)
#endif
+#ifndef SAFE_FREE
+#define SAFE_FREE(x) do { if ((x) != NULL) {free(x); (x)=NULL;} } while(0)
+#endif
+
#ifndef discard_const
#define discard_const(ptr) ((void *)((uintptr_t)(ptr)))
#endif
@@ -198,68 +202,6 @@ enum swrap_dbglvl_e {
pthread_mutex_unlock(&sic->meta.mutex); \
} while(0)
-#define DLIST_ADD(list, item) do { \
- if (!(list)) { \
- (item)->prev = NULL; \
- (item)->next = NULL; \
- (list) = (item); \
- } else { \
- (item)->prev = NULL; \
- (item)->next = (list); \
- (list)->prev = (item); \
- (list) = (item); \
- } \
-} while (0)
-
-#define SWRAP_DLIST_ADD(list, item) do { \
- SWRAP_LOCK(list); \
- DLIST_ADD(list, item); \
- SWRAP_UNLOCK(list); \
-} while (0)
-
-#define DLIST_REMOVE(list, item) do { \
- if ((list) == (item)) { \
- (list) = (item)->next; \
- if (list) { \
- (list)->prev = NULL; \
- } \
- } else { \
- if ((item)->prev) { \
- (item)->prev->next = (item)->next; \
- } \
- if ((item)->next) { \
- (item)->next->prev = (item)->prev; \
- } \
- } \
- (item)->prev = NULL; \
- (item)->next = NULL; \
-} while (0)
-
-#define SWRAP_DLIST_REMOVE(list,item) do { \
- SWRAP_LOCK(list); \
- DLIST_REMOVE(list, item); \
- SWRAP_UNLOCK(list); \
-} while (0)
-
-#define DLIST_ADD_AFTER(list, item, el) do { \
- if ((list) == NULL || (el) == NULL) { \
- DLIST_ADD(list, item); \
- } else { \
- (item)->prev = (el); \
- (item)->next = (el)->next; \
- (el)->next = (item); \
- if ((item)->next != NULL) { \
- (item)->next->prev = (item); \
- } \
- } \
-} while (0)
-
-#define SWRAP_DLIST_ADD_AFTER(list, item, el) do { \
- SWRAP_LOCK(list); \
- DLIST_ADD_AFTER(list, item, el); \
- SWRAP_UNLOCK(list); \
-} while (0)
-
#if defined(HAVE_GETTIMEOFDAY_TZ) || defined(HAVE_GETTIMEOFDAY_TZ_VOID)
#define swrapGetTimeOfDay(tval) gettimeofday(tval,NULL)
#else
@@ -288,7 +230,6 @@ enum swrap_dbglvl_e {
#define SOCKET_MAX_SOCKETS 1024
-
/*
* Maximum number of socket_info structures that can
* be used. Can be overriden by the environment variable
@@ -316,17 +257,6 @@ struct swrap_address {
} sa;
};
-struct socket_info_fd {
- struct socket_info_fd *prev, *next;
- int fd;
-
- /*
- * Points to corresponding index in array of
- * socket_info structures
- */
- int si_index;
-};
-
int first_free;
struct socket_info
@@ -376,7 +306,7 @@ static size_t max_sockets = 0;
* numerical value gets changed. So its better to store it locally to each
* process rather than including it within socket_info which will be shared.
*/
-static struct socket_info_fd *socket_fds;
+static int *socket_fds_idx;
/* The mutex for accessing the global libc.symbols */
static pthread_mutex_t libc_symbol_binding_mutex = PTHREAD_MUTEX_INITIALIZER;
@@ -390,12 +320,6 @@ static pthread_mutex_t autobind_start_mutex =
PTHREAD_MUTEX_INITIALIZER;
static pthread_mutex_t sockets_mutex = PTHREAD_MUTEX_INITIALIZER;
/*
- * Global mutex to protect modification of the socket_fds linked
- * list structure by different threads within a process.
- */
-static pthread_mutex_t socket_fds_mutex = PTHREAD_MUTEX_INITIALIZER;
-
-/*
* Global mutex to synchronize the query for first free index in array of
* socket_info structures by different threads within a process.
*/
@@ -1371,6 +1295,30 @@ done:
return max_sockets;
}
+static void socket_wrapper_init_fds_idx(void)
+{
+ int *tmp = NULL;
+ size_t i;
+
+ if (socket_fds_idx != NULL) {
+ return;
+ }
+
+ tmp = (int *)calloc(SOCKET_WRAPPER_MAX_SOCKETS_DEFAULT, sizeof(int));
+ if (tmp == NULL) {
+ SWRAP_LOG(SWRAP_LOG_ERROR,
+ "Failed to allocate socket fds index array: %s",
+ strerror(errno));
+ exit(-1);
+ }
+
+ for (i = 0; i < SOCKET_WRAPPER_MAX_SOCKETS_DEFAULT; i++) {
+ tmp[i] = -1;
+ }
+
+ socket_fds_idx = tmp;
+}
+
static void socket_wrapper_init_sockets(void)
{
size_t i;
@@ -1382,6 +1330,8 @@ static void socket_wrapper_init_sockets(void)
return;
}
+ socket_wrapper_init_fds_idx();
+
max_sockets = socket_wrapper_max_sockets();
sockets = (struct socket_info_container *)calloc(max_sockets,
@@ -1438,6 +1388,42 @@ static unsigned int socket_wrapper_default_iface(void)
return 1;/* 127.0.0.1 */
}
+static void set_socket_info_index(int fd, int idx)
+{
+ socket_fds_idx[fd] = idx;
+ /* This builtin issues a full memory barrier. */
+ __sync_synchronize();
+}
+
+static void reset_socket_info_index(int fd)
+{
+ set_socket_info_index(fd, -1);
+}
+
+static int find_socket_info_index(int fd)
+{
+ if (fd < 0) {
+ return -1;
+ }
+
+ if (socket_fds_idx == NULL) {
+ return -1;
+ }
+
+ if (fd >= SOCKET_WRAPPER_MAX_SOCKETS_DEFAULT) {
+ SWRAP_LOG(SWRAP_LOG_ERROR,
+ "The max socket index limit of %u has been reached, "
+ "trying to add %d",
+ SOCKET_WRAPPER_MAX_SOCKETS_DEFAULT,
+ fd);
+ return -1;
+ }
+
+ /* This builtin issues a full memory barrier. */
+ __sync_synchronize();
+ return socket_fds_idx[fd];
+}
+
static int swrap_add_socket_info(struct socket_info *si_input)
{
struct socket_info *si = NULL;
@@ -1473,25 +1459,23 @@ out:
static int swrap_create_socket(struct socket_info *si, int fd)
{
- struct socket_info_fd *fi = NULL;
int idx;
- fi = (struct socket_info_fd *)calloc(1, sizeof(struct socket_info_fd));
- if (fi == NULL) {
- errno = ENOMEM;
+ if (fd >= SOCKET_WRAPPER_MAX_SOCKETS_DEFAULT) {
+ SWRAP_LOG(SWRAP_LOG_ERROR,
+ "The max socket index limit of %u has been reached, "
+ "trying to add %d",
+ SOCKET_WRAPPER_MAX_SOCKETS_DEFAULT,
+ fd);
return -1;
}
idx = swrap_add_socket_info(si);
if (idx == -1) {
- free(fi);
return -1;
}
- fi->fd = fd;
- fi->si_index = idx;
-
- SWRAP_DLIST_ADD(socket_fds, fi);
+ set_socket_info_index(fd, idx);
return idx;
}
@@ -1863,34 +1847,6 @@ static int convert_in_un_alloc(struct socket_info *si,
const struct sockaddr *in
return 0;
}
-static struct socket_info_fd *find_socket_info_fd(int fd)
-{
- struct socket_info_fd *f;
-
- SWRAP_LOCK(socket_fds);
-
- for (f = socket_fds; f; f = f->next) {
- if (f->fd == fd) {
- break;
- }
- }
-
- SWRAP_UNLOCK(socket_fds);
-
- return f;
-}
-
-static int find_socket_info_index(int fd)
-{
- struct socket_info_fd *fi = find_socket_info_fd(fd);
-
- if (fi == NULL) {
- return -1;
- }
-
- return fi->si_index;
-}
-
static struct socket_info *find_socket_info(int fd)
{
int idx = find_socket_info_index(fd);
@@ -1996,29 +1952,25 @@ static bool check_addr_port_in_use(const struct
sockaddr *sa, socklen_t len)
static void swrap_remove_stale(int fd)
{
- struct socket_info_fd *fi = find_socket_info_fd(fd);
struct socket_info *si;
int si_index;
- if (fi == NULL) {
- return;
- }
-
SWRAP_LOG(SWRAP_LOG_TRACE, "remove stale wrapper for %d", fd);
- si_index = fi->si_index;
+ si_index = find_socket_info_index(fd);
+ if (si_index == -1) {
+ return;
+ }
si = swrap_get_socket_info(si_index);
+ reset_socket_info_index(fd);
+
SWRAP_LOCK(first_free);
SWRAP_LOCK_SI(si);
- SWRAP_DLIST_REMOVE(socket_fds, fi);
-
swrap_dec_refcount(si);
- free(fi);
-
if (swrap_get_refcount(si) > 0) {
goto out;
}
@@ -5869,29 +5821,26 @@ ssize_t writev(int s, const struct iovec *vector, int
count)
static int swrap_close(int fd)
{
- struct socket_info_fd *fi = find_socket_info_fd(fd);
struct socket_info *si = NULL;
int si_index;
int ret;
- if (fi == NULL) {
+ si_index = find_socket_info_index(fd);
+ if (si_index == -1) {
return libc_close(fd);
}
- si_index = fi->si_index;
+ reset_socket_info_index(fd);
+
si = swrap_get_socket_info(si_index);
SWRAP_LOCK(first_free);
SWRAP_LOCK_SI(si);
- SWRAP_DLIST_REMOVE(socket_fds, fi);
-
ret = libc_close(fd);
swrap_dec_refcount(si);
- free(fi);
-
if (swrap_get_refcount(si) > 0) {
/* there are still references left */
goto out;
@@ -5932,25 +5881,18 @@ int close(int fd)
static int swrap_dup(int fd)
{
struct socket_info *si;
- struct socket_info_fd *src_fi, *fi;
+ int dup_fd, idx;
- src_fi = find_socket_info_fd(fd);
- if (src_fi == NULL) {
+ idx = find_socket_info_index(fd);
+ if (idx == -1) {
return libc_dup(fd);
}
- si = swrap_get_socket_info(src_fi->si_index);
-
- fi = (struct socket_info_fd *)calloc(1, sizeof(struct socket_info_fd));
- if (fi == NULL) {
- errno = ENOMEM;
- return -1;
- }
+ si = swrap_get_socket_info(idx);
- fi->fd = libc_dup(fd);
- if (fi->fd == -1) {
+ dup_fd = libc_dup(fd);
+ if (dup_fd == -1) {
int saved_errno = errno;
- free(fi);
errno = saved_errno;
return -1;
}
@@ -5958,15 +5900,15 @@ static int swrap_dup(int fd)
SWRAP_LOCK_SI(si);
swrap_inc_refcount(si);
- fi->si_index = src_fi->si_index;
SWRAP_UNLOCK_SI(si);
/* Make sure we don't have an entry for the fd */
- swrap_remove_stale(fi->fd);
+ swrap_remove_stale(dup_fd);
+
+ set_socket_info_index(dup_fd, idx);
- SWRAP_DLIST_ADD_AFTER(socket_fds, fi, src_fi);
- return fi->fd;
+ return dup_fd;
}
int dup(int fd)
@@ -5981,14 +5923,14 @@ int dup(int fd)
static int swrap_dup2(int fd, int newfd)
{
struct socket_info *si;
- struct socket_info_fd *src_fi, *fi;
+ int dup_fd, idx;
- src_fi = find_socket_info_fd(fd);
- if (src_fi == NULL) {
+ idx = find_socket_info_index(fd);
+ if (idx == -1) {
return libc_dup2(fd, newfd);
}
- si = swrap_get_socket_info(src_fi->si_index);
+ si = swrap_get_socket_info(idx);
if (fd == newfd) {
/*
@@ -6006,16 +5948,9 @@ static int swrap_dup2(int fd, int newfd)
swrap_close(newfd);
}
- fi = (struct socket_info_fd *)calloc(1, sizeof(struct socket_info_fd));
- if (fi == NULL) {
- errno = ENOMEM;
- return -1;
- }
-
- fi->fd = libc_dup2(fd, newfd);
- if (fi->fd == -1) {
+ dup_fd = libc_dup2(fd, newfd);
+ if (dup_fd == -1) {
int saved_errno = errno;
- free(fi);
errno = saved_errno;
return -1;
}
@@ -6023,15 +5958,15 @@ static int swrap_dup2(int fd, int newfd)
SWRAP_LOCK_SI(si);
swrap_inc_refcount(si);
- fi->si_index = src_fi->si_index;
SWRAP_UNLOCK_SI(si);
/* Make sure we don't have an entry for the fd */
- swrap_remove_stale(fi->fd);
+ swrap_remove_stale(dup_fd);
- SWRAP_DLIST_ADD_AFTER(socket_fds, fi, src_fi);
- return fi->fd;
+ set_socket_info_index(dup_fd, idx);
+
+ return dup_fd;
}
int dup2(int fd, int newfd)
@@ -6045,29 +5980,21 @@ int dup2(int fd, int newfd)
static int swrap_vfcntl(int fd, int cmd, va_list va)
{
- struct socket_info_fd *src_fi, *fi;
struct socket_info *si;
- int rc;
+ int rc, dup_fd, idx;
- src_fi = find_socket_info_fd(fd);
- if (src_fi == NULL) {
+ idx = find_socket_info_index(fd);
+ if (idx == -1) {
return libc_vfcntl(fd, cmd, va);
}
- si = swrap_get_socket_info(src_fi->si_index);
+ si = swrap_get_socket_info(idx);
switch (cmd) {
case F_DUPFD:
- fi = (struct socket_info_fd *)calloc(1, sizeof(struct
socket_info_fd));
- if (fi == NULL) {
- errno = ENOMEM;
- return -1;
- }
-
- fi->fd = libc_vfcntl(fd, cmd, va);
- if (fi->fd == -1) {
+ dup_fd = libc_vfcntl(fd, cmd, va);
+ if (dup_fd == -1) {
int saved_errno = errno;
- free(fi);
errno = saved_errno;
return -1;
}
@@ -6075,16 +6002,15 @@ static int swrap_vfcntl(int fd, int cmd, va_list va)
--
Socket Wrapper Repository
