Fix: sessiond: ODR violation results in memory corruption

Issue observed
==============

Address sanitizer reports the following invalid accesses while running
the test_mi test.

❯ ASAN_OPTIONS=detect_odr_violation=0 lttng-sessiond
=================================================================
==289173==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60400000e280 at pc 0x55cbbe35e2e0 bp 0x7f01672f1550 sp 0x7f01672f1540
WRITE of size 4 at 0x60400000e280 thread T13
    #0 0x55cbbe35e2df in mark_thread_as_ready /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/manage-consumer.cpp:32
    #1 0x55cbbe360160 in thread_consumer_management /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/manage-consumer.cpp:267
    #2 0x55cbbe336ac4 in launch_thread /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/thread.cpp:66
    #3 0x7f01729c15c1 in start_thread (/usr/lib/libc.so.6+0x8d5c1)
    #4 0x7f0172a46583 in __clone (/usr/lib/libc.so.6+0x112583)

0x60400000e280 is located 8 bytes to the right of 40-byte region [0x60400000e250,0x60400000e278)
allocated by thread T7 here:
    #0 0x7f01733b1fb9 in __interceptor_calloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x55cbbe33adf3 in zmalloc_internal ../../../src/common/macros.hpp:60
    #2 0x55cbbe33ae03 in thread_notifiers* zmalloc<thread_notifiers>() ../../../src/common/macros.hpp:89
    #3 0x55cbbe3617f9 in launch_consumer_management_thread(consumer_data*) /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/manage-consumer.cpp:440
    #4 0x55cbbe33cf49 in spawn_consumer_thread /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:188
    lttng#5 0x55cbbe33f7cf in start_consumerd /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:394
    lttng#6 0x55cbbe345713 in process_client_msg /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:1277
    lttng#7 0x55cbbe34d74b in thread_manage_clients /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:2622
    lttng#8 0x55cbbe336ac4 in launch_thread /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/thread.cpp:66
    lttng#9 0x7f01729c15c1 in start_thread (/usr/lib/libc.so.6+0x8d5c1)

Thread T13 created by T7 here:
    #0 0x7f0173353eb7 in __interceptor_pthread_create /usr/src/debug/gcc/libsanitizer/asan/asan_interceptors.cpp:216
    #1 0x55cbbe336f9e in lttng_thread_create(char const*, void* (*)(void*), bool (*)(void*), void (*)(void*), void*) /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/thread.cpp:106
    #2 0x55cbbe3618cc in launch_consumer_management_thread(consumer_data*) /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/manage-consumer.cpp:453
    #3 0x55cbbe33cf49 in spawn_consumer_thread /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:188
    #4 0x55cbbe33f7cf in start_consumerd /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:394
    lttng#5 0x55cbbe345713 in process_client_msg /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:1277
    lttng#6 0x55cbbe34d74b in thread_manage_clients /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:2622
    lttng#7 0x55cbbe336ac4 in launch_thread /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/thread.cpp:66
    lttng#8 0x7f01729c15c1 in start_thread (/usr/lib/libc.so.6+0x8d5c1)

Thread T7 created by T0 here:
    #0 0x7f0173353eb7 in __interceptor_pthread_create /usr/src/debug/gcc/libsanitizer/asan/asan_interceptors.cpp:216
    #1 0x55cbbe336f9e in lttng_thread_create(char const*, void* (*)(void*), bool (*)(void*), void (*)(void*), void*) /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/thread.cpp:106
    #2 0x55cbbe34eebf in launch_client_thread() /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:2756
    #3 0x55cbbe27f31a in main /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/main.cpp:1838
    #4 0x7f017296130f in __libc_start_call_main (/usr/lib/libc.so.6+0x2d30f)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/manage-consumer.cpp:32 in mark_thread_as_ready
Shadow bytes around the buggy address:
  0x0c087fff9c00: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
  0x0c087fff9c10: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
  0x0c087fff9c20: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
  0x0c087fff9c30: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
  0x0c087fff9c40: fa fa fd fd fd fd fd fa fa fa 00 00 00 00 00 fa
