From ae5a62add82eda1488cd293e2780f7fdbed8fdfd Mon Sep 17 00:00:00 2001 From: Gareth Sylvester-Bradley Date: Wed, 20 May 2026 10:56:37 +0100 Subject: [PATCH 1/2] Fix silent integer-to-char narrowing on origin_t numeric members Assigning a uint64_t to sdp_parameters::origin_t::session_version silently selected utility::string_t::operator=(char), producing a one-character string from the low octet of the right-hand side. This manifests as a corrupted o= line after IS-05 PATCH-driven SDP regeneration, e.g. o=- 1779263096 \xfc IN IP4 ... where \xfc is the low byte of the NTP-seconds session-version. Introduce nmos::numeric_string, a thin wrapper around utility::string_t that exposes two operator= overloads (utility::string_t with valid_numeric_string validation, and uint64_t with to_string_t). Other integer-ish right-hand sides (signed int, char, bool, ...) chain to operator=(uint64_t) and therefore produce the same implicit-conversion diagnostics on integer assignment that a uint64_t member would produce. Default construction delegates to numeric_string(uint64_t{}) so that origin_t() yields "0" / "0" for session_id / session_version, matching the historic uint64_t behaviour and producing a well-formed o= line from a freshly-constructed origin_t. Retype origin_t::session_id and origin_t::session_version to numeric_string. The existing string-pair and uint64_t-pair constructors compile unchanged because the wrapper accepts both forms; the explicit to_string_t / valid_numeric_string calls in those constructor bodies are now unnecessary and removed. Two value_of call sites in make_session_description access .value explicitly because std::pair cannot chain two user-defined conversions (numeric_string -> string_t -> value_init). Tighten valid_numeric_string to reject the empty string (previously accepted because std::all_of on an empty range is vacuously true), aligning the big-digits-converter validation with digits2jn which already rejects empty input via istream >> uint64_t. Upgrade std::isdigit / std::isalnum calls on utility::char_t in valid_numeric_string and cpprest::is_tchar to the locale-aware templated overload std::isdigit(c, std::locale::classic()), removing the latent UB when utility::char_t is wchar_t and c is outside the unsigned-char range; the classic locale gives the same C-style ASCII semantics across narrow and wide char. Add testNumericString covering default == "0", uint64_t across the full range, leading-zero preservation, empty + non-numeric rejection, and the historic NTP-seconds footgun via origin_t::session_version. Signed-off-by: Gareth Sylvester-Bradley --- Development/cpprest/http_utils.cpp | 2 +- Development/nmos/sdp_utils.cpp | 7 +-- Development/nmos/sdp_utils.h | 36 ++++++++++++--- Development/nmos/test/sdp_utils_test.cpp | 58 ++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 10 deletions(-) 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..5c339aae9 100644 --- a/Development/nmos/sdp_utils.h +++ b/Development/nmos/sdp_utils.h @@ -68,6 +68,30 @@ namespace nmos // Validate the numeric string const utility::string_t& valid_numeric_string(const utility::string_t& 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(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=(uint64_t n) { return value = utility::conversions::details::to_string_t(n), *this; } + + operator const utility::string_t&() const { return value; } + }; + // Format-specific types struct video_raw_parameters; @@ -87,19 +111,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) { From 00211fe7bac14c03a7b0ecd0a1725f3757a08f99 Mon Sep 17 00:00:00 2001 From: Gareth Sylvester-Bradley <31761158+garethsb@users.noreply.github.com> Date: Fri, 22 May 2026 13:25:08 +0100 Subject: [PATCH 2/2] Add numeric_string construction and assignment from string rvalue Signed-off-by: Gareth Sylvester-Bradley --- Development/nmos/sdp_utils.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Development/nmos/sdp_utils.h b/Development/nmos/sdp_utils.h index 5c339aae9..59f5bed71 100644 --- a/Development/nmos/sdp_utils.h +++ b/Development/nmos/sdp_utils.h @@ -67,6 +67,7 @@ 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 @@ -84,9 +85,11 @@ namespace nmos 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; }