From 929f71ec24b58045319473f050a7f235f726ec78 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Fri, 11 Jan 2019 15:49:44 -0500 Subject: [PATCH 1/1] Fix: leak of sessiond configuration on launch of run-as worker MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The run-as worker is spawned through fork() without using exec*(). This means that any resource allocated by the session daemon before the launch of the run-as worker will be leaked in the run-as worker's process. A callback is added to the run_as launch interface to allow users a chance to clean-up after the fork occurs. This mechanism is fragile as it may not always be easy (or possible) to track all such resources in the future. This makes a strong argument for using a new process image (through exec*()) and forego any such problem at some point. The lttng-consumerd from a similar (and more severe) problem with its own run-as worker. A fix adressing the consumerd's problem follows. Signed-off-by: Jérémie Galarneau --- src/bin/lttng-consumerd/lttng-consumerd.c | 2 +- src/bin/lttng-sessiond/main.c | 22 +++++++++++++++++++++- src/common/runas.c | 19 +++++++++++++++---- src/common/runas.h | 18 +++++++++++++++++- 4 files changed, 54 insertions(+), 7 deletions(-) diff --git a/src/bin/lttng-consumerd/lttng-consumerd.c b/src/bin/lttng-consumerd/lttng-consumerd.c index d97dae2d1..ddd07a142 100644 --- a/src/bin/lttng-consumerd/lttng-consumerd.c +++ b/src/bin/lttng-consumerd/lttng-consumerd.c @@ -411,7 +411,7 @@ int main(int argc, char **argv) set_ulimit(); } - if (run_as_create_worker(argv[0]) < 0) { + if (run_as_create_worker(argv[0], NULL, NULL) < 0) { goto exit_init_data; } diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c index c45280d11..24855b976 100644 --- a/src/bin/lttng-sessiond/main.c +++ b/src/bin/lttng-sessiond/main.c @@ -1336,6 +1336,26 @@ static void destroy_all_sessions_and_wait(void) DBG("Destruction of all sessions completed"); } +static int run_as_worker_post_fork_cleanup(void *data) +{ + struct sessiond_config *sessiond_config = data; + + sessiond_config_fini(sessiond_config); + return 0; +} + +static int launch_run_as_worker(const char *procname) +{ + /* + * Clean-up before forking the run-as worker. Any dynamically + * allocated memory of which the worker is not aware will + * be leaked as the process forks a run-as worker (and performs + * no exec*()). The same would apply to any opened fd. + */ + return run_as_create_worker(procname, run_as_worker_post_fork_cleanup, + &config); +} + /* * main */ @@ -1469,7 +1489,7 @@ int main(int argc, char **argv) } } - if (run_as_create_worker(argv[0]) < 0) { + if (launch_run_as_worker(argv[0]) < 0) { goto exit_create_run_as_worker_cleanup; } diff --git a/src/common/runas.c b/src/common/runas.c index d727cf417..84b3513ee 100644 --- a/src/common/runas.c +++ b/src/common/runas.c @@ -870,7 +870,9 @@ end: } static -int run_as_create_worker_no_lock(const char *procname) +int run_as_create_worker_no_lock(const char *procname, + post_fork_cleanup_cb clean_up_func, + void *clean_up_user_data) { pid_t pid; int i, ret = 0; @@ -915,6 +917,12 @@ int run_as_create_worker_no_lock(const char *procname) reset_sighandler(); set_worker_sighandlers(); + if (clean_up_func) { + if (clean_up_func(clean_up_user_data) < 0) { + ERR("Run-as post-fork clean-up failed, exiting."); + exit(EXIT_FAILURE); + } + } /* Just close, no shutdown. */ if (close(worker->sockpair[0])) { @@ -998,7 +1006,7 @@ int run_as_restart_worker(struct run_as_worker *worker) run_as_destroy_worker(); /* Create a new run_as worker process*/ - ret = run_as_create_worker_no_lock(procname); + ret = run_as_create_worker_no_lock(procname, NULL, NULL); if (ret < 0 ) { ERR("Restarting the worker process failed"); ret = -1; @@ -1214,12 +1222,15 @@ int run_as_extract_sdt_probe_offsets(int fd, const char* provider_name, } LTTNG_HIDDEN -int run_as_create_worker(const char *procname) +int run_as_create_worker(const char *procname, + post_fork_cleanup_cb clean_up_func, + void *clean_up_user_data) { int ret; pthread_mutex_lock(&worker_lock); - ret = run_as_create_worker_no_lock(procname); + ret = run_as_create_worker_no_lock(procname, clean_up_func, + clean_up_user_data); pthread_mutex_unlock(&worker_lock); return ret; } diff --git a/src/common/runas.h b/src/common/runas.h index 5319c26d8..4d5639c4b 100644 --- a/src/common/runas.h +++ b/src/common/runas.h @@ -23,6 +23,21 @@ #include #include +/* + * The run-as process is launched by forking without an exec*() call. This means + * that any resource allocated before the run-as worker is launched should be + * cleaned-up after the fork(). This callback allows the user to perform this + * clean-up. + * + * Note that the callback will _not_ be invoked if the LTTNG_DEBUG_NOCLONE + * environment variable is set as the clean-up is not needed (and may not be + * expected). + * + * A negative return value will cause the run-as process to exit with a non-zero + * value. + */ +typedef int (*post_fork_cleanup_cb)(void *user_data); + LTTNG_HIDDEN int run_as_mkdir_recursive(const char *path, mode_t mode, uid_t uid, gid_t gid); LTTNG_HIDDEN @@ -41,7 +56,8 @@ int run_as_extract_sdt_probe_offsets(int fd, const char *provider_name, const char* probe_name, uid_t uid, gid_t gid, uint64_t **offsets, uint32_t *num_offset); LTTNG_HIDDEN -int run_as_create_worker(const char *procname); +int run_as_create_worker(const char *procname, + post_fork_cleanup_cb clean_up_func, void *clean_up_user_data); LTTNG_HIDDEN void run_as_destroy_worker(void); -- 2.34.1