Skip to content

Fix SDP session ID length limitation #438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Apr 24, 2025
Merged

Fix SDP session ID length limitation #438

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1d2f0be
session id is a numeric string, and should not be limited to size of …
lo-simon Mar 7, 2025
88a6c35
Use boost::lexical_cast to convert uint64_t to utility::string_t
lo-simon Mar 7, 2025
eab1c10
Remove session version length limitation, same to session id
lo-simon Mar 7, 2025
fd1fd2f
Update Development/sdp/test/sdp_test.cpp
lo-simon Mar 9, 2025
4fcb7ac
"session Id" and "session version" are an numeric string which accept…
lo-simon Mar 9, 2025
f4ae108
Merge branch 'master' into fix-session_id
lo-simon Mar 13, 2025
b17a399
Merge branch 'master' into fix-session_id
lo-simon Apr 4, 2025
0f60e6d
Thanks Gareth Sylvester-Bradley (NVIDIA) suggestion, best practice is…
lo-simon Apr 8, 2025
d57fd4b
Tidy up
lo-simon Apr 8, 2025
1a0970e
Add test to confirm that session id and session version can be number…
lo-simon Apr 22, 2025
55bad47
Merge remote-tracking branch 'origin/master' into fix-session_id
lo-simon Apr 22, 2025
f1526b9
Merge branch 'sony:master' into fix-session_id
lo-simon Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions Development/nmos/sdp_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <functional>
#include <map>
#include <stdexcept>
#include <boost/lexical_cast.hpp>
#include "bst/any.h"
#include "bst/optional.h"
#include "cpprest/basic_utils.h"
Expand Down Expand Up @@ -82,19 +83,35 @@ namespace nmos
struct origin_t
{
utility::string_t user_name;
uint64_t session_id;
uint64_t session_version;
utility::string_t session_id;
utility::string_t session_version;

origin_t() : session_id(), session_version() {}
origin_t(const utility::string_t& user_name, uint64_t session_id, uint64_t session_version)
: user_name(user_name)
, session_id(session_id)
, session_version(session_version)
, session_id(boost::lexical_cast<utility::string_t>(session_id))
, session_version(boost::lexical_cast<utility::string_t>(session_version))
{}
origin_t(const utility::string_t& user_name, uint64_t session_id_version)
origin_t(const utility::string_t& user_name, const utility::string_t& session_id, const utility::string_t& session_version)
: user_name(user_name)
, session_id(session_id_version)
, session_version(session_id_version)
, session_version(session_version)
{
// validate session_id is a numeric string
if (!std::all_of(session_id.begin(), session_id.end(), ::isdigit))
{
throw std::invalid_argument("session id must be a numeric string");
}
this->session_id = session_id;

// validate session_version is a numeric string
if (!std::all_of(session_version.begin(), session_version.end(), ::isdigit))
{
throw std::invalid_argument("session version must be a numeric string");
}
this->session_version = session_version;
}
origin_t(const utility::string_t& user_name, uint64_t session_id_version)
: origin_t{ user_name, session_id_version, session_id_version }
{}
} origin;

Expand Down
4 changes: 2 additions & 2 deletions Development/sdp/json.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ namespace sdp
// See https://tools.ietf.org/html/rfc4566#section-5.2
const web::json::field_as_value origin{ U("origin") };
const web::json::field_as_string user_name{ U("user_name") };
const web::json::field<uint64_t> session_id{ U("session_id") };
const web::json::field<uint64_t> session_version{ U("session_version") };
const web::json::field_as_string session_id{ U("session_id") };
const web::json::field_as_string session_version{ U("session_version") };
const web::json::field_as_string network_type{ U("network_type") }; // see sdp::network_types
const web::json::field_as_string address_type{ U("address_type") }; // see sdp::address_types
const web::json::field_as_string unicast_address{ U("unicast_address") };
Expand Down
23 changes: 21 additions & 2 deletions Development/sdp/sdp_grammar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include "sdp/sdp.h"

#include <stdexcept>
#include <boost/lexical_cast.hpp>
#include "bst/regex.h"
#include "cpprest/basic_utils.h"
#include "cpprest/json_visit.h"
Expand Down Expand Up @@ -68,6 +69,22 @@ namespace sdp
return web::json::value(v);
}

inline std::string jns2s(const web::json::value& v)
{
if (v.is_number()) return jn2s(v);
else return js2s(v);
}
inline web::json::value digits2jns(const std::string& s)
{
if (!std::all_of(s.begin(), s.end(), ::isdigit)) throw sdp_parse_error("expected a sequence of digits");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return s2js(valid_numeric_string(s))?


uint64_t v;
std::istringstream is(s);
is >> v;
if (is.fail() || !is.eof()) return s2js(s);
return web::json::value(boost::lexical_cast<utility::string_t>(v));
}

// find the first delimiter in str, beginning at pos, and return the substring from pos to the delimiter (or end)
// set pos to the end of the delimiter
inline std::string substr_find(const std::string& str, std::string::size_type& pos, const std::string& delimiter = {})
Expand Down Expand Up @@ -96,6 +113,8 @@ namespace sdp

const converter digits_converter{ jn2s, digits2jn };

const converter long_digits_converter{ jns2s, digits2jns };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor quibble... Long is often understood to refer to the long type... Maybe big_digits_converter as in "BigNum", something which is for holding numbers that are bigger than the largest built-in type?


