Fix: acquire stream lock during kernel metadata snapshot

The stream lock is not taken when interacting with the kernel
metadata stream that is created at the time a snapshot is taken.

This was noticed while reviewing the code for an unrelated reason,
so there is no known problem caused by this. Nevertheless, this
is incorrect as the stream is globally visible in the consumer.

Moreover, the stream was not cleaned-up which can cause a leak
whenever a metadata snapshot fails.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>

diff --git a/src/common/kernel-consumer/kernel-consumer.c b/src/common/kernel-consumer/kernel-consumer.c
index 87ade12fa33a32e71dd52cc685a6f3acb02e967b..3455f827b6b715c5415bcb3ee11c0d6147a46972 100644 (file)
--- a/src/common/kernel-consumer/kernel-consumer.c
+++ b/src/common/kernel-consumer/kernel-consumer.c
@@ -336,7 +336,7 @@ end:
  *
  * Returns 0 on success, < 0 on error
  */
  *
  * Returns 0 on success, < 0 on error
  */

-int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
+static int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,

                uint64_t relayd_id, struct lttng_consumer_local_data *ctx)
 {
        int ret, use_relayd = 0;
                uint64_t relayd_id, struct lttng_consumer_local_data *ctx)
 {
        int ret, use_relayd = 0;


@@ -355,11 +355,12 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
        if (!metadata_channel) {
                ERR("Kernel snapshot metadata not found for key %" PRIu64, key);
                ret = -1;
        if (!metadata_channel) {
                ERR("Kernel snapshot metadata not found for key %" PRIu64, key);
                ret = -1;

-               goto error;
+               goto error_no_channel;

        }
 
        metadata_stream = metadata_channel->metadata_stream;
        assert(metadata_stream);
        }
 
        metadata_stream = metadata_channel->metadata_stream;
        assert(metadata_stream);

+       pthread_mutex_lock(&metadata_stream->lock);

 
        /* Flag once that we have a valid relayd for the stream. */
        if (relayd_id != (uint64_t) -1ULL) {
 
        /* Flag once that we have a valid relayd for the stream. */
        if (relayd_id != (uint64_t) -1ULL) {


@@ -369,7 +370,7 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
        if (use_relayd) {
                ret = consumer_send_relayd_stream(metadata_stream, path);
                if (ret < 0) {
        if (use_relayd) {
                ret = consumer_send_relayd_stream(metadata_stream, path);
                if (ret < 0) {

-                       goto error;
+                       goto error_snapshot;

                }
        } else {
                ret = utils_create_stream_file(path, metadata_stream->name,
                }
        } else {
                ret = utils_create_stream_file(path, metadata_stream->name,


@@ -377,7 +378,7 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
                                metadata_stream->tracefile_count_current,
                                metadata_stream->uid, metadata_stream->gid, NULL);
                if (ret < 0) {
                                metadata_stream->tracefile_count_current,
                                metadata_stream->uid, metadata_stream->gid, NULL);
                if (ret < 0) {

-                       goto error;
+                       goto error_snapshot;

                }
                metadata_stream->out_fd = ret;
        }
                }
                metadata_stream->out_fd = ret;
        }


@@ -390,7 +391,8 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
                        if (ret_read != -EAGAIN) {
                                ERR("Kernel snapshot reading metadata subbuffer (ret: %zd)",
                                                ret_read);
                        if (ret_read != -EAGAIN) {
                                ERR("Kernel snapshot reading metadata subbuffer (ret: %zd)",
                                                ret_read);

-                               goto error;
+                               ret = ret_read;
+                               goto error_snapshot;

                        }
                        /* ret_read is negative at this point so we will exit the loop. */
                        continue;
                        }
                        /* ret_read is negative at this point so we will exit the loop. */
                        continue;


@@ -415,11 +417,12 @@ int lttng_kconsumer_snapshot_metadata(uint64_t key, char *path,
        }
 
        ret = 0;
        }
 
        ret = 0;

-
+error_snapshot:
+       pthread_mutex_unlock(&metadata_stream->lock);

        cds_list_del(&metadata_stream->send_node);
        consumer_stream_destroy(metadata_stream, NULL);
        metadata_channel->metadata_stream = NULL;
        cds_list_del(&metadata_stream->send_node);
        consumer_stream_destroy(metadata_stream, NULL);
        metadata_channel->metadata_stream = NULL;

-error:
+error_no_channel:

        rcu_read_unlock();
        return ret;
 }
        rcu_read_unlock();
        return ret;
 }