From: Jérémie Galarneau Date: Thu, 14 Apr 2022 23:01:25 +0000 (-0400) Subject: Fix: sessiond: ODR violation results in memory corruption X-Git-Url: https://git.lttng.org/?a=commitdiff_plain;h=f149493493fbd8a3efa4748832c03278c96c38ca;hp=f149493493fbd8a3efa4748832c03278c96c38ca;p=lttng-tools.git 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() ../../../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 #5 0x55cbbe33f7cf in start_consumerd /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:394 #6 0x55cbbe345713 in process_client_msg /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:1277 #7 0x55cbbe34d74b in thread_manage_clients /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:2622 #8 0x55cbbe336ac4 in launch_thread /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/thread.cpp:66 #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 #5 0x55cbbe345713 in process_client_msg /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:1277 #6 0x55cbbe34d74b in thread_manage_clients /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/client.cpp:2622 #7 0x55cbbe336ac4 in launch_thread /home/jgalar/EfficiOS/src/lttng-tools/src/bin/lttng-sessiond/thread.cpp:66 #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 all end up having the same mangled name (e.g. _Z7zmallocI16thread_notifiersEPT_v). At link time, only one instance of zmalloc 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 Change-Id: I867e5a287ad8cf3ada617335bc1a80b800bf0833 ---