From a62fef288c0fb03d2bad21bd0eaa01a4a14489b7 Mon Sep 17 00:00:00 2001 From: Bruce Merry Date: Tue, 27 Jun 2023 12:05:01 +0200 Subject: [PATCH] Fix a bug with unwanted sharing of stats between stream_config Copying a stream_config would leave them pointing at the same vector, with correctness consequences if either copy later adds more statistics. This happens as part of chunk_stream::adjust_config. This could also lead to race conditions if a config was modified after constructing a stream with it, since it would affect and stream_stats instances that it shared with. The type has been changed to shared_ptr> throughout to prevent modifying shared data. This makes add_stat much less efficient (it copies the vector every time) but that is not on the critical path. --- doc/changelog.rst | 8 ++++++++ include/spead2/recv_stream.h | 17 ++++++++++++----- src/recv_stream.cpp | 19 +++++++++---------- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index ddb9dafeb..f08fdfc3b 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -5,6 +5,14 @@ Changelog - Update :meth:`!test_async_flush` and :meth:`!test_async_flush_fail` to keep handles to async tasks, to prevent them being garbage collected too early. +- Fix a bug where copying a :cpp:class:`spead2::recv::stream_config` would not + deep copy the names of custom statistics, and so any statistics added to the + copy would also affect the original, and there were also potential race + conditions if a stream config was modified while holding stream statistics. +- Fix a bug (caused by the bug above) where passing a + :cpp:class:`spead2::recv::stream_config` to construct a + :cpp:class:`spead2::recv::chunk_stream` would modify the config. Passing + the same config to construct two chunk streams would fail with an error. .. rubric:: 3.11.1 diff --git a/include/spead2/recv_stream.h b/include/spead2/recv_stream.h index c72e0e0e1..3a1ca7bda 100644 --- a/include/spead2/recv_stream.h +++ b/include/spead2/recv_stream.h @@ -184,7 +184,7 @@ class stream_stats_iterator : public boost::iterator_facade< class stream_stats { private: - std::shared_ptr> config; + std::shared_ptr> config; std::vector values; public: @@ -206,9 +206,9 @@ class stream_stats /// Construct with the default set of statistics, and all zero values stream_stats(); /// Construct with all zero values - explicit stream_stats(std::shared_ptr> config); + explicit stream_stats(std::shared_ptr> config); /// Construct with provided values - stream_stats(std::shared_ptr> config, + stream_stats(std::shared_ptr> config, std::vector values); /* Copy constructor and copy assignment need to be implemented manually @@ -357,8 +357,15 @@ class stream_config bool allow_out_of_order = false; /// A user-defined identifier for a stream std::uintptr_t stream_id = 0; - /// Statistics (includes the built-in ones) - std::shared_ptr> stats; + /** Statistics (includes the built-in ones) + * + * This is a shared_ptr so that instances of @ref stream_stats can share + * it. Every modification creates a new vector (copy-on-write). This is + * potentially very inefficient, since it creates a copy even when there + * are no sharers, but there are not expected to be huge numbers of + * statistics. + */ + std::shared_ptr> stats; public: stream_config(); diff --git a/src/recv_stream.cpp b/src/recv_stream.cpp index 98d16e80d..fa3fcd847 100644 --- a/src/recv_stream.cpp +++ b/src/recv_stream.cpp @@ -98,7 +98,7 @@ static std::size_t get_stat_index( } -static std::shared_ptr> make_default_stats() +static std::shared_ptr> make_default_stats() { auto stats = std::make_shared>(); // Keep this in sync with the stream_stat_* constexprs in the header @@ -121,21 +121,21 @@ static std::shared_ptr> make_default_stats() * Sharing this means the compatibility check for operator+ requires only a * pointer comparison rather than comparing arrays. */ -static std::shared_ptr> default_stats = make_default_stats(); +static std::shared_ptr> default_stats = make_default_stats(); stream_stats::stream_stats() : stream_stats(default_stats) { } -stream_stats::stream_stats(std::shared_ptr> config) +stream_stats::stream_stats(std::shared_ptr> config) : stream_stats(config, std::vector(config->size())) { // Note: annoyingly, can't use std::move(config) above, because we access // config to get the size to use for the vector. } -stream_stats::stream_stats(std::shared_ptr> config, +stream_stats::stream_stats(std::shared_ptr> config, std::vector values) : config(std::move(config)), values(std::move(values)), @@ -358,12 +358,11 @@ std::size_t stream_config::add_stat(std::string name, stream_stat_config::mode m { if (spead2::recv::get_stat_index_nothrow(*stats, name) != stats->size()) throw std::invalid_argument("A statistic called " + name + " already exists"); - // If we're pointing at the default, make a copy so that we don't modify - // the default. - if (stats == default_stats) - stats = std::make_shared>(*default_stats); - std::size_t index = stats->size(); - stats->emplace_back(std::move(name), mode); + // Make a copy so that we don't modify any shared copies + auto new_stats = std::make_shared>(*stats); + std::size_t index = new_stats->size(); + new_stats->emplace_back(std::move(name), mode); + stats = std::move(new_stats); return index; }