From 17e83de54f7fc2f93c50d50555dddce64d8681bc Mon Sep 17 00:00:00 2001 From: Antonio Maiorano Date: Fri, 3 Sep 2021 19:40:36 +0000 Subject: [PATCH] Fix UB when parsing HexFloat with large exponents During HexFloat parsing, if exponent was too large, we would overflow the signed integer being used to store its value. We now use an uint32_t to avoid UB, then convert to int32_t when it's safe to do so. Also error out if the input exponent is > INT_MAX - 127, which ensures we will not wrap around and produce an invalid result when adding the exponent bias of 127. Bug: chromium:1240048 Bug: tint:1150 Change-Id: I1b57b2c965358b803ebb68ea70b76e759cdd3939 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/63120 Reviewed-by: David Neto Kokoro: Kokoro Commit-Queue: Antonio Maiorano --- src/reader/wgsl/lexer.cc | 37 ++++++++++++++----- .../wgsl/parser_impl_const_literal_test.cc | 6 ++- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/reader/wgsl/lexer.cc b/src/reader/wgsl/lexer.cc index 74afcf6b66..600585a762 100644 --- a/src/reader/wgsl/lexer.cc +++ b/src/reader/wgsl/lexer.cc @@ -305,7 +305,7 @@ Token Lexer::try_hex_float() { } uint32_t mantissa = 0; - int32_t exponent = 0; + uint32_t exponent = 0; // `set_next_mantissa_bit_to` sets next `mantissa` bit starting from msb to // lsb to value 1 if `set` is true, 0 otherwise @@ -428,12 +428,23 @@ Token Lexer::try_hex_float() { has_exponent = true; auto prev_exponent = input_exponent; input_exponent = (input_exponent * 10) + dec_value(content_->data[end]); + // Check if we've overflowed input_exponent if (prev_exponent > input_exponent) { return {Token::Type::kError, source, "exponent is too large for hex float"}; } end++; } + + // Ensure input exponent is not too large; i.e. that it won't overflow when + // adding the exponent bias. + const uint32_t kIntMax = + static_cast(std::numeric_limits::max()); + const uint32_t kMaxInputExponent = kIntMax - kExponentBias; + if (input_exponent > kMaxInputExponent) { + return {Token::Type::kError, source, "exponent is too large for hex float"}; + } + if (!has_exponent) { return {Token::Type::kError, source, "expected an exponent value for hex float"}; @@ -444,7 +455,8 @@ Token Lexer::try_hex_float() { end_source(source); // Compute exponent so far - exponent = exponent + (input_exponent * exponent_sign); + exponent += static_cast(static_cast(input_exponent) * + exponent_sign); // Determine if value is zero // Note: it's not enough to check mantissa == 0 as we drop initial bit from @@ -467,6 +479,10 @@ Token Lexer::try_hex_float() { } } + // We can now safely work with exponent as a signed quantity, as there's no + // chance to overflow + int32_t signed_exponent = static_cast(exponent); + // Shift mantissa to occupy the low 23 bits mantissa >>= kMantissaShiftRight; @@ -474,27 +490,27 @@ Token Lexer::try_hex_float() { if (!is_zero) { // Denorm has exponent 0 and non-zero mantissa. We set the top bit here, // then shift the mantissa to make exponent zero. - if (exponent <= 0) { + if (signed_exponent <= 0) { mantissa >>= 1; mantissa |= (1 << kMantissaMsb); } - while (exponent < 0) { + while (signed_exponent < 0) { mantissa >>= 1; - ++exponent; + ++signed_exponent; // If underflow, clamp to zero if (mantissa == 0) { - exponent = 0; + signed_exponent = 0; } } } - if (exponent > kExponentMax) { + if (signed_exponent > kExponentMax) { // Overflow: set to infinity - exponent = kExponentMax; + signed_exponent = kExponentMax; mantissa = 0; - } else if (exponent == kExponentMax && mantissa != 0) { + } else if (signed_exponent == kExponentMax && mantissa != 0) { // NaN: set to infinity mantissa = 0; } @@ -502,7 +518,8 @@ Token Lexer::try_hex_float() { // Combine sign, mantissa, and exponent uint32_t result_u32 = sign_bit << kSignBit; result_u32 |= mantissa; - result_u32 |= (exponent & kExponentMask) << kExponentLeftShift; + result_u32 |= (static_cast(signed_exponent) & kExponentMask) + << kExponentLeftShift; // Reinterpret as float and return float result; diff --git a/src/reader/wgsl/parser_impl_const_literal_test.cc b/src/reader/wgsl/parser_impl_const_literal_test.cc index 941e8f59ef..7da4945f45 100644 --- a/src/reader/wgsl/parser_impl_const_literal_test.cc +++ b/src/reader/wgsl/parser_impl_const_literal_test.cc @@ -220,6 +220,7 @@ FloatLiteralTestCase hexfloat_literal_test_cases[] = { {"0x1.1p+128", PosInf}, {"-0x1p+129", NegInf}, {"-0x1.1p+128", NegInf}, + {"0x1.0p2147483520", PosInf}, // INT_MAX - 127 (largest valid exponent) // Underflow -> Zero {"0x1p-500", 0.f}, // Exponent underflows @@ -227,7 +228,8 @@ FloatLiteralTestCase hexfloat_literal_test_cases[] = { {"0x0.00000000001p-126", 0.f}, // Fraction causes underflow {"-0x0.0000000001p-127", -0.f}, {"0x0.01p-142", 0.f}, - {"-0x0.01p-142", -0.f}, // Fraction causes additional underflow + {"-0x0.01p-142", -0.f}, // Fraction causes additional underflow + {"0x1.0p-2147483520", 0}, // -(INT_MAX - 127) (smallest valid exponent) // Zero with non-zero exponent -> Zero {"0x0p+0", 0.f}, @@ -295,6 +297,8 @@ INSTANTIATE_TEST_SUITE_P( testing::ValuesIn(invalid_hexfloat_mantissa_too_large_cases)); InvalidLiteralTestCase invalid_hexfloat_exponent_too_large_cases[] = { + {"0x0p+2147483521", "1:1: exponent is too large for hex float"}, + {"0x0p-2147483521", "1:1: exponent is too large for hex float"}, {"0x0p+4294967296", "1:1: exponent is too large for hex float"}, {"0x0p-4294967296", "1:1: exponent is too large for hex float"}, };