From 6c21e7be6217817485dae2b1caaafaff2265716b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 17:11:23 +0200 Subject: [PATCH] bank/gui: bound parseMinor's input so an amount out of int64 range is rejected rather than undefined (fixes #663) `bankgui::fmt::parseMinor` guarded only `!ok || major < 0.0` and then cast `major * scale + 0.5` to `std::int64_t`. `QString::toDouble` accepts `1e30`, `inf` and `nan`, none of the six call sites bounds its input, and five of them are GUI controllers reading a QML text field that carries no validator -- so every amount a user types reached an out-of-range floating-to-integer conversion, which is undefined behaviour, not a large number. Reproduced on 563502ab with the header as it stood, clang 22.1.8, `-fsanitize=undefined -fno-sanitize-recover=undefined`: examples/bank/gui/controllers/Format.hpp:76:38: runtime error: 1e+32 is outside the range of representable values of type 'long' and without a sanitizer, `parseMinor("1e30")`, `parseMinor("inf")`, `parseMinor("nan")` and `parseMinor("9.3e16")` each returned `-9223372036854775808`, which every call site then fed through `.value_or(0)` into a balance. The fix rejects the input rather than clamping the result: `std::nullopt` is what the function already returns for unparseable text and for negatives, and every caller already handles it. The bound is checked on the scaled value against `0x1p63` rather than on `text`'s value against `numeric_limits::max() / scale`, because the latter is not exact -- `max()` is 2^63-1, which is not representable as a `double` and converts *upwards* to 2^63, and dividing that by `scale` rounds again. 2^63 is exactly representable, and `double` arithmetic cannot trap, so scaling first and comparing against it is the one form of the bound with no rounding in it. The comparison is written negated so that a NaN -- which reaches this point because `nan < 0.0` is false -- is rejected rather than admitted. Verified: a boundary sweep of 24576 doubles straddling the accept/reject edge for `decimals` 0, 1 and 2, under `-fsanitize=undefined -fno-sanitize-recover=undefined`, accepts exactly the values whose scaled form is below 2^63 and raises no diagnostic (12288 accepted, 12288 rejected). The new `tests/gui/test_bank_gui_format.cpp` joins `bank_gui_tests`, which links Qt6::Core and needs no engine or platform plugin. Its cases assert on the returned `optional`, not on the arithmetic: under UBSan the unfixed function aborts rather than returning a wrong answer, so a test that checked the value would report the defect as a crash on one configuration and as nothing at all on the others. Against the pre-fix header the suite fails 7 assertions in 2 of its 4 test cases (exit 42) without a sanitizer, and aborts on the first `1e30` case with one. The `+ 0.5` rounding note in #663 is labelled weak by the filer and is a separate question; it is pinned by a test here and otherwise left alone rather than folded into a UB fix. One consequence of that is worth stating rather than leaving to be found: the old line carried a `bugprone-incorrect-roundings` finding ("casting (double + 0.5) to integer leads to incorrect rounding; consider using lround"), and splitting the expression to introduce the bound stops that check matching. The rounding is unchanged, the check no longer says so, and the question it was pointing at is not settled here. Filed separately. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- examples/bank/CMakeLists.txt | 1 + examples/bank/gui/controllers/Format.hpp | 42 ++++++++- .../bank/tests/gui/test_bank_gui_format.cpp | 93 +++++++++++++++++++ 3 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 examples/bank/tests/gui/test_bank_gui_format.cpp 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()); +}