From 1281f86d480cf496b8b42162b4b0d32b14dba4e0 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 21 Oct 2019 17:37:33 -0400 Subject: [PATCH] Fix: src.ctf.fs: free ds_file_info when add_ds_file_to_ds_file_group fails When add_ds_file_to_ds_file_group fails, the created ds_file_info is not freed, resulting in this leak: ==16942== 168 (16 direct, 152 indirect) bytes in 1 blocks are definitely lost in loss record 11 of 29 ==16942== at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==16942== by 0x534BB10: g_malloc0 (gmem.c:129) ==16942== by 0x7795E39: ctf_fs_ds_file_info_create (fs.c:558) ==16942== by 0x7795E39: add_ds_file_to_ds_file_group (fs.c:781) ==16942== by 0x7795E39: create_ds_file_groups (fs.c:954) ==16942== by 0x7795E39: ctf_fs_trace_create (fs.c:1100) ==16942== by 0x7795E39: ctf_fs_component_create_ctf_fs_trace_one_path (fs.c:1183) ==16942== by 0x7795E39: ctf_fs_component_create_ctf_fs_trace (fs.c:2059) ==16942== by 0x7797044: ctf_fs_create (fs.c:2352) ==16942== by 0x7797044: ctf_fs_init (fs.c:2386) ==16942== by 0x4E7172F: add_component_with_init_method_data (graph.c:1330) ==16942== by 0x4E76213: bt_graph_add_source_component_with_initialize_method_data (graph.c:1405) ==16942== by 0x4E762D0: bt_graph_add_source_component (graph.c:1417) ==16942== by 0x1149A9: cmd_run_ctx_create_components_from_config_components (babeltrace2.c:2313) ==16942== by 0x110353: cmd_run_ctx_create_components (babeltrace2.c:2405) ==16942== by 0x110353: cmd_run (babeltrace2.c:2519) ==16942== by 0x110353: main (babeltrace2.c:2814) This can happen if the index fails to build, as in: $ ./src/cli/babeltrace2 /home/smarchi/lttng-traces/auto-20191021-162849/ust/uid/1000/64-bit If add_ds_file_to_ds_file_group is successful, it transfers its ownership of the ds_file_info object to the ds_file_group, in which case it should not free it. But if it fails, the ds_file_info object should be freed. This patch introduces the BT_MOVE_REF macro, analogous to C++'s std::move, for cases like these. It passes the value of a pointer variable to a function, while setting that variable to NULL. This is meant to be used when the caller transfers its owning reference to the callee. Change-Id: I376403caf95e1eee9355ccd587620cf15c75b686 Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/2234 Reviewed-by: Francis Deslauriers --- src/common/macros.h | 23 +++++++++++++++++++++++ src/plugins/ctf/fs-src/fs.c | 7 +++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/common/macros.h b/src/common/macros.h index 23d4b310..036b3e01 100644 --- a/src/common/macros.h +++ b/src/common/macros.h @@ -38,4 +38,27 @@ #define BT_HIDDEN __attribute__((visibility("hidden"))) #endif +/* + * Yield `ref`'s value while setting `ref` to NULL. + * + * This can be used to give a strong reference to a callee: + * + * add_foo_to_list(list, BT_MOVE_REF(foo)); + * + * or in a simple assignment: + * + * my_struct->foo = BT_MOVE_REF(foo); + * + * When moving a reference in a function call, the reference is given to the + * callee even if that function call fails, so make sure the called function + * is written accordingly. + */ + +#define BT_MOVE_REF(ref) \ + ({ \ + __auto_type _ref = ref; \ + ref = NULL; \ + _ref; \ + }) + #endif diff --git a/src/plugins/ctf/fs-src/fs.c b/src/plugins/ctf/fs-src/fs.c index e92de4bc..28e2b811 100644 --- a/src/plugins/ctf/fs-src/fs.c +++ b/src/plugins/ctf/fs-src/fs.c @@ -818,7 +818,8 @@ int add_ds_file_to_ds_file_group(struct ctf_fs_trace *ctf_fs_trace, goto error; } - ds_file_group_insert_ds_file_info_sorted(ds_file_group, ds_file_info); + ds_file_group_insert_ds_file_info_sorted(ds_file_group, + BT_MOVE_REF(ds_file_info)); add_group = true; goto end; @@ -855,7 +856,8 @@ int add_ds_file_to_ds_file_group(struct ctf_fs_trace *ctf_fs_trace, merge_ctf_fs_ds_indexes(ds_file_group->index, index); } - ds_file_group_insert_ds_file_info_sorted(ds_file_group, ds_file_info); + ds_file_group_insert_ds_file_info_sorted(ds_file_group, + BT_MOVE_REF(ds_file_info)); goto end; @@ -870,6 +872,7 @@ end: } ctf_fs_ds_file_destroy(ds_file); + ctf_fs_ds_file_info_destroy(ds_file_info); if (msg_iter) { bt_msg_iter_destroy(msg_iter); -- 2.34.1