diff --git a/examples/bank/CMakeLists.txt b/examples/bank/CMakeLists.txt index ae75f7090..06034d805 100644 --- a/examples/bank/CMakeLists.txt +++ b/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 diff --git a/examples/bank/gui/controllers/Format.hpp b/examples/bank/gui/controllers/Format.hpp index 5700b9e0e..21702986c 100644 --- a/examples/bank/gui/controllers/Format.hpp +++ b/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::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(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 parseMinor(const QString& text, int decimals = 2) { bool ok = false; const double major = text.trimmed().toDouble(&ok); @@ -73,7 +104,14 @@ inline std::optional parseMinor(const QString& text, int decimals } // Reuse the core scale primitive so parse and format share one source. const auto scale = static_cast(bank::pow10i(decimals)); - return static_cast(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(minor); } } // namespace bankgui::fmt diff --git a/examples/bank/tests/gui/test_bank_gui_format.cpp b/examples/bank/tests/gui/test_bank_gui_format.cpp new file mode 100644 index 000000000..81da71f0f --- /dev/null +++ b/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 +#include +#include + +#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()); +}