From 26cedfcaf0abe8730e6ffc0db37d7b58b674a708 Mon Sep 17 00:00:00 2001 From: Francis Deslauriers Date: Wed, 16 Jun 2021 12:10:42 -0400 Subject: [PATCH] Fix: use of uninitialised bytes valgrind warning MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Issue ===== Valgrind reports usage of uninitialised stack allocated memory: ==2961363== Thread 9 Client manageme: ==2961363== Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s) ==2961363== at 0x521418D: __libc_sendmsg (sendmsg.c:28) ==2961363== by 0x521418D: sendmsg (sendmsg.c:25) ==2961363== by 0x53411B: lttcomm_send_unix_sock (unix.c:294) ==2961363== by 0x48AA8C: send_unix_sock (client.c:896) ==2961363== by 0x484F45: thread_manage_clients (client.c:2865) ==2961363== by 0x480FB4: launch_thread (thread.c:66) ==2961363== by 0x5208608: start_thread (pthread_create.c:477) ==2961363== by 0x5346292: clone (clone.S:95) ==2961363== Address 0x7575389 is 25 bytes inside a block of size 16,384 alloc'd ==2961363== at 0x483DFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==2961363== by 0x4EB618: lttng_dynamic_buffer_set_capacity (dynamic-buffer.c:166) ==2961363== by 0x4EB52C: lttng_dynamic_buffer_append (dynamic-buffer.c:55) ==2961363== by 0x48CBA1: setup_lttng_msg (client.c:125) ==2961363== by 0x48AD70: setup_lttng_msg_no_cmd_header (client.c:860) ==2961363== by 0x489825: process_client_msg (client.c:2253) ==2961363== by 0x484A97: thread_manage_clients (client.c:2807) ==2961363== by 0x480FB4: launch_thread (thread.c:66) ==2961363== by 0x5208608: start_thread (pthread_create.c:477) ==2961363== by 0x5346292: clone (clone.S:95) ==2961363== Uninitialised value was created by a stack allocation ==2961363== at 0x485FE4: process_client_msg (client.c:928) After some digging, I found that this warning was caused by the padding of the `struct lttng_session_list_schedules_return` during the `LTTNG_SESSION_LIST_ROTATION_SCHEDULES` command. All the fields are of the stack allocated struct are initialised by the designated initializer but the padding is not. These padding bytes are reported by Valgrind as being used uninitialised. Fix === Remove the padding by adding the LTTNG_PACKED attribute to the nested structs in `struct lttng_session_list_schedules_return`. Notes ===== In light of the actual root cause, this is stacktrace is not really useful. The realloc call to grow the buffer makes it hard to find what is the actual uninitialised stack allocation because Valgrind reports the realloc call as the problematic site. I was able to track this issue by adding a "consuming" step in the `lttng_dynamic_buffer_append()` function. This consuming step would sum all the bytes of the `buf` parameter so as to force Valgrind to check each byte and not wait until the `sendmsg()` call. This way, I was able to get a more precise location of the root cause of the issue. Signed-off-by: Francis Deslauriers Signed-off-by: Jérémie Galarneau Change-Id: Ib4a729575e9117cf95716ad25e1417c833f4232b --- include/lttng/rotate-internal.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/lttng/rotate-internal.h b/include/lttng/rotate-internal.h index 91289b4cf..0296efed1 100644 --- a/include/lttng/rotate-internal.h +++ b/include/lttng/rotate-internal.h @@ -108,11 +108,11 @@ struct lttng_session_list_schedules_return { struct { uint8_t set; uint64_t value; - } periodic; + } LTTNG_PACKED periodic; struct { uint8_t set; uint64_t value; - } size; + } LTTNG_PACKED size; } LTTNG_PACKED; #endif /* LTTNG_ROTATE_INTERNAL_ABI_H */ -- 2.34.1