[lttng-dev] [PATCH lttng-ust v2] Fix: destructors do not run on probe provider dlclose

[Mathieu Desnoyers]

----- On Feb 22, 2018, at 3:41 PM, Francis Deslauriers francis.deslauriers at efficios.com wrote:

> Calling dlclose on the probe provider library that first loaded
> __tracepoints__disable_destructors in the symbol table does not
> unregister the probes from the callsites as the destructors are not
> executed.
> 
> The __tracepoints__disable_destructors weak symbol is exposed by probe
> providers, liblttng-ust.so and liblttng-ust-tracepoint.so libraries. If
> a probe provider is loaded first into the address space, its definition
> is binded to the symbol. All the subsequent loaded libraries using the


binded -> bound

> symbol will use the existing definition of the symbol, thus creating a
> situation where liblttng-ust.so or liblttng-ust-tracepoint.so depend on
> the probe provider library.
> 
> This prevents the dynamic loader from unloading the library as it is
> still in use by other libraries. Because of this, the execution of its
> destructors and the unregistration of the probes is postponed.
> 
> To overcome this issue, we no longer expose this symbol in the
> tracepoint.h file to remove the explicit dependency of the probe
> provider on the symbol. We instead use the existing dlopen handle on
> liblttng-ust-tracepoint.so to get handles on getter and setter functions
> for this value.
> 
> Signed-off-by: Francis Deslauriers <francis.deslauriers at efficios.com>
> ---
> include/lttng/tracepoint.h | 82 +++++++++++++++++++++++++++++++++++-----------
> liblttng-ust/tracepoint.c  | 35 ++++++++++++++++++++
> 2 files changed, 98 insertions(+), 19 deletions(-)
> 
> diff --git a/include/lttng/tracepoint.h b/include/lttng/tracepoint.h
> index 39f2c4d..d285347 100644
> --- a/include/lttng/tracepoint.h
> +++ b/include/lttng/tracepoint.h
> @@ -226,21 +226,6 @@ struct lttng_ust_tracepoint_dlopen {
> extern struct lttng_ust_tracepoint_dlopen tracepoint_dlopen;
> extern struct lttng_ust_tracepoint_dlopen *tracepoint_dlopen_ptr;
> 
> -/* Disable tracepoint destructors. */
> -int __tracepoints__disable_destructors __attribute__((weak));

I think we may want to keep that symbol around, but move it into
tracepoint.c perhaps ? This would be for backward compatibility with
applications previously built against older lttng-ust.

We also need to read both the old symbol and new refcount in the
state getter.

> -
> -/*
> - * Programs that have threads that survive after they exit, and
> - * therefore call library destructors, should disable the tracepoint
> - * destructors by calling tracepoint_disable_destructors(). This will
> - * leak the tracepoint instrumentation library shared object, leaving
> - * its teardown to the operating system process teardown.
> - */
> -static inline void tracepoint_disable_destructors(void)
> -{
> -	__tracepoints__disable_destructors = 1;
> -}
> -
> /*
>  * These weak symbols, the constructor, and destructor take care of
>  * registering only _one_ instance of the tracepoints per shared-ojbect
> @@ -265,6 +250,47 @@ struct lttng_ust_tracepoint_dlopen tracepoint_dlopen
> struct lttng_ust_tracepoint_dlopen *tracepoint_dlopen_ptr
> 	__attribute__((weak, visibility("hidden")));
> 
> +/*
> + * Tracepoint dynamic linkage handling (callbacks). Hidden visibility: shared
> + * across objects in a module/main executable. The callbacks are used to
> + * control and check if the destructors should be executed.
> + */
> +struct lttng_ust_tracepoint_destructors_syms {
> +	void (*tracepoint_enable_destructors)(void);
> +	void (*tracepoint_disable_destructors)(void);
> +	int (*tracepoint_get_destructors_state)(void);
> +};
> +
> +extern struct lttng_ust_tracepoint_destructors_syms
> tracepoint_destructors_syms;
> +extern struct lttng_ust_tracepoint_destructors_syms
> *tracepoint_destructors_syms_ptr;
> +
> +struct lttng_ust_tracepoint_destructors_syms tracepoint_destructors_syms
> +	__attribute__((weak, visibility("hidden")));
> +struct lttng_ust_tracepoint_destructors_syms *tracepoint_destructors_syms_ptr
> +	__attribute__((weak, visibility("hidden")));
> +
> +static inline void tracepoint_disable_destructors(void)
> +{
> +	if (!tracepoint_dlopen_ptr)
> +		tracepoint_dlopen_ptr = &tracepoint_dlopen;
> +	if (!tracepoint_destructors_syms_ptr)
> +		tracepoint_destructors_syms_ptr = &tracepoint_destructors_syms;
> +	if (tracepoint_dlopen_ptr->liblttngust_handle
> +			&& tracepoint_destructors_syms_ptr->tracepoint_disable_destructors)
> +		tracepoint_destructors_syms_ptr->tracepoint_disable_destructors();
> +}
> +
> +static inline void tracepoint_enable_destructors(void)
> +{
> +	if (!tracepoint_dlopen_ptr)
> +		tracepoint_dlopen_ptr = &tracepoint_dlopen;
> +	if (!tracepoint_destructors_syms_ptr)
> +		tracepoint_destructors_syms_ptr = &tracepoint_destructors_syms;
> +	if (tracepoint_dlopen_ptr->liblttngust_handle
> +			&& tracepoint_destructors_syms_ptr->tracepoint_enable_destructors)
> +		tracepoint_destructors_syms_ptr->tracepoint_enable_destructors();
> +}
> +
> #ifndef _LGPL_SOURCE
> static inline void lttng_ust_notrace
> __tracepoint__init_urcu_sym(void);
> @@ -335,8 +361,11 @@ __tracepoints__destroy(void)
> 		return;
> 	if (!tracepoint_dlopen_ptr)
> 		tracepoint_dlopen_ptr = &tracepoint_dlopen;
> -	if (!__tracepoints__disable_destructors
> -			&& tracepoint_dlopen_ptr->liblttngust_handle
> +	if (!tracepoint_destructors_syms_ptr)
> +		tracepoint_destructors_syms_ptr = &tracepoint_destructors_syms;
> +	if (tracepoint_dlopen_ptr->liblttngust_handle
> +			&& tracepoint_destructors_syms_ptr->tracepoint_get_destructors_state
> +			&& tracepoint_destructors_syms_ptr->tracepoint_get_destructors_state()
> 			&& !__tracepoint_ptrs_registered) {
> 		ret = dlclose(tracepoint_dlopen_ptr->liblttngust_handle);
> 		if (ret) {
> @@ -415,6 +444,8 @@ __tracepoints__ptrs_init(void)
> 			dlopen("liblttng-ust-tracepoint.so.0", RTLD_NOW | RTLD_GLOBAL);
> 	if (!tracepoint_dlopen_ptr->liblttngust_handle)
> 		return;
> +	if (!tracepoint_destructors_syms_ptr)
> +		tracepoint_destructors_syms_ptr = &tracepoint_destructors_syms;
> 	tracepoint_dlopen_ptr->tracepoint_register_lib =
> 		URCU_FORCE_CAST(int (*)(struct lttng_ust_tracepoint * const *, int),
> 				dlsym(tracepoint_dlopen_ptr->liblttngust_handle,
> @@ -423,6 +454,18 @@ __tracepoints__ptrs_init(void)
> 		URCU_FORCE_CAST(int (*)(struct lttng_ust_tracepoint * const *),
> 				dlsym(tracepoint_dlopen_ptr->liblttngust_handle,
> 					"tracepoint_unregister_lib"));
> +	tracepoint_destructors_syms_ptr->tracepoint_enable_destructors =
> +		URCU_FORCE_CAST(void (*)(void),
> +				dlsym(tracepoint_dlopen_ptr->liblttngust_handle,
> +					"tp_enable_destructors"));
> +	tracepoint_destructors_syms_ptr->tracepoint_disable_destructors =
> +		URCU_FORCE_CAST(void (*)(void),
> +				dlsym(tracepoint_dlopen_ptr->liblttngust_handle,
> +					"tp_disable_destructors"));
> +	tracepoint_destructors_syms_ptr->tracepoint_get_destructors_state =
> +		URCU_FORCE_CAST(int (*)(void),
> +				dlsym(tracepoint_dlopen_ptr->liblttngust_handle,
> +					"tp_get_destructors_state"));
> 	__tracepoint__init_urcu_sym();
> 	if (tracepoint_dlopen_ptr->tracepoint_register_lib) {
> 		tracepoint_dlopen_ptr->tracepoint_register_lib(__start___tracepoints_ptrs,
> @@ -444,8 +487,9 @@ __tracepoints__ptrs_destroy(void)
> 		tracepoint_dlopen_ptr = &tracepoint_dlopen;
> 	if (tracepoint_dlopen_ptr->tracepoint_unregister_lib)
> 		tracepoint_dlopen_ptr->tracepoint_unregister_lib(__start___tracepoints_ptrs);
> -	if (!__tracepoints__disable_destructors
> -			&& tracepoint_dlopen_ptr->liblttngust_handle
> +	if (tracepoint_dlopen_ptr->liblttngust_handle
> +			&& tracepoint_destructors_syms_ptr->tracepoint_get_destructors_state
> +			&& tracepoint_destructors_syms_ptr->tracepoint_get_destructors_state()
> 			&& !__tracepoint_ptrs_registered) {
> 		ret = dlclose(tracepoint_dlopen_ptr->liblttngust_handle);
> 		if (ret) {
> diff --git a/liblttng-ust/tracepoint.c b/liblttng-ust/tracepoint.c
> index 2c25d46..2e03801 100644
> --- a/liblttng-ust/tracepoint.c
> +++ b/liblttng-ust/tracepoint.c
> @@ -53,6 +53,11 @@ struct {
> /* Set to 1 to enable tracepoint debug output */
> static const int tracepoint_debug;
> static int initialized;
> +
> +/*
> + * If tracepoint_destructors_refcount > 0, tracepoint destructors are enabled.
> + */
> +static int tracepoint_destructors_refcount = 1;
> static void (*new_tracepoint_cb)(struct lttng_ust_tracepoint *);
> 
> /*
> @@ -983,3 +988,33 @@ void *tp_rcu_dereference_sym_bp(void *p)
> {
> 	return rcu_dereference_bp(p);
> }
> +
> +/*
> + * Programs that have threads that survive after they exit, and therefore call
> + * library destructors, should disable the tracepoint destructors by calling
> + * tp_disable_destructors(). This will leak the tracepoint
> + * instrumentation library shared object, leaving its teardown to the operating
> + * system process teardown.
> + *
> + * To access and/or modify this value, users need to use a combination of
> + * dlopen(3) and dlsym(3) to get an handle on the
> + * tp_{enable,disable}_destructors and tp_get_destructors_state symbols below.
> + */
> +void tp_disable_destructors(void)
> +{
> +	uatomic_dec(&tracepoint_destructors_refcount);
> +}
> +
> +void tp_enable_destructors(void)
> +{
> +	uatomic_inc(&tracepoint_destructors_refcount);
> +}
> +
> +/*
> + * Returns 1 if the destructors are enabled and should be executed.
> + * Returns 0 if the destructors are disabled.
> + */
> +int tp_get_destructors_state(void)
> +{
> +	return !!uatomic_read(&tracepoint_destructors_refcount);

return uatomic_read(&tracepoint_destructors_refcount) > 0);

because it can be negative.

We should also sample the state of the __tracepoints__disable_destructors symbol here,
e.g.

return !__tracepoints__disable_destructors && uatomic_read(&tracepoint_destructors_refcount) > 0);

Thanks,

Mathieu

> +}
> --
> 2.7.4

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com