=>0x0c087fff9c50:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff9c60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff9c70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff9c80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff9c90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c087fff9ca0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==289173==ABORTING

Cause
=====

The start functions of the various worker threads of the session daemon
are implemented in separate translation units (TU). To make use of the
lttng_thread API, they all define different control structures to
control their shutdown.

Those structures are all named 'thread_notifiers' and are all allocated
using zmalloc<>. The various instances of zmalloc<thread_notifiers> all
end up having the same mangled name (e.g.
_Z7zmallocI16thread_notifiersEPT_v).

At link time, only one instance of zmalloc<thread_notifiers> is kept.
Since those structures all have different layout/sizes, this is
problematic. However, it is an acceptable behaviour according to the ODR
[1].

I first considered making the various memory allocation functions in
macros.hpp 'static' which results in each TU holding the appropriate
specialization of the various functions. While this works, it doesn't
make us ODR-compliant. To make a long story short, a program defining
multiple types sharing the same name, in the same namespace, is
ill-formed.

Another concern is that marking all templated free-functions as static
will eventually result in code bloat.

Solution
========

All structures defined in TUs (but not in a header) are placed in
unnamed namespaces (also called anonymous namespaces) [2].

This results in separate copies of the templated functions being
generated when specialized using a structure in an anonymous
namespace (e.g. _Z7zmallocIN12_GLOBAL__N_116thread_notifiersEEPT_v).

We could have renamed the various `thread_notifiers` structures to give
them different names. However, I found those are not the only structures
sharing a name in different TUs. For instance, the same problem applies
to `struct lttng_index` (index in a stream, index in a map).

I propose we systematically namespace structures defined in TUs in the
future.

This will also save us trouble if those POD structures eventually become
non-POD: we would experience the same "clashes" if those structures had
constructors, for example.

References
==========

[1] https://en.cppreference.com/w/cpp/language/definition
[2] https://en.cppreference.com/w/cpp/language/namespace

Signed-off-by: Jérémie Galarneau <[email protected]>
Change-Id: I867e5a287ad8cf3ada617335bc1a80b800bf0833

src/bin/lttng-crash/lttng-crash.cpp

@@ -98,6 +98,7 @@ enum rb_modes {
 	RING_BUFFER_DISCARD = 1,        /* Discard when buffer full */
 };
 
