Fix a bug with unwanted sharing of stats between stream_config

Copying a stream_config would leave them pointing at the same
vector<stream_stat_config>, 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<const vector<...>>
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

@@ -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
 

include/spead2/recv_stream.h

@@ -184,7 +184,7 @@ class stream_stats_iterator : public boost::iterator_facade<
 class stream_stats
 {
 private:
-    std::shared_ptr<std::vector<stream_stat_config>> config;
+    std::shared_ptr<const std::vector<stream_stat_config>> config;
     std::vector<std::uint64_t> 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<std::vector<stream_stat_config>> config);
+    explicit stream_stats(std::shared_ptr<const std::vector<stream_stat_config>> config);
     /// Construct with provided values
-    stream_stats(std::shared_ptr<std::vector<stream_stat_config>> config,
+    stream_stats(std::shared_ptr<const std::vector<stream_stat_config>> config,
                  std::vector<std::uint64_t> 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<std::vector<stream_stat_config>> 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<const std::vector<stream_stat_config>> stats;
 
 public:
     stream_config();

src/recv_stream.cpp

@@ -98,7 +98,7 @@ static std::size_t get_stat_index(
 }
 
 
-static std::shared_ptr<std::vector<stream_stat_config>> make_default_stats()
+static std::shared_ptr<const std::vector<stream_stat_config>> make_default_stats()
 {
     auto stats = std::make_shared<std::vector<stream_stat_config>>();
     // Keep this in sync with the stream_stat_* constexprs in the header
@@ -121,21 +121,21 @@ static std::shared_ptr<std::vector<stream_stat_config>> make_default_stats()
  * Sharing this means the compatibility check for operator+ requires only a
  * pointer comparison rather than comparing arrays.
  */
-static std::shared_ptr<std::vector<stream_stat_config>> default_stats = make_default_stats();
+static std::shared_ptr<const std::vector<stream_stat_config>> default_stats = make_default_stats();
 
 stream_stats::stream_stats()
     : stream_stats(default_stats)
 {
 }
 
-stream_stats::stream_stats(std::shared_ptr<std::vector<stream_stat_config>> config)
+stream_stats::stream_stats(std::shared_ptr<const std::vector<stream_stat_config>> config)
     : stream_stats(config, std::vector<std::uint64_t>(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<std::vector<stream_stat_config>> config,
+stream_stats::stream_stats(std::shared_ptr<const std::vector<stream_stat_config>> config,
                            std::vector<std::uint64_t> 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<std::vector<stream_stat_config>>(*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<std::vector<stream_stat_config>>(*stats);
+    std::size_t index = new_stats->size();
+    new_stats->emplace_back(std::move(name), mode);
+    stats = std::move(new_stats);
     return index;
 }