bank/gui: bound parseMinor's input so an out-of-range amount is rejected rather than undefined (fixes #663)

examples/bank/CMakeLists.txt

@@ -206,6 +206,7 @@ if(MORPH_BUILD_TESTS)
         if(TARGET bank_gui_lib)
             add_executable(bank_gui_tests
                 tests/gui/test_bank_qml_surface.cpp
+                tests/gui/test_bank_gui_format.cpp
                 # QmlSurfaceAudit, compiled in rather than linked from
                 # morph::ladder_testkit. That library is only created by
                 # MORPH_BUILD_LADDER=ON, an option entirely independent of

examples/bank/gui/controllers/Format.hpp

@@ -64,7 +64,38 @@ inline QString last4(const std::string& number) {
     return QStringLiteral("•••• ") + QString::fromStdString(number).right(4);
 }
 
-/// Parses a user-entered major-unit amount into minor units (assumes @p decimals).
+/// @brief The first `double` value that no longer fits in a `std::int64_t`, i.e. 2^63.
+///
+/// `std::numeric_limits<std::int64_t>::max()` is 2^63-1, which is *not*
+/// representable as a `double` -- converting it rounds **up**, to 2^63. So a
+/// bound written as `static_cast<double>(max())` is off by one in the unsafe
+/// direction, and one written as `max() / scale` is off by the rounding of a
+/// division on top of that. 2^63 is exactly representable, so this literal is
+/// the one form of the bound that is exact.
+inline constexpr double kMinorUnitsBound = 0x1p63;
+
+/// @brief Parses a user-entered major-unit amount into minor units (assumes @p decimals).
+///
+/// Returns `std::nullopt` for anything that is not a non-negative amount that
+/// fits in `std::int64_t` minor units -- unparseable text, a negative value,
+/// `inf`/`nan` (both of which `QString::toDouble` accepts), and any magnitude
+/// whose scaled value would not fit. Every caller already treats `nullopt` as
+/// "reject this input", so the out-of-range cases join the ones that were
+/// already rejected rather than needing new handling.
+///
+/// The range check is what stops the conversion below being undefined
+/// behaviour: converting a `double` whose truncated value is outside the
+/// destination's range is UB ([conv.fpint]), and `QString::toDouble` happily
+/// accepts `1e30` from a QML text field with no validator (morph#663). The
+/// check is on the *scaled* value rather than on @p text's value, because only
+/// the scaled value is what gets converted -- `double` arithmetic itself
+/// cannot trap here, so computing it first costs nothing and removes the need
+/// to reason about how dividing the bound by @p scale rounds.
+///
+/// @param text     the user-entered amount, in major units
+/// @param decimals the number of minor-unit digits of the target currency
+/// @return the amount in minor units, or `std::nullopt` if @p text is not a
+///         representable non-negative amount
 inline std::optional<std::int64_t> parseMinor(const QString& text, int decimals = 2) {
     bool ok = false;
     const double major = text.trimmed().toDouble(&ok);
@@ -73,7 +104,14 @@ inline std::optional<std::int64_t> parseMinor(const QString& text, int decimals
     }
     // Reuse the core scale primitive so parse and format share one source.
     const auto scale = static_cast<double>(bank::pow10i(decimals));
-    return static_cast<std::int64_t>(major * scale + 0.5);
+    const double minor = (major * scale) + 0.5;
+    // Negated rather than written as `minor >= kMinorUnitsBound`, so that a
+    // NaN -- which compares false against everything, and which reaches here
+    // because `nan < 0.0` is false -- is rejected rather than let through.
+    if (!(minor < kMinorUnitsBound)) {
+        return std::nullopt;
+    }
+    return static_cast<std::int64_t>(minor);
 }
 
 }  // namespace bankgui::fmt

examples/bank/tests/gui/test_bank_gui_format.cpp

