util: formatRationalDecimal negates INT64_MIN in int64 (UB), under a comment claiming it widens first

[Yaraslaut]

Summary

morph::units::detail::formatRationalDecimal (include/morph/util/quantity.hpp:98-102) performs signed negation of INT64_MIN, which is undefined behaviour. The comment directly above it says the opposite — that the value is widened first specifically to avoid this.

Verification status

Reproduced under UndefinedBehaviorSanitizer. Branch bridge-async-threading-contract, clang 22, -fsanitize=undefined -fno-sanitize-recover=all.

numerator = -9223372036854775808
include/morph/util/quantity.hpp:101:47: runtime error: negation of -9223372036854775808 cannot be represented
in type 'std::int64_t' (aka 'long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior include/morph/util/quantity.hpp:101:47

Reproducer (12 lines):

morph::math::Rational r{std::numeric_limits<std::int64_t>::min(), morph::math::DecimalPlaces{2}};
std::cout << morph::units::detail::formatRationalDecimal(r) << "\n";

The code and the comment

// Negating INT64_MIN would overflow int64; widen before taking the absolute
// value so the magnitude is always representable.
auto const num =
    negative ? static_cast<std::uint64_t>(-static_cast<std::int64_t>(static_cast<std::uint64_t>(value.numerator)))
             : static_cast<std::uint64_t>(value.numerator);

Nothing is widened. The casts nest as uint64_t( -int64_t( uint64_t(n) ) ): the inner round-trip lands back in int64_t, and the unary - is signed negation. The outer uint64_t cast happens after the UB, not before it.

Reachability

INT64_MIN genuinely reaches this function:

• Rational's whole-integer constructor (rational.hpp:389-390) does not call canonicalise(), so the numerator is retained verbatim — this is what the reproducer uses.
• numerator is a public data member.

Note the contrast: values that do go through canonicalise() (rational.hpp:925-929) are clamped to -INT64_MAX with an error log, so the three-arg path is safe. It is specifically the non-canonicalising entry points that reach this.

The fix already exists in an included header

morph::math::detail::absU64 (rational.hpp:376-378) does exactly what the comment describes:

0ULL - static_cast<std::uint64_t>(value)

quantity.hpp already includes rational.hpp, so this is a one-line substitution.

What would change the verdict

• Close it if every construction path into formatRationalDecimal is shown to canonicalise first — but the two-argument constructor demonstrably does not, and the member is public.
• Worth checking whether the clang-ubsan CI leg would have caught this: it evidently does not today, which suggests no test formats a non-canonicalised Rational.

Related

docs/spec/util/rational.md:163-165, 180-199 describes a different, now-stale version of the INT64_MIN hazard — it claims canonicalise itself negates INT64_MIN via an absoluteNumerator local. That identifier no longer exists (grep returns nothing) and the header clamps correctly. So the spec warns about a fixed problem while the real one, one file away, is undocumented.

[Yaraslaut]

Triage: triage: valid, bug, area: util

Premise re-checked empirically. Reproduced under UBSan against the current tip:

numerator = -9223372036854775808
include/morph/util/quantity.hpp:101:47: runtime error: negation of -9223372036854775808 cannot be
represented in type 'std::int64_t' (aka 'long'); cast to an unsigned type to negate this value to itself

Invariant 3 is the interesting one, and it exonerates the design. Rational does the right thing at the type level: canonicalise() clamps INT64_MIN to -INT64_MAX and reports the fact via reportClamp(), leaving the policy to the layer. The defect is not in that split — it is that formatRationalDecimal renders a value that never went through it, because the two-argument whole-integer constructor does not canonicalise and numerator is public. So this is a plain bug at one call site, not an architecture question.

Invariant 7 applies: the clang-ubsan CI leg does not catch this today, which tells us no test formats a non-canonicalised Rational. A regression test must construct through the non-canonicalising path, or it will be green whether the fix lands or not.

Direction fits as written — detail::absU64 (rational.hpp:376-378) already implements exactly what the comment claims, in a header quantity.hpp already includes.

Worth recording: docs/spec/util/rational.md warned about a different, already-fixed version of this hazard (an absoluteNumerator local that no longer exists) while this one went undocumented. That half is corrected in PR #503.