Haivision / srt

Secure, Reliable, Transport
https://www.srtalliance.org
Mozilla Public License 2.0
3.12k stars 855 forks source link

[BUG] setting/getting SRTO_STREAMID to group socket is broken in SRT 1.5.4 #3072

Closed maxtomilov closed 1 week ago

maxtomilov commented 1 week ago

When SRTO_STREAMID is set to group socket SRT lib fails on "SRT_ASSERT(opt_size == default_opt_size);" assertion on group.cpp:456 due to incorrect option size interpretation in importOption function (default_opt_size == sizeof(std::string) == 32 and opt_size returned from getOptDefault call on group.cpp:454 is std::string().size() == 0) so process crashes with the following error:

srt/srtcore/group.cpp:456: void srt::importOption(std::vector&, SRT_SOCKOPT, const ValueType&) [with ValueType = std::__cxx11::basic_string; SRT_SOCKOPT = SRT_SOCKOPT]: Assertion `opt_size == default_opt_size' failed.

To Reproduce Steps to reproduce the behavior:

  1. build SRT with --enable-debug=1
  2. create a group socket and set SRTO_STREAMID:

auto sock = srt_create_group(SRT_GTYPE_BROADCAST); std::string stream_id = "live/stream"; srt_setsockopt(sock, 0, SRTO_STREAMID, stream_id.c_str(), stream_id.length());

  1. See error

assertion failure later trying using this socket later (srt_connect_group and srt_sendmsg)

tested on Ubuntu 22.04 / SRT 1.5.4

maxsharabayko commented 1 week ago

@maxtomilov Thank you for reporting this promptly!

A patch to unit test to reproduce the issue (test_srt --gtest_filter="Bonding.Options"):

diff --git a/test/test_bonding.cpp b/test/test_bonding.cpp
index 0e48c8a0..87afec1a 100644
--- a/test/test_bonding.cpp
+++ b/test/test_bonding.cpp
@@ -1,3 +1,4 @@
+#include <array>
 #include <future>
 #include <thread>
 #include <chrono>
@@ -359,6 +360,9 @@ TEST(Bonding, Options)
 #endif
 #endif

+    std::array<char, 10> streamid = { 's', 't', 'r', 'e', 0, 'm', 'i', 'd', '%', '&' };
+    EXPECT_NE(srt_setsockflag(grp, SRTO_STREAMID, &streamid, streamid.size()), SRT_ERROR);
+
     int lat = 500;
     EXPECT_NE(srt_setsockflag(grp, SRTO_RCVLATENCY, &lat, sizeof lat), SRT_ERROR);
maxsharabayko commented 1 week ago

Setting SRTO_STREAMID on a group was also not working in SRT v1.5.3. 🤔

A patch to unit test v1.5.3.

diff --git a/test/test_bonding.cpp b/test/test_bonding.cpp
index a03b7c5a..15fbcf19 100644
--- a/test/test_bonding.cpp
+++ b/test/test_bonding.cpp
@@ -1,3 +1,4 @@
+#include <array>
 #include <future>
 #include <thread>
 #include <chrono>
@@ -141,6 +142,15 @@ TEST(Bonding, NonBlockingGroupConnect)
     ASSERT_NE(srt_setsockopt(ss, 0, SRTO_RCVSYN, &no, sizeof no), SRT_ERROR); // non-blocking mode
     ASSERT_NE(srt_setsockopt(ss, 0, SRTO_SNDSYN, &no, sizeof no), SRT_ERROR); // non-blocking mode

+    std::array<char, 10> streamid = { 's', 't', 'r', 'e', 0, 'm', 'i', 'd', '%', '&' };
+    EXPECT_NE(srt_setsockflag(ss, SRTO_STREAMID, &streamid, streamid.size()), SRT_ERROR);
+
+    std::array<char, 512> tmpbuf;
+    auto opt_len = (int)tmpbuf.size();
+    EXPECT_EQ(srt_getsockflag(ss, SRTO_STREAMID, tmpbuf.data(), &opt_len), SRT_SUCCESS);
+    EXPECT_EQ(size_t(opt_len), streamid.size());
+    EXPECT_EQ(std::memcmp(tmpbuf.data(), streamid.data(), opt_len), 0);
+
     const int poll_id = srt_epoll_create();
     // Will use this epoll to wait for srt_accept(...)
     const int epoll_out = SRT_EPOLL_OUT | SRT_EPOLL_ERR;
maxtomilov commented 1 week ago

I double-checked and also see getsockopt does not work indeed on 1.5.3 on a client socket (we use only setsockopt) - but setsockopt worked fine and STREAMID set is passed to the server, and getsockopt with STREAMID works on server socket (accepting the client)

maxsharabayko commented 1 week ago

@maxtomilov You are right. v1.5.3 works. srt_setsockopt applied on a group socket worked in v1.5.3. srt_getsockopt on the same group socket didn't work in v1.5.3. On the listener side options derivation didn't work well for a group, but retrieving SRTO_STREAMID on an accepted group connection went down to the first member socket, so the value was there.

Lokks like it was broken in PR #2891 (v1.5.4). Even removing the assertion or reverting changes from PR #3066 the srt_getsockopt(SRTO_STREAMID) fails to return a value.

"vector subscript out of range" in CUDTGroup::getopt(..)` line 833:

memcpy((pw_optval), &i->value[0], i->value.size());

because i is a zero length std::vector.

maxsharabayko commented 1 week ago

So the bug is:

  1. Getting SRTO_STREAMID on an accepted group connection does not work (listener side). Broken in PR #2891.
  2. There would also be a debug assertion in a debug build on the listener side when a group connection with SRTO_STREAMID set is being accepted. Assertion added in PR #3066.