Fix silent integer-to-char narrowing on origin_t numeric members (#489)

* 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<string_t, web::json::details::value_init>
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 <garethsb@nvidia.com>

* Add numeric_string construction and assignment from string rvalue

Signed-off-by: Gareth Sylvester-Bradley <garethsb@nvidia.com>

---------

Signed-off-by: Gareth Sylvester-Bradley <garethsb@nvidia.com>

Author: garethsb

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"

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");
         }

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 }

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<const utility::string_t&>(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<const utility::string_t&>(ns));
+        ns = uint64_t{ 1779263096 };
+        BST_REQUIRE_EQUAL(U("1779263096"), static_cast<const utility::string_t&>(ns));
+        ns = (std::numeric_limits<uint64_t>::max)();
+        BST_REQUIRE_EQUAL(U("18446744073709551615"), static_cast<const utility::string_t&>(ns));
+    }
+
+    // String assignment preserves any leading zeros from the input
+    {
+        nmos::numeric_string ns;
+        ns = U("0001779263096");
+        BST_REQUIRE_EQUAL(U("0001779263096"), static_cast<const utility::string_t&>(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)
 {