// <key>[<separator><value>]
converter key_value_converter(char separator, const std::pair<utility::string_t, converter>& key_converter, const std::pair<utility::string_t, converter>& value_converter)
{
Expand Down Expand Up @@ -290,8 +309,8 @@ namespace sdp
'o',
object_converter({
{ sdp::fields::user_name, string_converter },
{ sdp::fields::session_id, digits_converter },
{ sdp::fields::session_version, digits_converter },
{ sdp::fields::session_id, long_digits_converter },
{ sdp::fields::session_version, long_digits_converter },
{ sdp::fields::network_type, string_converter },
{ sdp::fields::address_type, string_converter },
{ sdp::fields::unicast_address, string_converter }
Expand Down
40 changes: 36 additions & 4 deletions Development/sdp/test/sdp_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ a=mid:SECONDARY
{ sdp::fields::protocol_version, 0 },
{ sdp::fields::origin, web::json::value_of({
{ sdp::fields::user_name, U("-") },
{ sdp::fields::session_id, 3745911798u },
{ sdp::fields::session_version, 3745911798u },
{ sdp::fields::session_id, U("3745911798") },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the conversion from JSON data model to textual SDP file now handle both strings and numbers? Do we test both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are covered in the BST_TEST_CASE(testSdpSessionId) and BST_TEST_CASE(testSdpSessionVersion)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so, those check only parsing from textual SDP to JSON. My concern was whether an application developer who assumes the value in the JSON is a number as before will now get an exception either when they process the parsed JSON or when they construct the JSON with a number and generate SDP text. You can't make a value be both a number and a string of course! The generation could handle both (does it? this is the original question I was asking) but the parser can't. I guess given the type of the accessor object has changed they will get a compile error if they use that? Maybe that's good enough if so...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @garethsb, a new test testSdpSessionIdSessionVersionToSdp has been added to confirm that session id and session version can be numbers or strings for constructing the texture SDP.

{ sdp::fields::session_version, U("3745911798") },
{ sdp::fields::network_type, sdp::network_types::internet.name },
{ sdp::fields::address_type, sdp::address_types::IP4.name },
{ sdp::fields::unicast_address, U("192.168.9.142") }
Expand Down Expand Up @@ -346,8 +346,8 @@ a=framerate:59.94
{ sdp::fields::protocol_version, 0 },
{ sdp::fields::origin, web::json::value_of({
{ sdp::fields::user_name, U("-") },
{ sdp::fields::session_id, 0 },
{ sdp::fields::session_version, 0 },
{ sdp::fields::session_id, U("0") },
{ sdp::fields::session_version, U("0") },
{ sdp::fields::network_type, sdp::network_types::internet.name },
{ sdp::fields::address_type, sdp::address_types::IP4.name },
{ sdp::fields::unicast_address, U("192.0.2.0") }
Expand Down Expand Up @@ -571,3 +571,35 @@ a=fmtp:96)";
BST_REQUIRE_THROW(sdp::parse_session_description(test_sdp + bp), std::runtime_error);
}
}

////////////////////////////////////////////////////////////////////////////////////////////
BST_TEST_CASE(testSdpSessionId)
{
const std::string before = "v=0\r\no=- ";
const std::string after = " 42 IN IP4 10.0.0.1\r\ns= \r\nt=0 0\r\n";
BST_REQUIRE_NO_THROW(sdp::parse_session_description(before + "0" + after));
BST_REQUIRE_NO_THROW(sdp::parse_session_description(before + "007" + after));
BST_REQUIRE_NO_THROW(sdp::parse_session_description(before + "18446744073709551615" + after)); // session id to UINT64_MAX
BST_REQUIRE_NO_THROW(sdp::parse_session_description(before + "184467440737095516150" + after)); // session id greater than UINT64_MAX
// an invalid session id results in "sdp parse error - expected a sequence of digits at line 2"
BST_REQUIRE_THROW(sdp::parse_session_description(before + "foo" + after), std::runtime_error);
BST_REQUIRE_THROW(sdp::parse_session_description(before + "0foo" + after), std::runtime_error);
BST_REQUIRE_THROW(sdp::parse_session_description(before + "0.0" + after), std::runtime_error);
BST_REQUIRE_THROW(sdp::parse_session_description(before + "0x0" + after), std::runtime_error);
}

////////////////////////////////////////////////////////////////////////////////////////////
BST_TEST_CASE(testSdpSessionVersion)
{
const std::string before = "v=0\r\no=- 42 ";
const std::string after = " IN IP4 10.0.0.1\r\ns= \r\nt=0 0\r\n";
BST_REQUIRE_NO_THROW(sdp::parse_session_description(before + "0" + after));
BST_REQUIRE_NO_THROW(sdp::parse_session_description(before + "007" + after));
BST_REQUIRE_NO_THROW(sdp::parse_session_description(before + "18446744073709551615" + after)); // session version to UINT64_MAX
BST_REQUIRE_NO_THROW(sdp::parse_session_description(before + "184467440737095516150" + after)); // session version greater than UINT64_MAX
// an invalid session version results in "sdp parse error - expected a sequence of digits at line 2"
BST_REQUIRE_THROW(sdp::parse_session_description(before + "foo" + after), std::runtime_error);
BST_REQUIRE_THROW(sdp::parse_session_description(before + "0foo" + after), std::runtime_error);
BST_REQUIRE_THROW(sdp::parse_session_description(before + "0.0" + after), std::runtime_error);
BST_REQUIRE_THROW(sdp::parse_session_description(before + "0x0" + after), std::runtime_error);
}
Loading