When accepting a connection on a TCP or Unix domain socket we recorded
the peer address in both the thread_data struct and thread-local
storage. But for no reason because it was never used anywhere. Since
we were only allocating a ‘struct sockaddr’ (rather than a ‘struct
sockaddr_storage’) it's likely that some peer addresses would have
been truncated.
Remove all this code, it had no effect.
Plugins that want to get the peer address can use nbdkit_peer_name()
which was added in commit 03a2cc3d766e and doesn't suffer from the
above truncation problem.
(I considered an alternative where we use the saved address to answer
nbdkit_peer_name but since that call will in general be used very
rarely it doesn't make sense to do the extra work for all callers.)
---
server/internal.h | 4 ----
server/sockets.c | 12 ++----------
server/threadlocal.c | 19 -------------------
3 files changed, 2 insertions(+), 33 deletions(-)
diff --git a/server/internal.h b/server/internal.h
index 1f72b01..c31bb34 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -431,10 +431,6 @@ extern void threadlocal_set_name (const char *name)
extern const char *threadlocal_get_name (void);
extern void threadlocal_set_instance_num (size_t instance_num);
extern size_t threadlocal_get_instance_num (void);
-extern void threadlocal_set_sockaddr (const struct sockaddr *addr,
- socklen_t addrlen)
- __attribute__((__nonnull__ (1)));
-/*extern void threadlocal_get_sockaddr ();*/
extern void threadlocal_set_error (int err);
extern int threadlocal_get_error (void);
extern void *threadlocal_buffer (size_t size);
diff --git a/server/sockets.c b/server/sockets.c
index dfaa3ea..3514c69 100644
--- a/server/sockets.c
+++ b/server/sockets.c
@@ -260,8 +260,6 @@ free_listening_sockets (int *socks, size_t nr_socks)
struct thread_data {
int sock;
size_t instance_num;
- struct sockaddr addr;
- socklen_t addrlen;
};
static void *
@@ -274,7 +272,6 @@ start_thread (void *datav)
/* Set thread-local data. */
threadlocal_new_server_thread ();
threadlocal_set_instance_num (data->instance_num);
- threadlocal_set_sockaddr (&data->addr, data->addrlen);
handle_single_connection (data->sock, data->sock);
@@ -299,12 +296,9 @@ accept_connection (int listen_sock)
}
thread_data->instance_num = instance_num++;
- thread_data->addrlen = sizeof thread_data->addr;
again:
#ifdef HAVE_ACCEPT4
- thread_data->sock = accept4 (listen_sock,
- &thread_data->addr, &thread_data->addrlen,
- SOCK_CLOEXEC);
+ thread_data->sock = accept4 (listen_sock, NULL, NULL, SOCK_CLOEXEC);
#else
/* If we were fully parallel, then this function could be accepting
* connections in one thread while another thread could be in a
@@ -316,9 +310,7 @@ accept_connection (int listen_sock)
assert (backend->thread_model (backend) <=
NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS);
lock_request (NULL);
- thread_data->sock = set_cloexec (accept (listen_sock,
- &thread_data->addr,
- &thread_data->addrlen));
+ thread_data->sock = set_cloexec (accept (listen_sock, NULL, NULL));
unlock_request (NULL);
#endif
if (thread_data->sock == -1) {
diff --git a/server/threadlocal.c b/server/threadlocal.c
index a796fce..71cc2d2 100644
--- a/server/threadlocal.c
+++ b/server/threadlocal.c
@@ -55,8 +55,6 @@
struct threadlocal {
char *name; /* Can be NULL. */
size_t instance_num; /* Can be 0. */
- struct sockaddr *addr;
- socklen_t addrlen;
int err;
void *buffer;
size_t buffer_size;
@@ -71,7 +69,6 @@ free_threadlocal (void *threadlocalv)
struct threadlocal *threadlocal = threadlocalv;
free (threadlocal->name);
- free (threadlocal->addr);
free (threadlocal->buffer);
free (threadlocal);
}
@@ -133,22 +130,6 @@ threadlocal_set_instance_num (size_t instance_num)
threadlocal->instance_num = instance_num;
}
-void
-threadlocal_set_sockaddr (const struct sockaddr *addr, socklen_t addrlen)
-{
- struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key);
-
- if (threadlocal) {
- free (threadlocal->addr);
- threadlocal->addr = calloc (1, addrlen);
- if (threadlocal->addr == NULL) {
- perror ("calloc");
- exit (EXIT_FAILURE);
- }
- memcpy (threadlocal->addr, addr, addrlen);
- }
-}
-
const char *
threadlocal_get_name (void)
{
--
2.23.0
Show replies by date
On 9/18/19 4:08 AM, Richard W.M. Jones wrote:
When accepting a connection on a TCP or Unix domain socket we
recorded
the peer address in both the thread_data struct and thread-local
storage. But for no reason because it was never used anywhere. Since
we were only allocating a ‘struct sockaddr’ (rather than a ‘struct
sockaddr_storage’) it's likely that some peer addresses would have
been truncated.
Remove all this code, it had no effect.
Indeed. Looks good.
Plugins that want to get the peer address can use nbdkit_peer_name()
which was added in commit 03a2cc3d766e and doesn't suffer from the
above truncation problem.
(I considered an alternative where we use the saved address to answer
nbdkit_peer_name but since that call will in general be used very
rarely it doesn't make sense to do the extra work for all callers.)
---
server/internal.h | 4 ----
server/sockets.c | 12 ++----------
server/threadlocal.c | 19 -------------------
3 files changed, 2 insertions(+), 33 deletions(-)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization:
qemu.org |
libvirt.org