| Message ID | 20231023022529.15081-2-felix.moessbauer@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Rework socket creation to better integrate with systemd | expand |
On 23.10.23 04:25, 'Felix Moessbauer' via swupdate wrote: > When running with a non default TMPDIR, the IPC sockets were not cleaned > up correctly. The reason for that was an incorrect detection if swupdate > is started as systemd service. In addition, the creation logic and > cleanup logic was spread across multiple files and non consistent. > > This is now fixed by tracking which sockets were created (this can variy > if sockets are passed by systemd) and deallocating these in the atexit > handler. For that, a new network_utils unit is added which provides > functions to globally register sockets that should be cleaned up. > By that, duplicate logic across swupdate is also removed. > > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> > --- > core/Makefile | 1 + > core/network_thread.c | 25 ++++------------- > core/network_utils.c | 60 +++++++++++++++++++++++++++++++++++++++++ > core/progress_thread.c | 19 ------------- > core/swupdate.c | 4 +++ > include/network_utils.h | 23 ++++++++++++++++ > 6 files changed, 93 insertions(+), 39 deletions(-) > create mode 100644 core/network_utils.c > create mode 100644 include/network_utils.h > > diff --git a/core/Makefile b/core/Makefile > index 8cdf42c..945d4c1 100644 > --- a/core/Makefile > +++ b/core/Makefile > @@ -21,6 +21,7 @@ obj-y += swupdate.o \ > state.o \ > syslog.o \ > installer.o \ > + network_utils.o \ > network_thread.o \ > stream_interface.o \ > progress_thread.o \ > diff --git a/core/network_thread.c b/core/network_thread.c > index 05ac14b..936b8cf 100644 > --- a/core/network_thread.c > +++ b/core/network_thread.c > @@ -26,6 +26,7 @@ > #include "util.h" > #include "network_ipc.h" > #include "network_interface.h" > +#include "network_utils.h" > #include "installer.h" > #include "installer_priv.h" > #include "swupdate.h" > @@ -252,7 +253,10 @@ int listener_create(const char *path, int type) > bzero(&servaddr, sizeof(servaddr)); > servaddr.sun_family = AF_LOCAL; > strlcpy(servaddr.sun_path, path, sizeof(servaddr.sun_path) - 1); > - > + if(register_socket_unlink(path) != 0){ > + ERROR("Out of memory, skipping..."); > + return -1; > + } > if (bind(listenfd, (struct sockaddr *) &servaddr, sizeof(servaddr)) < 0) { > close(listenfd); > return -1; > @@ -311,20 +315,6 @@ static void empty_pipe(int fd) > } while (1); > } > > -static void unlink_socket(void) > -{ > -#ifdef CONFIG_SYSTEMD > - if (sd_booted()) { > - /* > - * There were socket fds handed-over by systemd, > - * so don't delete the socket file. > - */ > - return; > - } > -#endif > - unlink(get_ctrl_socket()); > -} > - > static void send_subprocess_reply( > const struct subprocess_msg_elem *const subprocess_msg) > { > @@ -473,11 +463,6 @@ void *network_thread (void *data) > exit(2); > } > > - if (atexit(unlink_socket) != 0) { > - TRACE("Cannot setup socket cleanup on exit, %s won't be unlinked.", > - get_ctrl_socket()); > - } > - > thread_ready(); > do { > clilen = sizeof(cliaddr); > diff --git a/core/network_utils.c b/core/network_utils.c > new file mode 100644 > index 0000000..fd539c1 > --- /dev/null > +++ b/core/network_utils.c > @@ -0,0 +1,60 @@ > +/* > + * (C) Copyright 2023 > + * Felix Moessbauer <felix.moessbauer@siemens.com> > + * > + * SPDX-License-Identifier: GPL-2.0-only > + */ > + > +#include <pthread.h> > +#include <stdlib.h> > +#include <string.h> > +#include <unistd.h> > + > +#include "bsdqueue.h" > +#include "network_utils.h" > +#include "util.h" > + > +struct socket_meta { > + char *path; > + SIMPLEQ_ENTRY(socket_meta) next; > +}; > + > +static pthread_mutex_t sockets_toclose_lock = PTHREAD_MUTEX_INITIALIZER; > +SIMPLEQ_HEAD(self_sockets, socket_meta); > +static struct self_sockets sockets_toclose; > + > +int register_socket_unlink(const char* path){ > + struct socket_meta *socketm = malloc(sizeof(*socketm)); > + if(!socketm){ > + return -1; > + } > + socketm->path = strdup(path); > + if(!socketm->path){ > + free(socketm); > + return -1; > + } > + pthread_mutex_lock(&sockets_toclose_lock); > + SIMPLEQ_INSERT_TAIL(&sockets_toclose, socketm, next); > + pthread_mutex_unlock(&sockets_toclose_lock); > + return 0; > +} > + > +static void unlink_sockets(void) > +{ > + pthread_mutex_lock(&sockets_toclose_lock); > + while(!SIMPLEQ_EMPTY(&sockets_toclose)) { > + struct socket_meta *socketm; > + socketm = SIMPLEQ_FIRST(&sockets_toclose); > + TRACE("unlink socket %s", socketm->path); > + unlink(socketm->path); > + free(socketm->path); > + SIMPLEQ_REMOVE_HEAD(&sockets_toclose, next); > + free(socketm); > + } > + pthread_mutex_unlock(&sockets_toclose_lock); > +} > + > +int init_socket_unlink_handler(void){ > + SIMPLEQ_INIT(&sockets_toclose); > + return atexit(unlink_sockets); > +} > diff --git a/core/progress_thread.c b/core/progress_thread.c > index f579f30..baa3899 100644 > --- a/core/progress_thread.c > +++ b/core/progress_thread.c > @@ -252,20 +252,6 @@ void swupdate_progress_done(const char *info) > pthread_mutex_unlock(&pprog->lock); > } > > -static void unlink_socket(void) > -{ > -#ifdef CONFIG_SYSTEMD > - if (sd_booted()) { > - /* > - * There were socket fds handed-over by systemd, > - * so don't delete the socket file. > - */ > - return; > - } > -#endif > - unlink(get_prog_socket()); > -} > - > void *progress_bar_thread (void __attribute__ ((__unused__)) *data) > { > int listen, connfd; > @@ -284,11 +270,6 @@ void *progress_bar_thread (void __attribute__ ((__unused__)) *data) > exit(2); > } > > - if (atexit(unlink_socket) != 0) { > - TRACE("Cannot setup socket cleanup on exit, %s won't be unlinked.", > - get_prog_socket()); > - } > - > thread_ready(); > do { > clilen = sizeof(cliaddr); > diff --git a/core/swupdate.c b/core/swupdate.c > index 1625ca9..45fa325 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -39,6 +39,7 @@ > #include "mongoose_interface.h" > #include "download_interface.h" > #include "network_ipc.h" > +#include "network_utils.h" > #include "sslapi.h" > #include "suricatta/suricatta.h" > #include "delta_process.h" > @@ -885,6 +886,9 @@ int main(int argc, char **argv) > * Start daemon if just a check is required > * SWUpdate will exit after the check > */ > + if(init_socket_unlink_handler() != 0){ > + TRACE("Cannot setup socket cleanup on exit, sockets won't be unlinked."); > + } > network_daemon = start_thread(network_initializer, &swcfg); > > start_thread(progress_bar_thread, NULL); > diff --git a/include/network_utils.h b/include/network_utils.h > new file mode 100644 > index 0000000..3bd198b > --- /dev/null > +++ b/include/network_utils.h > @@ -0,0 +1,23 @@ > +/* > + * (C) Copyright 2023 > + * Felix Moessbauer <felix.moessbauer@siemens.com> > + * > + * SPDX-License-Identifier: GPL-2.0-only > + */ > +#pragma once > + > +/** > + * \brief initialize unlink functionality for sockets > + * > + * Call this function before \c register_socket_unlink > + * \return 0 on success > + */ > +int init_socket_unlink_handler(void); > + > +/** > + * \brief unlink socket path on exit > + * > + * \note threadsafe > + * \return 0 on success > + */ > +int register_socket_unlink(const char* path); Acked-by: Stefano Babic <stefano.babic@swupdate.org> Best regards, Stefano Babic
diff --git a/core/Makefile b/core/Makefile index 8cdf42c..945d4c1 100644 --- a/core/Makefile +++ b/core/Makefile @@ -21,6 +21,7 @@ obj-y += swupdate.o \ state.o \ syslog.o \ installer.o \ + network_utils.o \ network_thread.o \ stream_interface.o \ progress_thread.o \ diff --git a/core/network_thread.c b/core/network_thread.c index 05ac14b..936b8cf 100644 --- a/core/network_thread.c +++ b/core/network_thread.c @@ -26,6 +26,7 @@ #include "util.h" #include "network_ipc.h" #include "network_interface.h" +#include "network_utils.h" #include "installer.h" #include "installer_priv.h" #include "swupdate.h" @@ -252,7 +253,10 @@ int listener_create(const char *path, int type) bzero(&servaddr, sizeof(servaddr)); servaddr.sun_family = AF_LOCAL; strlcpy(servaddr.sun_path, path, sizeof(servaddr.sun_path) - 1); - + if(register_socket_unlink(path) != 0){ + ERROR("Out of memory, skipping..."); + return -1; + } if (bind(listenfd, (struct sockaddr *) &servaddr, sizeof(servaddr)) < 0) { close(listenfd); return -1; @@ -311,20 +315,6 @@ static void empty_pipe(int fd) } while (1); } -static void unlink_socket(void) -{ -#ifdef CONFIG_SYSTEMD - if (sd_booted()) { - /* - * There were socket fds handed-over by systemd, - * so don't delete the socket file. - */ - return; - } -#endif - unlink(get_ctrl_socket()); -} - static void send_subprocess_reply( const struct subprocess_msg_elem *const subprocess_msg) { @@ -473,11 +463,6 @@ void *network_thread (void *data) exit(2); } - if (atexit(unlink_socket) != 0) { - TRACE("Cannot setup socket cleanup on exit, %s won't be unlinked.", - get_ctrl_socket()); - } - thread_ready(); do { clilen = sizeof(cliaddr); diff --git a/core/network_utils.c b/core/network_utils.c new file mode 100644 index 0000000..fd539c1 --- /dev/null +++ b/core/network_utils.c @@ -0,0 +1,60 @@ +/* + * (C) Copyright 2023 + * Felix Moessbauer <felix.moessbauer@siemens.com> + * + * SPDX-License-Identifier: GPL-2.0-only + */ + +#include <pthread.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> + +#include "bsdqueue.h" +#include "network_utils.h" +#include "util.h" + +struct socket_meta { + char *path; + SIMPLEQ_ENTRY(socket_meta) next; +}; + +static pthread_mutex_t sockets_toclose_lock = PTHREAD_MUTEX_INITIALIZER; +SIMPLEQ_HEAD(self_sockets, socket_meta); +static struct self_sockets sockets_toclose; + +int register_socket_unlink(const char* path){ + struct socket_meta *socketm = malloc(sizeof(*socketm)); + if(!socketm){ + return -1; + } + socketm->path = strdup(path); + if(!socketm->path){ + free(socketm); + return -1; + } + pthread_mutex_lock(&sockets_toclose_lock); + SIMPLEQ_INSERT_TAIL(&sockets_toclose, socketm, next); + pthread_mutex_unlock(&sockets_toclose_lock); + return 0; +} + +static void unlink_sockets(void) +{ + pthread_mutex_lock(&sockets_toclose_lock); + while(!SIMPLEQ_EMPTY(&sockets_toclose)) { + struct socket_meta *socketm; + socketm = SIMPLEQ_FIRST(&sockets_toclose); + TRACE("unlink socket %s", socketm->path); + unlink(socketm->path); + free(socketm->path); + SIMPLEQ_REMOVE_HEAD(&sockets_toclose, next); + free(socketm); + } + pthread_mutex_unlock(&sockets_toclose_lock); +} + +int init_socket_unlink_handler(void){ + SIMPLEQ_INIT(&sockets_toclose); + return atexit(unlink_sockets); +} diff --git a/core/progress_thread.c b/core/progress_thread.c index f579f30..baa3899 100644 --- a/core/progress_thread.c +++ b/core/progress_thread.c @@ -252,20 +252,6 @@ void swupdate_progress_done(const char *info) pthread_mutex_unlock(&pprog->lock); } -static void unlink_socket(void) -{ -#ifdef CONFIG_SYSTEMD - if (sd_booted()) { - /* - * There were socket fds handed-over by systemd, - * so don't delete the socket file. - */ - return; - } -#endif - unlink(get_prog_socket()); -} - void *progress_bar_thread (void __attribute__ ((__unused__)) *data) { int listen, connfd; @@ -284,11 +270,6 @@ void *progress_bar_thread (void __attribute__ ((__unused__)) *data) exit(2); } - if (atexit(unlink_socket) != 0) { - TRACE("Cannot setup socket cleanup on exit, %s won't be unlinked.", - get_prog_socket()); - } - thread_ready(); do { clilen = sizeof(cliaddr); diff --git a/core/swupdate.c b/core/swupdate.c index 1625ca9..45fa325 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -39,6 +39,7 @@ #include "mongoose_interface.h" #include "download_interface.h" #include "network_ipc.h" +#include "network_utils.h" #include "sslapi.h" #include "suricatta/suricatta.h" #include "delta_process.h" @@ -885,6 +886,9 @@ int main(int argc, char **argv) * Start daemon if just a check is required * SWUpdate will exit after the check */ + if(init_socket_unlink_handler() != 0){ + TRACE("Cannot setup socket cleanup on exit, sockets won't be unlinked."); + } network_daemon = start_thread(network_initializer, &swcfg); start_thread(progress_bar_thread, NULL); diff --git a/include/network_utils.h b/include/network_utils.h new file mode 100644 index 0000000..3bd198b --- /dev/null +++ b/include/network_utils.h @@ -0,0 +1,23 @@ +/* + * (C) Copyright 2023 + * Felix Moessbauer <felix.moessbauer@siemens.com> + * + * SPDX-License-Identifier: GPL-2.0-only + */ +#pragma once + +/** + * \brief initialize unlink functionality for sockets + * + * Call this function before \c register_socket_unlink + * \return 0 on success + */ +int init_socket_unlink_handler(void); + +/** + * \brief unlink socket path on exit + * + * \note threadsafe + * \return 0 on success + */ +int register_socket_unlink(const char* path);
When running with a non default TMPDIR, the IPC sockets were not cleaned up correctly. The reason for that was an incorrect detection if swupdate is started as systemd service. In addition, the creation logic and cleanup logic was spread across multiple files and non consistent. This is now fixed by tracking which sockets were created (this can variy if sockets are passed by systemd) and deallocating these in the atexit handler. For that, a new network_utils unit is added which provides functions to globally register sockets that should be cleaned up. By that, duplicate logic across swupdate is also removed. Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> --- core/Makefile | 1 + core/network_thread.c | 25 ++++------------- core/network_utils.c | 60 +++++++++++++++++++++++++++++++++++++++++ core/progress_thread.c | 19 ------------- core/swupdate.c | 4 +++ include/network_utils.h | 23 ++++++++++++++++ 6 files changed, 93 insertions(+), 39 deletions(-) create mode 100644 core/network_utils.c create mode 100644 include/network_utils.h