Skip to content

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

Open
garethsb wants to merge 1 commit into
sony:masterfrom
garethsb:bugfix/session_version
Open

Fix silent integer-to-char narrowing on origin_t numeric members#489
garethsb wants to merge 1 commit into
sony:masterfrom
garethsb:bugfix/session_version

Conversation

@garethsb
Copy link
Copy Markdown
Contributor

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.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant