Fix silent integer-to-char narrowing on origin_t numeric members#489
Open
garethsb wants to merge 1 commit into
Open
Fix silent integer-to-char narrowing on origin_t numeric members#489garethsb wants to merge 1 commit into
garethsb wants to merge 1 commit into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
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.