diff --git a/Development/cpprest/http_utils.cpp b/Development/cpprest/http_utils.cpp index 1760b7232..227a5228a 100644 --- a/Development/cpprest/http_utils.cpp +++ b/Development/cpprest/http_utils.cpp @@ -189,7 +189,7 @@ namespace web inline bool is_tchar(utility::char_t c) { static const utility::string_t tchar_punct{ U("!#$%&'*+-.^_`|~") }; - return std::isalnum(c) || std::string::npos != tchar_punct.find(c); + return std::isalnum(c, std::locale::classic()) || std::string::npos != tchar_punct.find(c); } // "A sender SHOULD NOT generate a quoted-pair in a quoted-string except where necessary to quote DQUOTE and backslash" diff --git a/Development/nmos/sdp_utils.cpp b/Development/nmos/sdp_utils.cpp index 9e59dca0c..93eec88d4 100644 --- a/Development/nmos/sdp_utils.cpp +++ b/Development/nmos/sdp_utils.cpp @@ -480,8 +480,8 @@ namespace nmos // See https://tools.ietf.org/html/rfc4566#section-5.2 { sdp::fields::origin, value_of({ { sdp::fields::user_name, sdp_params.origin.user_name }, - { sdp::fields::session_id, sdp_params.origin.session_id }, - { sdp::fields::session_version, sdp_params.origin.session_version }, + { sdp::fields::session_id, sdp_params.origin.session_id.value }, + { sdp::fields::session_version, sdp_params.origin.session_version.value }, { sdp::fields::network_type, sdp::network_types::internet.name }, { sdp::fields::address_type, details::get_address_type_multicast(origin_address).first.name }, { sdp::fields::unicast_address, origin_address } @@ -1626,9 +1626,10 @@ namespace nmos } // Validate the numeric string + // i.e. a non-empty sequence of decimal digits (leading zeros allowed) const utility::string_t& valid_numeric_string(const utility::string_t& s) { - if (!std::all_of(s.begin(), s.end(), [](utility::char_t c) { return std::isdigit(c); })) + if (s.empty() || !std::all_of(s.begin(), s.end(), [](utility::char_t c) { return std::isdigit(c, std::locale::classic()); })) { throw std::invalid_argument("not a numeric string"); } diff --git a/Development/nmos/sdp_utils.h b/Development/nmos/sdp_utils.h index bf854a47c..59f5bed71 100644 --- a/Development/nmos/sdp_utils.h +++ b/Development/nmos/sdp_utils.h @@ -67,6 +67,33 @@ namespace nmos // Validate the numeric string const utility::string_t& valid_numeric_string(const utility::string_t& s); + inline utility::string_t&& valid_numeric_string(utility::string_t&& s) { return valid_numeric_string(s), std::move(s); } + + // A non-negative integer represented as a numeric string. + // Behaves as a utility::string_t for read access. On assignment it + // accepts either a validated numeric string (preserving any leading + // zeros) or a uint64_t (rendered as a minimal-width decimal string), + // and produces the same implicit-conversion diagnostics on integer + // assignment that a uint64_t member would produce (e.g. -Wsign-conversion + // when the right-hand side is int/int64_t/etc.). + // Introduced to prevent the silent narrowing footgun that + // utility::string_t::operator=(char) would otherwise allow on a plain + // string member, e.g. for sdp_parameters::origin_t::session_version. + struct numeric_string + { + utility::string_t value; + + numeric_string() : numeric_string(uint64_t{}) {} + numeric_string(const utility::string_t& s) : value(valid_numeric_string(s)) {} + numeric_string(utility::string_t&& s) : value(valid_numeric_string(std::move(s))) {} + numeric_string(uint64_t n) : value(utility::conversions::details::to_string_t(n)) {} + + numeric_string& operator=(const utility::string_t& s) { return value = valid_numeric_string(s), *this; } + numeric_string& operator=(utility::string_t&& s) { return value = valid_numeric_string(std::move(s)), *this; } + numeric_string& operator=(uint64_t n) { return value = utility::conversions::details::to_string_t(n), *this; } + + operator const utility::string_t&() const { return value; } + }; // Format-specific types @@ -87,19 +114,19 @@ namespace nmos struct origin_t { utility::string_t user_name; - utility::string_t session_id; - utility::string_t session_version; + numeric_string session_id; + numeric_string session_version; origin_t() {} origin_t(const utility::string_t& user_name, uint64_t session_id, uint64_t session_version) : user_name(user_name) - , session_id(utility::conversions::details::to_string_t(session_id)) - , session_version(utility::conversions::details::to_string_t(session_version)) + , session_id(session_id) + , session_version(session_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(valid_numeric_string(session_id)) - , session_version(valid_numeric_string(session_version)) + , session_id(session_id) + , 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 } diff --git a/Development/nmos/test/sdp_utils_test.cpp b/Development/nmos/test/sdp_utils_test.cpp index edf850a23..0d34eae33 100644 --- a/Development/nmos/test/sdp_utils_test.cpp +++ b/Development/nmos/test/sdp_utils_test.cpp @@ -10,6 +10,7 @@ #include "nmos/media_type.h" #include "nmos/random.h" #include "nmos/test/sdp_test_utils.h" +#include "sdp/ntp.h" #include "sdp/sdp.h" //////////////////////////////////////////////////////////////////////////////////////////// @@ -218,6 +219,63 @@ BST_TEST_CASE(testValidateSdpParameters) } } +//////////////////////////////////////////////////////////////////////////////////////////// +BST_TEST_CASE(testNumericString) +{ + // Default-constructed value matches uint64_t{}, i.e. the string "0", + // so a default-constructed origin_t still produces a well-formed SDP o= line + { + nmos::numeric_string ns; + BST_REQUIRE_EQUAL(U("0"), static_cast(ns)); + } + + // Integer assignment produces the corresponding minimal-width decimal string. + // Other integer-ish right-hand sides (signed int, char, bool, etc.) implicitly + // convert to uint64_t (the only operator= overload that takes a number) and + // therefore produce a decimal string in the same way; we don't enumerate them + // here because that would trigger any implicit-conversion compiler warnings + // (the same as a uint64_t member would produce). + { + nmos::numeric_string ns; + ns = uint64_t{ 0 }; + BST_REQUIRE_EQUAL(U("0"), static_cast(ns)); + ns = uint64_t{ 1779263096 }; + BST_REQUIRE_EQUAL(U("1779263096"), static_cast(ns)); + ns = (std::numeric_limits::max)(); + BST_REQUIRE_EQUAL(U("18446744073709551615"), static_cast(ns)); + } + + // String assignment preserves any leading zeros from the input + { + nmos::numeric_string ns; + ns = U("0001779263096"); + BST_REQUIRE_EQUAL(U("0001779263096"), static_cast(ns)); + } + + // Empty and non-numeric strings are rejected + { + nmos::numeric_string ns; + BST_REQUIRE_THROW(ns = U(""), std::invalid_argument); + BST_REQUIRE_THROW(ns = U("abc"), std::invalid_argument); + BST_REQUIRE_THROW(ns = U("12a"), std::invalid_argument); + BST_REQUIRE_THROW(ns = U("-1"), std::invalid_argument); + BST_REQUIRE_THROW(ns = U(" 1"), std::invalid_argument); + BST_REQUIRE_THROW(ns = U("1.0"), std::invalid_argument); + BST_REQUIRE_THROW(nmos::numeric_string{ U("") }, std::invalid_argument); + BST_REQUIRE_THROW(nmos::numeric_string{ U("abc") }, std::invalid_argument); + } + + // The historic footgun: assigning an integer to an origin_t numeric + // string member must produce a multi-digit decimal string + { + nmos::sdp_parameters::origin_t o; + o.session_version = sdp::ntp_now() >> 32; + const utility::string_t& sv = o.session_version; + BST_REQUIRE(sv.size() > 1); + BST_REQUIRE(std::all_of(sv.begin(), sv.end(), [](utility::char_t c) { return std::isdigit(c, std::locale::classic()); })); + } +} + //////////////////////////////////////////////////////////////////////////////////////////// BST_TEST_CASE(testSdpParametersRoundtrip) {