@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: Apache-2.0
+//
+// `bankgui::fmt::parseMinor` — the one function in the bank GUI that turns
+// arbitrary user text into an integer, and therefore the one that has to
+// survive arbitrary user text.
+//
+// Why this file exists at all: `gui/controllers/Format.hpp` is a header under
+// `examples/`, and the root `.clang-tidy`'s `HeaderFilterRegex` discarded
+// every finding in every such header (morph#664), so no analyser had ever
+// reported on it. What it contained was an unbounded `double` → `std::int64_t`
+// conversion (morph#663): `QString::toDouble` accepts `1e30`, `inf` and `nan`
+// from a QML field that carries no validator, and converting any of those is
+// undefined behaviour, not a large number.
+//
+// The cases below are written against the returned `std::optional`, not
+// against the arithmetic, and that is deliberate: on a UBSan build the
+// unfixed function *aborts* rather than returning a wrong answer, so a test
+// that asserted on the value would report the defect as a crash on one
+// configuration and as nothing at all on the others. Asserting that the
+// out-of-range inputs are *rejected* fails on both — as `-9223372036854775808
+// != nullopt` without a sanitizer, and as an abort with one.
+//
+// This TU needs Qt6::Core and nothing else (no engine, no platform plugin),
+// so it lives in `bank_gui_tests` alongside the QML surface audit rather than
+// in the binary that owns a QGuiApplication.
+
+#include <QString>
+#include <catch2/catch_test_macros.hpp>
+#include <cstdint>
+
+#include "controllers/Format.hpp"
+
+namespace {
+
+using bankgui::fmt::parseMinor;
+
+}  // namespace
+
+TEST_CASE("parseMinor turns well-formed amounts into minor units", "[bank][gui][format]") {
+    CHECK(parseMinor(QStringLiteral("12.34")) == 1234);
+    CHECK(parseMinor(QStringLiteral("0")) == 0);
+    CHECK(parseMinor(QStringLiteral("  7.5  ")) == 750);
+    // Rounds to nearest rather than truncating, which is what the `+ 0.5`
+    // does; pinned here only so that a future change to it is a visible one.
+    CHECK(parseMinor(QStringLiteral("0.005")) == 1);
+    // `decimals` comes from the selected currency (JPY has none).
+    CHECK(parseMinor(QStringLiteral("1200"), 0) == 1200);
+}
+
+TEST_CASE("parseMinor rejects text that is not a non-negative amount", "[bank][gui][format]") {
+    CHECK_FALSE(parseMinor(QStringLiteral("")).has_value());
+    CHECK_FALSE(parseMinor(QStringLiteral("abc")).has_value());
+    CHECK_FALSE(parseMinor(QStringLiteral("-1.00")).has_value());
+}
+
+// The morph#663 regression. Each of these returned `-9223372036854775808`
+// before the bound existed — via undefined behaviour, and via an abort under
+// UBSan — and every call site then fed that through `.value_or(0)` into a
+// balance.
+TEST_CASE("parseMinor rejects amounts that do not fit in int64 minor units", "[bank][gui][format]") {
+    // The value the issue reproduced with: 1e30 major units scale to 1e32.
+    CHECK_FALSE(parseMinor(QStringLiteral("1e30")).has_value());
+    CHECK_FALSE(parseMinor(QStringLiteral("1e300")).has_value());
+
+    // Not an absurd magnitude: anything above ~9.2e16 major units already
+    // overflows once scaled by 100, and nothing in the GUI said so.
+    CHECK_FALSE(parseMinor(QStringLiteral("9.3e16")).has_value());
+
+    // `QString::toDouble` accepts both of these, and `nan` passes a `< 0.0`
+    // guard because every comparison against a NaN is false.
+    CHECK_FALSE(parseMinor(QStringLiteral("inf")).has_value());
+    CHECK_FALSE(parseMinor(QStringLiteral("nan")).has_value());
+
+    // The scale is what decides the ceiling, so a value that overflows at two
+    // decimals is accepted at none.
+    CHECK_FALSE(parseMinor(QStringLiteral("1e17"), 2).has_value());
+    CHECK(parseMinor(QStringLiteral("1e17"), 0).has_value());
+}
+
+TEST_CASE("parseMinor's ceiling is the int64 range, not an arbitrary cap", "[bank][gui][format]") {
+    // 9.2e16 major units scale to 9.2e18 minor, just inside 2^63-1 ≈ 9.223e18,
+    // and are accepted exactly. A bound that was conservative by a factor or an
+    // order of magnitude would fail here rather than pass quietly.
+    // Compared as an `optional`, not dereferenced: `operator==` against a value
+    // is false for a disengaged optional, so this asserts both halves at once,
+    // and bugprone-unchecked-optional-access cannot see a `REQUIRE` guard
+    // through Catch2's macro expansion in any case.
+    CHECK(parseMinor(QStringLiteral("92000000000000000")) == 9200000000000000000LL);
+
+    // One order of magnitude further is out, so the accept/reject edge sits
+    // between them rather than somewhere arbitrary below.
+    CHECK_FALSE(parseMinor(QStringLiteral("920000000000000000")).has_value());
+}