From 6f626d284c2bb02ae8980da6e8053e191d604286 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Fri, 10 Mar 2017 18:08:25 -0500 Subject: [PATCH] Fix: race between lttng-ust getenv() and application setenv() The LTTng-UST listener threads invoke getenv(), which can cause issues if the application issues setenv() concurrently. This is a legitimate use by the application because it may have a single thread and not be aware that it runs with liblttng-ust. Fix this by keeping our own environment variable table for the variables we care about. Initialize this table within the lttng-ust library constructor, when we don't race with the application. As this thread shows: https://sourceware.org/bugzilla/show_bug.cgi?id=5069#c10 getenv() does _not_ appear to be thread-safe if an application uses setenv() or putenv(). Signed-off-by: Mathieu Desnoyers --- liblttng-ust-ctl/ustctl.c | 1 + liblttng-ust/Makefile.am | 2 + liblttng-ust/getenv.c | 98 ++++++++++++++++++++++++++++++ liblttng-ust/getenv.h | 29 +++------ liblttng-ust/lttng-clock.c | 2 +- liblttng-ust/lttng-getcpu.c | 2 +- liblttng-ust/lttng-ust-comm.c | 9 +-- liblttng-ust/lttng-ust-statedump.c | 5 +- snprintf/core.c | 5 ++ 9 files changed, 126 insertions(+), 27 deletions(-) create mode 100644 liblttng-ust/getenv.c diff --git a/liblttng-ust-ctl/ustctl.c b/liblttng-ust-ctl/ustctl.c index 2af79147..40677582 100644 --- a/liblttng-ust-ctl/ustctl.c +++ b/liblttng-ust-ctl/ustctl.c @@ -2205,6 +2205,7 @@ static __attribute__((constructor)) void ustctl_init(void) { init_usterr(); + lttng_ust_getenv_init(); /* Needs init_usterr() to be completed. */ lttng_ust_clock_init(); lttng_ring_buffer_metadata_client_init(); lttng_ring_buffer_client_overwrite_init(); diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am index 7edac266..e79591a8 100644 --- a/liblttng-ust/Makefile.am +++ b/liblttng-ust/Makefile.am @@ -70,6 +70,8 @@ liblttng_ust_support_la_SOURCES = \ lttng-tracer.h \ lttng-tracer-core.h \ ust-core.c \ + getenv.h \ + getenv.c \ lttng-ust-dynamic-type.c \ lttng-rb-clients.h \ lttng-ring-buffer-client.h \ diff --git a/liblttng-ust/getenv.c b/liblttng-ust/getenv.c new file mode 100644 index 00000000..b4dc73a0 --- /dev/null +++ b/liblttng-ust/getenv.c @@ -0,0 +1,98 @@ +/* + * Copyright (C) 2017 - Mathieu Desnoyers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; only + * version 2.1 of the License. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include +#include +#include +#include +#include +#include +#include "getenv.h" + +enum lttng_env_secure { + LTTNG_ENV_SECURE, + LTTNG_ENV_NOT_SECURE, +}; + +struct lttng_env { + const char *key; + enum lttng_env_secure secure; + char *value; +}; + +static struct lttng_env lttng_env[] = { + /* + * LTTNG_UST_DEBUG is used directly by snprintf, because it + * needs to be already set for ERR() used in + * lttng_ust_getenv_init(). + */ + { "LTTNG_UST_DEBUG", LTTNG_ENV_NOT_SECURE, NULL, }, + + /* Env. var. which can be used in setuid/setgid executables. */ + { "LTTNG_UST_WITHOUT_BADDR_STATEDUMP", LTTNG_ENV_NOT_SECURE, NULL, }, + { "LTTNG_UST_REGISTER_TIMEOUT", LTTNG_ENV_NOT_SECURE, NULL, }, + + /* Env. var. which are not fetched in setuid/setgid executables. */ + { "LTTNG_UST_CLOCK_PLUGIN", LTTNG_ENV_SECURE, NULL, }, + { "LTTNG_UST_GETCPU_PLUGIN", LTTNG_ENV_SECURE, NULL, }, + { "LTTNG_UST_BLOCKING_RETRY_TIMEOUT", LTTNG_ENV_SECURE, NULL, }, + { "HOME", LTTNG_ENV_SECURE, NULL, }, + { "LTTNG_HOME", LTTNG_ENV_SECURE, NULL, }, +}; + +static +int lttng_is_setuid_setgid(void) +{ + return geteuid() != getuid() || getegid() != getgid(); +} + +char *lttng_getenv(const char *name) +{ + size_t i; + struct lttng_env *e; + bool found = false; + + for (i = 0; i < LTTNG_ARRAY_SIZE(lttng_env); i++) { + e = <tng_env[i]; + + if (strcmp(e->key, name) == 0) { + found = true; + break; + } + } + if (!found) { + return NULL; + } + return e->value; +} + +void lttng_ust_getenv_init(void) +{ + size_t i; + + for (i = 0; i < LTTNG_ARRAY_SIZE(lttng_env); i++) { + struct lttng_env *e = <tng_env[i]; + + if (e->secure == LTTNG_ENV_SECURE && lttng_is_setuid_setgid()) { + ERR("Getting environment variable '%s' from setuid/setgid binary refused for security reasons.", + e->key); + continue; + } + e->value = getenv(e->key); + } +} diff --git a/liblttng-ust/getenv.h b/liblttng-ust/getenv.h index 05864fb8..14d70506 100644 --- a/liblttng-ust/getenv.h +++ b/liblttng-ust/getenv.h @@ -19,26 +19,17 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ -#include -#include -#include -#include +/* + * Always add the lttng-ust environment variables to lttng_getenv() + * infrastructure rather than using getenv() directly from lttng-ust. + * This ensures that we don't trigger races between getenv() invoked by + * lttng-ust listener threads invoked concurrently with setenv() called + * by an otherwise single-threaded application thread. (the application + * is not aware that it runs with lttng-ust) + */ -static inline -int lttng_is_setuid_setgid(void) -{ - return geteuid() != getuid() || getegid() != getgid(); -} +char *lttng_getenv(const char *name); -static inline -char *lttng_secure_getenv(const char *name) -{ - if (lttng_is_setuid_setgid()) { - ERR("Getting environment variable '%s' from setuid/setgid binary refused for security reasons.", - name); - return NULL; - } - return getenv(name); -} +void lttng_ust_getenv_init(void); #endif /* _COMPAT_GETENV_H */ diff --git a/liblttng-ust/lttng-clock.c b/liblttng-ust/lttng-clock.c index b24ff37c..877b5d61 100644 --- a/liblttng-ust/lttng-clock.c +++ b/liblttng-ust/lttng-clock.c @@ -102,7 +102,7 @@ void lttng_ust_clock_init(void) if (clock_handle) return; - libname = lttng_secure_getenv("LTTNG_UST_CLOCK_PLUGIN"); + libname = lttng_getenv("LTTNG_UST_CLOCK_PLUGIN"); if (!libname) return; clock_handle = dlopen(libname, RTLD_NOW); diff --git a/liblttng-ust/lttng-getcpu.c b/liblttng-ust/lttng-getcpu.c index 7b608ef9..7cc9faa8 100644 --- a/liblttng-ust/lttng-getcpu.c +++ b/liblttng-ust/lttng-getcpu.c @@ -47,7 +47,7 @@ void lttng_ust_getcpu_init(void) if (getcpu_handle) return; - libname = lttng_secure_getenv("LTTNG_UST_GETCPU_PLUGIN"); + libname = lttng_getenv("LTTNG_UST_GETCPU_PLUGIN"); if (!libname) return; getcpu_handle = dlopen(libname, RTLD_NOW); diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c index 651d2aaa..43728ff4 100644 --- a/liblttng-ust/lttng-ust-comm.c +++ b/liblttng-ust/lttng-ust-comm.c @@ -366,11 +366,11 @@ const char *get_lttng_home_dir(void) { const char *val; - val = (const char *) lttng_secure_getenv("LTTNG_HOME"); + val = (const char *) lttng_getenv("LTTNG_HOME"); if (val != NULL) { return val; } - return (const char *) lttng_secure_getenv("HOME"); + return (const char *) lttng_getenv("HOME"); } /* @@ -471,7 +471,7 @@ long get_timeout(void) long constructor_delay_ms = LTTNG_UST_DEFAULT_CONSTRUCTOR_TIMEOUT_MS; if (!got_timeout_env) { - str_timeout = getenv("LTTNG_UST_REGISTER_TIMEOUT"); + str_timeout = lttng_getenv("LTTNG_UST_REGISTER_TIMEOUT"); got_timeout_env = 1; } if (str_timeout) @@ -538,7 +538,7 @@ static void get_blocking_retry_timeout(void) { const char *str_blocking_retry_timeout = - lttng_secure_getenv("LTTNG_UST_BLOCKING_RETRY_TIMEOUT"); + lttng_getenv("LTTNG_UST_BLOCKING_RETRY_TIMEOUT"); if (str_blocking_retry_timeout) { long timeout = strtol(str_blocking_retry_timeout, NULL, 10); @@ -1679,6 +1679,7 @@ void __attribute__((constructor)) lttng_ust_init(void) * sessiond before the init functions are completed). */ init_usterr(); + lttng_ust_getenv_init(); /* Needs init_usterr() to be completed. */ init_tracepoint(); lttng_ust_init_fd_tracker(); lttng_ust_clock_init(); diff --git a/liblttng-ust/lttng-ust-statedump.c b/liblttng-ust/lttng-ust-statedump.c index 171cbaec..efa8a55a 100644 --- a/liblttng-ust/lttng-ust-statedump.c +++ b/liblttng-ust/lttng-ust-statedump.c @@ -34,6 +34,7 @@ #include "lttng-tracer-core.h" #include "lttng-ust-statedump.h" #include "jhash.h" +#include "getenv.h" #define TRACEPOINT_DEFINE #include "ust_lib.h" /* Only define. */ @@ -548,7 +549,7 @@ void lttng_ust_dl_update(void *ip) { struct dl_iterate_data data; - if (getenv("LTTNG_UST_WITHOUT_BADDR_STATEDUMP")) + if (lttng_getenv("LTTNG_UST_WITHOUT_BADDR_STATEDUMP")) return; /* @@ -582,7 +583,7 @@ void lttng_ust_dl_update(void *ip) static int do_baddr_statedump(void *owner) { - if (getenv("LTTNG_UST_WITHOUT_BADDR_STATEDUMP")) + if (lttng_getenv("LTTNG_UST_WITHOUT_BADDR_STATEDUMP")) return 0; lttng_ust_dl_update(LTTNG_UST_CALLER_IP()); ust_dl_table_statedump(owner); diff --git a/snprintf/core.c b/snprintf/core.c index 966b5887..c017b832 100644 --- a/snprintf/core.c +++ b/snprintf/core.c @@ -27,6 +27,11 @@ void init_usterr(void) char *ust_debug; if (ust_loglevel == UST_LOGLEVEL_UNKNOWN) { + /* + * This getenv is not part of lttng_getenv() because it + * is required to print ERR() performed during getenv + * initialization. + */ ust_debug = getenv("LTTNG_UST_DEBUG"); if (ust_debug) ust_loglevel = UST_LOGLEVEL_DEBUG; -- 2.34.1