From 3e74174f36cd90a803c9e560a82288e523997533 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Wei=C3=9F?= Date: Fri, 10 Jan 2025 18:01:12 +0100 Subject: [PATCH] daemon: introduce observer_finisher for delayed free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We introduce an observer finisher which can be registered in the compartment struct. This finisher is called now in the method compartment_notify_observers() after all observers have been executed in the notify observer list. With this it is possible to register a callback which can cleanly destroy the compartment object triggerd by an observer. Previously on container reload the reaload callback destroyed the compartment object, but in the compartment_notify observers() method the compartment object is accessed after each callback, even if it is the last one. This caused the use-after-free error below. ================================================================= ==210==ERROR: AddressSanitizer: heap-use-after-free on address \ 0x5100000011b8 at pc 0x560d9ae6a3a5 bp 0x7fffd1ff8710 sp 0x7fffd1ff8700 READ of size 8 at 0x5100000011b8 thread T0 #0 0x560d9ae6a3a4 in compartment_notify_observers daemon/compartment.c:1677 #1 0x560d9ae6fd49 in compartment_sigchld_handle_helpers daemon/compartment.c:700 #2 0x560d9ae70319 in compartment_sigchld_cb daemon/compartment.c:783 #3 0x560d9af16329 in event_signal_handler common/event.c:780 #4 0x560d9af16329 in event_loop common/event.c:851 #5 0x560d9ae4e6ac in main daemon/main.c:146 #6 0x7fe2bf99b863 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #7 0x7fe2bf99b90a in __libc_start_main_impl ../csu/libc-start.c:389 #8 0x560d9ae509d4 in _start (/usr/sbin/cmld+0x13e9d4) Signed-off-by: Michael Weiß --- daemon/cmld.c | 12 +++++++++++- daemon/compartment.c | 22 ++++++++++++++++++++-- daemon/compartment.h | 6 ++++++ daemon/container.c | 9 +++++++++ daemon/container.h | 3 +++ 5 files changed, 49 insertions(+), 3 deletions(-) diff --git a/daemon/cmld.c b/daemon/cmld.c index 57198d19c..9a5f85460 100644 --- a/daemon/cmld.c +++ b/daemon/cmld.c @@ -586,6 +586,15 @@ cmld_container_config_sync_cb(container_t *container, container_callback_t *cb, mem_free0(c_uuid); } +static void +cmld_reload_delayed_free(void *data) +{ + ASSERT(data); + + container_t *c = data; + container_free(c); +} + container_t * cmld_reload_container(const uuid_t *uuid, const char *path) { @@ -619,7 +628,8 @@ cmld_reload_container(const uuid_t *uuid, const char *path) container_get_name(c_current)); cmld_containers_list = list_remove(cmld_containers_list, c_current); - container_free(c_current); + // delayed free to allow all observers to finish up + container_finish_observers(c_current, cmld_reload_delayed_free, c_current); } DEBUG("Loaded config for container %s", container_get_name(c)); diff --git a/daemon/compartment.c b/daemon/compartment.c index 5a4655c16..222da41d4 100644 --- a/daemon/compartment.c +++ b/daemon/compartment.c @@ -105,6 +105,10 @@ struct compartment { char *debug_log_dir; /* log for output of compartment child */ list_t *observer_list; /* list of function callbacks to be called when the state changes */ + void (*observer_finish_cb)( + void *); /* finisher called after all observers run during state change */ + void *observer_finish_cb_data; /* data pointer as paramenter for finisher callback */ + event_timer_t *stop_timer; /* timer to handle compartment stop timeout */ event_timer_t *start_timer; /* timer to handle a compartment start timeout */ @@ -697,8 +701,8 @@ compartment_sigchld_handle_helpers(compartment_t *compartment, event_signal_t *s compartment_state_t state = compartment->is_rebooting ? COMPARTMENT_STATE_REBOOTING : COMPARTMENT_STATE_STOPPED; - compartment_set_state(compartment, state); compartment->is_rebooting = false; + compartment_set_state(compartment, state); } } @@ -803,6 +807,7 @@ compartment_sigchld_early_cb(UNUSED int signum, event_signal_t *sig, void *data) /* remove the sigchld callback for this early child from the event loop */ event_remove_signal(sig); event_signal_free(sig); + compartment->pid_early = -1; // cleanup if early child returned with an error if ((WIFEXITED(status) && WEXITSTATUS(status)) || WIFSIGNALED(status)) { if (compartment->pid == -1) @@ -810,7 +815,6 @@ compartment_sigchld_early_cb(UNUSED int signum, event_signal_t *sig, void *data) compartment_set_state(compartment, COMPARTMENT_STATE_STOPPED); } - compartment->pid_early = -1; } // reap any open helper child and set state accordingly @@ -1682,6 +1686,11 @@ compartment_notify_observers(compartment_t *compartment) l = l->next; } } + + if (compartment->observer_finish_cb) { + DEBUG("all observers handled, observer_finish_cb is set, execute it!"); + compartment->observer_finish_cb(compartment->observer_finish_cb_data); + } } void @@ -1763,6 +1772,15 @@ compartment_unregister_observer(compartment_t *compartment, compartment_callback } } +void +compartment_finish_observers(compartment_t *compartment, void (*cb)(void *), void *data) +{ + ASSERT(compartment); + + compartment->observer_finish_cb = cb; + compartment->observer_finish_cb_data = data; +} + const char * compartment_get_key(const compartment_t *compartment) { diff --git a/daemon/compartment.h b/daemon/compartment.h index a233d077a..727261814 100644 --- a/daemon/compartment.h +++ b/daemon/compartment.h @@ -399,6 +399,12 @@ compartment_register_observer(compartment_t *compartment, void compartment_unregister_observer(compartment_t *compartment, compartment_callback_t *cb); +/** + * Set observer finishiner callback. + */ +void +compartment_finish_observers(compartment_t *compartment, void (*cb)(void *), void *data); + /** * Gets the compartment's key previously set by compartment_set_key or NULL if no key * has been set. diff --git a/daemon/container.c b/daemon/container.c index a0d169b47..2583b6e6f 100644 --- a/daemon/container.c +++ b/daemon/container.c @@ -313,6 +313,15 @@ container_unregister_observer(container_t *container, container_callback_t *cont mem_free0(container_cb); } +void +container_finish_observers(container_t *container, void (*cb)(void *), void *data) +{ + ASSERT(container); + ASSERT(cb); + + compartment_finish_observers(container->compartment, cb, data); +} + void container_init_env_prepend(container_t *container, char **init_env, size_t init_env_len) { diff --git a/daemon/container.h b/daemon/container.h index da1c33c00..3dd059a13 100644 --- a/daemon/container.h +++ b/daemon/container.h @@ -393,6 +393,9 @@ container_register_observer(container_t *container, void container_unregister_observer(container_t *container, container_callback_t *cb); +void +container_finish_observers(container_t *container, void (*cb)(void *), void *data); + void container_init_env_prepend(container_t *container, char **init_env, size_t init_env_len);