From ce8a10b9c4a0e8ca89376490c7b1cc318226b0a9 Mon Sep 17 00:00:00 2001 From: Tobias Tebbi Date: Fri, 18 Mar 2022 17:25:18 +0000 Subject: [PATCH] [ia32] Avoid signed overflow undefined behavior in InstructionSelector Bug: chromium:1305925 Change-Id: I95dab2250ae60739a70c0d1f6ec30121d0ddcf8f Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3537007 Reviewed-by: Igor Sheludko Reviewed-by: Nico Hartmann Commit-Queue: Tobias Tebbi Cr-Commit-Position: refs/heads/main@{#79554} --- src/base/bits.h | 10 ++++++++ .../backend/ia32/instruction-selector-ia32.cc | 6 +++-- .../instruction-selector-ia32-unittest.cc | 25 +++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/base/bits.h b/src/base/bits.h index 5c31addd39..9fe237fb30 100644 --- a/src/base/bits.h +++ b/src/base/bits.h @@ -329,6 +329,16 @@ inline uint32_t UnsignedMod32(uint32_t lhs, uint32_t rhs) { return rhs ? lhs % rhs : 0u; } +// Wraparound integer arithmetic without undefined behavior. + +inline int32_t WraparoundAdd32(int32_t lhs, int32_t rhs) { + return static_cast(static_cast(lhs) + + static_cast(rhs)); +} + +inline int32_t WraparoundNeg32(int32_t x) { + return static_cast(-static_cast(x)); +} // SignedSaturatedAdd64(lhs, rhs) adds |lhs| and |rhs|, // checks and returns the result. diff --git a/src/compiler/backend/ia32/instruction-selector-ia32.cc b/src/compiler/backend/ia32/instruction-selector-ia32.cc index 3a4fb705b6..1efc9f812c 100644 --- a/src/compiler/backend/ia32/instruction-selector-ia32.cc +++ b/src/compiler/backend/ia32/instruction-selector-ia32.cc @@ -9,6 +9,7 @@ #include #include +#include "src/base/bits.h" #include "src/base/flags.h" #include "src/base/iterator.h" #include "src/base/logging.h" @@ -132,11 +133,12 @@ class IA32OperandGenerator final : public OperandGenerator { size_t* input_count, RegisterMode register_mode = kRegister) { AddressingMode mode = kMode_MRI; if (displacement_mode == kNegativeDisplacement) { - displacement = -displacement; + displacement = base::bits::WraparoundNeg32(displacement); } if (base != nullptr) { if (base->opcode() == IrOpcode::kInt32Constant) { - displacement += OpParameter(base->op()); + displacement = base::bits::WraparoundAdd32( + displacement, OpParameter(base->op())); base = nullptr; } } diff --git a/test/unittests/compiler/ia32/instruction-selector-ia32-unittest.cc b/test/unittests/compiler/ia32/instruction-selector-ia32-unittest.cc index b6376ff280..d126c4d7d0 100644 --- a/test/unittests/compiler/ia32/instruction-selector-ia32-unittest.cc +++ b/test/unittests/compiler/ia32/instruction-selector-ia32-unittest.cc @@ -901,6 +901,31 @@ TEST_F(InstructionSelectorTest, SIMDSplatZero) { } } +TEST_F(InstructionSelectorTest, Int32AddMinNegativeDisplacement) { + // This test case is simplified from a Wasm fuzz test in + // https://crbug.com/1091892. The key here is that we match on a + // sequence like: Int32Add(Int32Sub(-524288, -2147483648), -26048), which + // matches on an EmitLea, with -2147483648 as the displacement. Since we + // have a Int32Sub node, it sets kNegativeDisplacement, and later we try to + // negate -2147483648, which overflows. + StreamBuilder m(this, MachineType::Int32()); + Node* const c0 = m.Int32Constant(-524288); + Node* const c1 = m.Int32Constant(std::numeric_limits::min()); + Node* const c2 = m.Int32Constant(-26048); + Node* const a0 = m.Int32Sub(c0, c1); + Node* const a1 = m.Int32Add(a0, c2); + m.Return(a1); + Stream s = m.Build(); + ASSERT_EQ(1U, s.size()); + + EXPECT_EQ(kIA32Lea, s[0]->arch_opcode()); + ASSERT_EQ(2U, s[0]->InputCount()); + EXPECT_EQ(kMode_MRI, s[0]->addressing_mode()); + EXPECT_TRUE(s[0]->InputAt(1)->IsImmediate()); + EXPECT_EQ(2147457600, + ImmediateOperand::cast(s[0]->InputAt(1))->inline_int32_value()); +} + struct SwizzleConstants { uint8_t shuffle[kSimd128Size]; bool omit_add;