From 028eb0a96666a5e75aeeb3f4c0247b5ee6061212 Mon Sep 17 00:00:00 2001 From: sideshowbarker Date: Sun, 24 May 2026 20:31:25 +0900 Subject: [PATCH] =?UTF-8?q?AK:=20Don=E2=80=99t=20let=20float=20precision?= =?UTF-8?q?=20sneak=20past=20is=5Fwithin=5Frange=20bounds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: is_within_range(F value) — where I is an integer and F is a floating-point type — is unexpectedly too permissive in some cases: a. Values that are 1 past the integer range unexpectedly pass; e.g., is_within_range(2147483648.0f) returns true — even though 2147483648 is INT_MAX + 1. b. Fractional values whose magnitude exceeds the destination max unexpectedly pass; e.g., is_within_range(4294967295.5) returns true — even though 4294967295.5 > UINT_MAX. c. Fractional values within the destination’s numeric range unexpectedly pass (e.g., is_within_range(2.5) returns true) — even though they aren’t exactly representable as the destination type. Cause: TypeBoundsChecker integer-bounds specializations compare against NumericLimits::max() and ::min() directly. When a caller’s value is a float, the integer max/min get implicitly converted to a float for the comparison. For Destination/Source pairs with the integer extreme not exactly representable in the float, that conversion rounds up to the next power-of-two boundary — so “value <= F(max)” accepts values that are actually out of range by one (case a). And the comparison itself doesn’t reject fractional values (cases b and c). Fix: When Source is a floating-point type: 1. First gate (case a) — Compare against 2^digits; exactly representable in any IEEE float, and equals max + 1 for unsigned / -min for two’s- complement signed integers. 2. Second gate (cases b and c) – Round-trip check: cast value to Destination, then cast back — and require equality. Only integer- valued floats whose truncation matches the original pass. Fixes https://github.com/LadybirdBrowser/ladybird/issues/6212 --- AK/Checked.h | 46 +++++++++++++++++++++++++++++++++++++--- AK/JsonValue.h | 13 +++--------- Tests/AK/TestChecked.cpp | 42 ++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 13 deletions(-) diff --git a/AK/Checked.h b/AK/Checked.h index c339ecb24d..f13ec91158 100644 --- a/AK/Checked.h +++ b/AK/Checked.h @@ -45,12 +45,41 @@ struct TypeBoundsChecker { } }; +// Computes 2^N as a floating-point Source value. The result is exactly representable in any IEEE floating-point type +// for any N within that type's exponent range — since 2^N has mantissa 1.0. +template +static constexpr Source two_to_the(size_t n) +{ + Source result = 1; + for (size_t i = 0; i < n; ++i) + result *= 2; + return result; +} + template struct TypeBoundsChecker { static constexpr bool is_within_range(Source value) { - return value <= NumericLimits::max() - && NumericLimits::min() <= value; + if constexpr (IsFloatingPoint) { + // The value must lie in [Destination::min, Destination::max] *and* must be exactly representable as + // Destination; that is, the float must be integer-valued — with no fractional part. + // + // First gate: the value must fit in the safe-cast window — so static_cast below is defined. + // Comparing directly against NumericLimits::max() doesn't work when max isn't exactly + // representable in Source (e.g., INT_MAX = 2^31-1 in float rounds up to 2^31). Use 2^digits as a strict + // upper bound instead: exactly representable in any IEEE float — covering the range [-2^digits, 2^digits) + // for two's-complement signed destinations. + auto const boundary = two_to_the(NumericLimits::digits()); + if (!(value >= -boundary && value < boundary)) + return false; + // Second gate: reject fractional values (round-trip check). Values in (-boundary, boundary) with a + // fractional part would truncate to an in-range integer — but aren't themselves within Destination's range. + auto const truncated = static_cast(value); + return static_cast(truncated) == value; + } else { + return value <= NumericLimits::max() + && NumericLimits::min() <= value; + } } }; @@ -58,7 +87,18 @@ template struct TypeBoundsChecker { static constexpr bool is_within_range(Source value) { - return value >= 0 && value <= NumericLimits::max(); + if constexpr (IsFloatingPoint) { + // See the signed case above. The first gate is [0, 2^digits). 2^digits is exactly representable in any IEEE + // float. The second gate is the round-trip check: rejecting fractional values whose magnitudes exceed + // Destination::max. + auto const upper_exclusive = two_to_the(NumericLimits::digits()); + if (!(value >= 0 && value < upper_exclusive)) + return false; + auto const truncated = static_cast(value); + return static_cast(truncated) == value; + } else { + return value >= 0 && value <= NumericLimits::max(); + } } }; diff --git a/AK/JsonValue.h b/AK/JsonValue.h index 9c5a01c047..70d92a67be 100644 --- a/AK/JsonValue.h +++ b/AK/JsonValue.h @@ -198,16 +198,9 @@ public: return m_value.visit( [](bool const&) { return Optional {}; }, [](U const& value) -> Optional { - if constexpr (Integral) { - if (!is_within_range(value)) - return {}; - return static_cast(value); - } else { - // FIXME: Make is_within_range work with floating point numbers. - if (static_cast(static_cast(value)) != value) - return {}; - return static_cast(value); - } + if (!is_within_range(value)) + return {}; + return static_cast(value); }, [](auto const&) { return Optional {}; }); } diff --git a/Tests/AK/TestChecked.cpp b/Tests/AK/TestChecked.cpp index 51821c3b40..77b92ab4ee 100644 --- a/Tests/AK/TestChecked.cpp +++ b/Tests/AK/TestChecked.cpp @@ -460,4 +460,46 @@ TEST_CASE(is_within_range_float_to_int) static_assert(AK::is_within_range(0.0)); static_assert(!AK::is_within_range(NAN)); + + // Boundary checks: implicit float<->integer conversion can round the integer up to a value that's one past the + // actual destination range — so naive 'value <= max()' is too permissive. These cases must report false. + static_assert(!AK::is_within_range(2147483648.0)); // INT_MAX + 1, exact in double + static_assert(!AK::is_within_range(2147483648.0f)); // INT_MAX + 1 = 2^31, exact in float + static_assert(AK::is_within_range(-2147483648.0)); // INT_MIN, exact in double + static_assert(AK::is_within_range(-2147483648.0f)); // INT_MIN, exact in float + static_assert(!AK::is_within_range(-2147483649.0)); // INT_MIN - 1 + + static_assert(AK::is_within_range(4294967295.0)); // UINT_MAX, exact in double + static_assert(!AK::is_within_range(4294967296.0)); // UINT_MAX + 1, exact in double + static_assert(!AK::is_within_range(4294967296.0f)); // UINT_MAX + 1 = 2^32, exact in float + static_assert(!AK::is_within_range(-1.0)); + + static_assert(AK::is_within_range(127.0f)); + static_assert(!AK::is_within_range(128.0f)); + static_assert(AK::is_within_range(-128.0f)); + static_assert(!AK::is_within_range(-129.0f)); + + static_assert(AK::is_within_range(255.0f)); + static_assert(!AK::is_within_range(256.0f)); + + // For 64-bit destinations the boundary is at the precision limit of double. + static_assert(!AK::is_within_range(9223372036854775808.0)); // INT64_MAX + 1 = 2^63 + static_assert(AK::is_within_range(-9223372036854775808.0)); // INT64_MIN + static_assert(!AK::is_within_range(18446744073709551616.0)); // UINT64_MAX + 1 = 2^64 + + // Fractional floats are rejected. A value like 4294967295.5 isn't representable as an unsigned int (it has a + // fractional part) — even though its truncation would fit. + static_assert(!AK::is_within_range(4294967295.5)); // 4294967295.5 > UINT_MAX + static_assert(!AK::is_within_range(4294967295.5f)); // literal also rounds to 2^32, OOB + static_assert(!AK::is_within_range(2147483647.5)); // 2147483647.5 > INT_MAX + static_assert(!AK::is_within_range(2147483647.5f)); // literal rounds to 2^31, OOB + static_assert(!AK::is_within_range(2.5)); // fractional, even well within INT bounds + static_assert(!AK::is_within_range(-2.5)); // negative fractional, same reason + static_assert(!AK::is_within_range(-0.5)); // negative, can't be unsigned + + // Integer-valued floats well within range remain in range. + static_assert(AK::is_within_range(0.0)); + static_assert(AK::is_within_range(-2147483648.0)); // INT_MIN + static_assert(AK::is_within_range(2147483647.0)); // INT_MAX + static_assert(AK::is_within_range(4294967295.0)); // UINT_MAX }