+namespace {
 struct crash_abi_unknown {
 	uint8_t magic[RB_CRASH_DUMP_ABI_MAGIC_LEN];
 	uint64_t mmap_length;	/* Overall length of crash record */
@@ -177,6 +178,7 @@ struct lttng_crash_layout {
 	uint64_t num_subbuf;	/* Number of sub-buffers for writer */
 	uint32_t mode;		/* Buffer mode: 0: overwrite, 1: discard */
 };
+} /* namespace */
 
 /* Variables */
 static const char *progname;

src/bin/lttng-relayd/sessiond-trace-chunks.cpp

@@ -56,6 +56,7 @@ struct sessiond_trace_chunk_registry {
 	struct cds_lfht *ht;
 };
 
+namespace {
 struct trace_chunk_registry_ht_key {
 	lttng_uuid sessiond_uuid;
 };
@@ -70,6 +71,7 @@ struct trace_chunk_registry_ht_element {
 	struct lttng_trace_chunk_registry *trace_chunk_registry;
 	struct sessiond_trace_chunk_registry *sessiond_trace_chunk_registry;
 };
+} /* namespace */
 
 static
 unsigned long trace_chunk_registry_ht_key_hash(

src/bin/lttng-relayd/tcp_keep_alive.cpp

@@ -61,6 +61,7 @@
 
 #endif /* ! defined (__linux__) && ! defined (__sun__) */
 
+namespace {
 struct tcp_keep_alive_support {
 	/* TCP keep-alive is supported by this platform. */
 	bool supported;
@@ -105,17 +106,18 @@ struct tcp_keep_alive_config {
 	int abort_threshold;
 };
 
-static struct tcp_keep_alive_config the_config = {.enabled = false,
+struct tcp_keep_alive_config the_config = {.enabled = false,
 		.idle_time = -1,
 		.probe_interval = -1,
 		.max_probe_count = -1,
 		.abort_threshold = -1};
 
-static struct tcp_keep_alive_support the_support = {.supported = false,
+struct tcp_keep_alive_support the_support = {.supported = false,
 		.idle_time_supported = false,
 		.probe_interval_supported = false,
 		.max_probe_count_supported = false,
 		.abort_threshold_supported = false};
+} /* namespace */
 
 /*
  * Common parser for string to positive int conversion where the value must be

src/bin/lttng-sessiond/action-executor.cpp

@@ -36,6 +36,20 @@
 #define THREAD_NAME "Action Executor"
 #define MAX_QUEUED_WORK_COUNT 8192
 
+struct action_executor {
+	struct lttng_thread *thread;
+	struct notification_thread_handle *notification_thread_handle;
+	struct {
+		uint64_t pending_count;
+		struct cds_list_head list;
+		pthread_cond_t cond;
+		pthread_mutex_t lock;
+	} work;
+	bool should_quit;
+	uint64_t next_work_item_id;
+};
+
+namespace {
 /*
  * A work item is composed of a dynamic array of sub-items which
  * represent a flattened, and augmented, version of a trigger's actions.
@@ -69,7 +83,6 @@
  * trigger object at the moment of execution, if the trigger is found to be
  * unregistered, the execution is skipped.
  */
-
 struct action_work_item {
 	uint64_t id;
 
@@ -94,19 +107,8 @@ struct action_work_subitem {
 		LTTNG_OPTIONAL(uint64_t) session_id;
 	} context;
 };
+} /* namespace */
 
-struct action_executor {
-	struct lttng_thread *thread;
-	struct notification_thread_handle *notification_thread_handle;
-	struct {
-		uint64_t pending_count;
-		struct cds_list_head list;
-		pthread_cond_t cond;
-		pthread_mutex_t lock;
-	} work;
-	bool should_quit;
-	uint64_t next_work_item_id;
-};
 
 /*
  * Only return non-zero on a fatal error that should shut down the action

src/bin/lttng-sessiond/agent-thread.cpp

@@ -22,6 +22,7 @@
 #include "utils.hpp"
 #include "thread.hpp"
 
+namespace {
 struct thread_notifiers {
 	struct lttng_pipe *quit_pipe;
 	sem_t ready;
@@ -36,15 +37,15 @@ struct agent_protocol_version {
 	unsigned int major, minor;
 };
 
-static int agent_tracing_enabled = -1;
+int agent_tracing_enabled = -1;
 
 /*
  * Note that there is not port here. It's set after this URI is parsed so we
  * can let the user define a custom one. However, localhost is ALWAYS the
  * default listening address.
  */
-static const char *default_reg_uri =
-	"tcp://" DEFAULT_NETWORK_VIEWER_BIND_ADDRESS;
+const char *default_reg_uri = "tcp://" DEFAULT_NETWORK_VIEWER_BIND_ADDRESS;
+} /* namespace */
 
 /*
  * Update agent application using the given socket. This is done just after

src/bin/lttng-sessiond/agent.cpp

@@ -39,6 +39,7 @@ typedef enum lttng_event_rule_status (*event_rule_logging_get_log_level_rule)(
 /*
  * Agent application context representation.
  */
+namespace {
 struct agent_app_ctx {
 	char *provider_name;
 	char *ctx_name;
@@ -49,6 +50,7 @@ struct agent_app_ctx {
 	/* For call_rcu teardown. */
 	struct rcu_head rcu_node;
 };
+} /* namespace */
 
 /*
  * Human readable agent return code.

src/bin/lttng-sessiond/clear.cpp

@@ -20,9 +20,11 @@
 #include "kernel.hpp"
 #include "cmd.hpp"
 
+namespace {
 struct cmd_clear_session_reply_context {
 	int reply_sock_fd;
 };
+} /* namespace */
 
 static
 void cmd_clear_session_reply(const struct ltt_session *session,

src/bin/lttng-sessiond/client.cpp

@@ -45,13 +45,15 @@
 #include "testpoint.hpp"
 #include "utils.hpp"
 
-static bool is_root;
+namespace {
+bool is_root;
 
-static struct thread_state {
+struct thread_state {
 	sem_t ready;
 	bool running;
 	int client_sock;
 } thread_state;
+} /* namespace */
 
 static void set_thread_status(bool running)
 {

src/bin/lttng-sessiond/cmd.cpp

@@ -74,6 +74,9 @@
 /* Sleep for 100ms between each check for the shm path's deletion. */
 #define SESSION_DESTROY_SHM_PATH_CHECK_DELAY_US 100000
 
+static enum lttng_error_code wait_on_path(void *path);
+
+namespace {
 struct cmd_destroy_session_reply_context {
 	int reply_sock_fd;
 	bool implicit_rotation_on_destroy;
@@ -84,15 +87,13 @@ struct cmd_destroy_session_reply_context {
 	enum lttng_error_code destruction_status;
 };
 
-static enum lttng_error_code wait_on_path(void *path);
-
 /*
  * Command completion handler that is used by the destroy command
  * when a session that has a non-default shm_path is being destroyed.
  *
  * See comment in cmd_destroy_session() for the rationale.
  */
-static struct destroy_completion_handler {
+struct destroy_completion_handler {
 	struct cmd_completion_handler handler;
 	char shm_path[member_sizeof(struct ltt_session, shm_path)];
 } destroy_completion_handler = {
@@ -103,17 +104,17 @@ static struct destroy_completion_handler {
 	.shm_path = { 0 },
 };
 
-static struct cmd_completion_handler *current_completion_handler;
-
 /*
  * Used to keep a unique index for each relayd socket created where this value
  * is associated with streams on the consumer so it can match the right relayd
  * to send to. It must be accessed with the relayd_net_seq_idx_lock
  * held.
  */
-static pthread_mutex_t relayd_net_seq_idx_lock = PTHREAD_MUTEX_INITIALIZER;
-static uint64_t relayd_net_seq_idx;
+pthread_mutex_t relayd_net_seq_idx_lock = PTHREAD_MUTEX_INITIALIZER;
+uint64_t relayd_net_seq_idx;
+} /* namespace */
 
+static struct cmd_completion_handler *current_completion_handler;
 static int validate_ust_event_name(const char *);
 static int cmd_enable_event_internal(struct ltt_session *session,
 		const struct lttng_domain *domain,

src/bin/lttng-sessiond/dispatch.cpp

@@ -21,12 +21,14 @@
 #include "lttng-sessiond.hpp"
 #include "thread.hpp"
 
+namespace {
 struct thread_notifiers {
 	struct ust_cmd_queue *ust_cmd_queue;
 	int apps_cmd_pipe_write_fd;
 	int apps_cmd_notify_pipe_write_fd;
 	int dispatch_thread_exit;
 };
+} /* namespace */
 
 /*
  * For each tracing session, update newly registered apps. The session list

src/bin/lttng-sessiond/event-notifier-error-accounting.cpp

@@ -25,6 +25,7 @@
 
 #define ERROR_COUNTER_INDEX_HT_INITIAL_SIZE 16
 
+namespace {
 struct index_ht_entry {
 	struct lttng_ht_node_u64 node;
 	uint64_t error_counter_index;
@@ -53,10 +54,10 @@ struct kernel_error_accounting_entry {
 	int error_counter_fd;
 };
 
-static struct kernel_error_accounting_entry kernel_error_accounting_entry;
+struct kernel_error_accounting_entry kernel_error_accounting_entry;
 
 /* Hashtable mapping uid to error_account_entry. */
-static struct lttng_ht *error_counter_uid_ht;
+struct lttng_ht *error_counter_uid_ht;
 
 struct error_accounting_state {
 	struct lttng_index_allocator *index_allocator;
@@ -65,8 +66,9 @@ struct error_accounting_state {
 	uint64_t number_indices;
 };
 
-static struct error_accounting_state ust_state;
-static struct error_accounting_state kernel_state;
+struct error_accounting_state ust_state;
+struct error_accounting_state kernel_state;
+} /* namespace */
 
 static inline void get_trigger_info_for_log(const struct lttng_trigger *trigger,
 		const char **trigger_name,
@@ -113,12 +115,14 @@ const char *error_accounting_status_str(
 }
 
 #ifdef HAVE_LIBLTTNG_UST_CTL
+namespace {
 struct event_notifier_counter {
 	pthread_mutex_t lock;
 	long count;
 };
 
-static struct event_notifier_counter the_event_notifier_counter;
+struct event_notifier_counter the_event_notifier_counter;
+} /* namespace */
 
 static void free_ust_error_accounting_entry(struct rcu_head *head)
 {

src/bin/lttng-sessiond/health.cpp

@@ -17,10 +17,12 @@
 #include "utils.hpp"
 #include "thread.hpp"
 
+namespace {
 struct thread_notifiers {
 	struct lttng_pipe *quit_pipe;
 	sem_t ready;
 };
+} /* namespace */
 
 static
 void mark_thread_as_ready(struct thread_notifiers *notifiers)

src/bin/lttng-sessiond/manage-apps.cpp

@@ -13,10 +13,12 @@
 #include "utils.hpp"
 #include "thread.hpp"
 
+namespace {
 struct thread_notifiers {
 	struct lttng_pipe *quit_pipe;
 	int apps_cmd_pipe_read_fd;
 };
+} /* namespace */
 
 static void cleanup_application_management_thread(void *data)
 {

src/bin/lttng-sessiond/manage-consumer.cpp

@@ -19,12 +19,14 @@
 #include "thread.hpp"
 #include "ust-consumer.hpp"
 
+namespace {
 struct thread_notifiers {
 	struct lttng_pipe *quit_pipe;
 	struct consumer_data *consumer_data;
 	sem_t ready;
 	int initialization_result;
 };
+} /* namespace */
 
 static void mark_thread_as_ready(struct thread_notifiers *notifiers)
 {

src/bin/lttng-sessiond/manage-kernel.cpp

@@ -18,10 +18,12 @@
 #include "kernel.hpp"
 #include "kernel-consumer.hpp"
 
+namespace {
 struct thread_notifiers {
 	struct lttng_pipe *quit_pipe;
 	int kernel_poll_pipe_read_fd;
 };
+} /* namespace */
 
 /*
  * Update the kernel poll set of all channel fd available over all tracing

src/bin/lttng-sessiond/notification-thread-events.cpp

@@ -60,12 +60,6 @@ enum lttng_object_type {
 	LTTNG_OBJECT_TYPE_SESSION,
 };
 
-struct lttng_trigger_list_element {
-	/* No ownership of the trigger object is assumed. */
-	struct lttng_trigger *trigger;
-	struct cds_list_head node;
-};
-
 struct lttng_channel_trigger_list {
 	struct channel_key channel_key;
 	/* List of struct lttng_trigger_list_element. */
@@ -117,6 +111,13 @@ struct lttng_session_trigger_list {
 	struct rcu_head rcu_node;
 };
 
+namespace {
+struct lttng_trigger_list_element {
+	/* No ownership of the trigger object is assumed. */
+	struct lttng_trigger *trigger;
+	struct cds_list_head node;
+};
+
 struct lttng_trigger_ht_element {
 	struct lttng_trigger *trigger;
 	struct cds_lfht_node node;
@@ -140,6 +141,7 @@ struct channel_state_sample {
 	/* call_rcu delayed reclaim. */
 	struct rcu_head rcu_node;
 };
+} /* namespace */
 
 static unsigned long hash_channel_key(struct channel_key *key);
 static int evaluate_buffer_condition(const struct lttng_condition *condition,

src/bin/lttng-sessiond/notify-apps.cpp

@@ -18,10 +18,12 @@
 #include "utils.hpp"
 #include "thread.hpp"
 
+namespace {
 struct thread_notifiers {
 	struct lttng_pipe *quit_pipe;
 	int apps_cmd_notify_pipe_read_fd;
 };
+} /* namespace */
 
 /*
  * This thread manage application notify communication.