From b9973dea9fc1eaf88ef997561878e45cdfd369b4 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Fri, 19 Nov 2021 15:11:37 -0500 Subject: [PATCH] Fix: relayd: ressource leaks on viewer_stream_create error MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Observed issue ============== When facing failure to open viewer stream chunks in the context of "Fix: relayd: failure to open chunk files concurrently with session clear", we observe that the relay daemon triggers an assertion due to a non-empty session hash table on cleanup. Cause ===== viewer_stream_create() does a stream_get(), but without any matching stream_put() on error. This in turn holds a reference on the ctf_trace, which holds a reference on the session. By inspecting the code, we notice that the following ressources can be leaked on error: - vstream->stream_file.handle, - vstream->index_file, - vstream->stream. In non-error scenarios, viewer_stream_release() is responsible for releasing references on the composite objects. The vstream->stream_file.trace_chunk is not an issue because it is put in the destroy handler (as well as within the release, before having its vstream->stream_file.trace_chunk pointer set to NULL). Solution ======== Properly put references on all objects which are contained by the viewer stream on error by introducing viewer_stream_release_composite_objects(), which is used both in the error path of viewer_stream_create() and in viewer_stream_release(). Note ==== Why not move those "put" operations in viewer_stream_destroy ? This is done in the release to ensure we put references on composite objects immediately when our own reference reaches 0, rather than waiting for a grace period through call_rcu, which could then cause chained call_rcu callbacks and require multiple invocation of rcu_barrier on relayd exit to guarantee that all callbacks have been executed and all ressources properly freed. Known drawbacks =============== None. Signed-off-by: Mathieu Desnoyers Signed-off-by: Jérémie Galarneau Change-Id: I864b58eda94ebda5e6faea45c922d4e814a15daa --- src/bin/lttng-relayd/viewer-stream.cpp | 38 ++++++++++++++------------ 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/src/bin/lttng-relayd/viewer-stream.cpp b/src/bin/lttng-relayd/viewer-stream.cpp index ca0be9a62..69671bd7a 100644 --- a/src/bin/lttng-relayd/viewer-stream.cpp +++ b/src/bin/lttng-relayd/viewer-stream.cpp @@ -20,9 +20,26 @@ #include "lttng-relayd.h" #include "viewer-stream.h" -static void viewer_stream_destroy(struct relay_viewer_stream *vstream) +static void viewer_stream_release_composite_objects(struct relay_viewer_stream *vstream) { + if (vstream->stream_file.handle) { + fs_handle_close(vstream->stream_file.handle); + vstream->stream_file.handle = NULL; + } + if (vstream->index_file) { + lttng_index_file_put(vstream->index_file); + vstream->index_file = NULL; + } + if (vstream->stream) { + stream_put(vstream->stream); + vstream->stream = NULL; + } lttng_trace_chunk_put(vstream->stream_file.trace_chunk); + vstream->stream_file.trace_chunk = NULL; +} + +static void viewer_stream_destroy(struct relay_viewer_stream *vstream) +{ free(vstream->path_name); free(vstream->channel_name); free(vstream); @@ -198,6 +215,8 @@ struct relay_viewer_stream *viewer_stream_create(struct relay_stream *stream, error: if (vstream) { + /* Not using `put` since vstream is assumed to be published. */ + viewer_stream_release_composite_objects(vstream); viewer_stream_destroy(vstream); } return NULL; @@ -221,23 +240,8 @@ static void viewer_stream_release(struct urcu_ref *ref) if (vstream->stream->is_metadata) { rcu_assign_pointer(vstream->stream->trace->viewer_metadata_stream, NULL); } - viewer_stream_unpublish(vstream); - - if (vstream->stream_file.handle) { - fs_handle_close(vstream->stream_file.handle); - vstream->stream_file.handle = NULL; - } - if (vstream->index_file) { - lttng_index_file_put(vstream->index_file); - vstream->index_file = NULL; - } - if (vstream->stream) { - stream_put(vstream->stream); - vstream->stream = NULL; - } - lttng_trace_chunk_put(vstream->stream_file.trace_chunk); - vstream->stream_file.trace_chunk = NULL; + viewer_stream_release_composite_objects(vstream); call_rcu(&vstream->rcu_node, viewer_stream_destroy_rcu); } -- 2.34.1