From 6da2ed6abd447008a47a44e8981dd0a632d74aaf Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 26 Jan 2021 23:36:01 -0800 Subject: [PATCH] Diagnose if a SLEB128 is too large to fit in an int64_t. Previously we'd hit UB due to an invalid left shift operand. Also fix the WASM emitter to properly use SLEB128 encoding instead of ULEB128 encoding for signed fields so that negative numbers don't result in overly-large values that we can't read back any more. In passing, don't diagnose a non-canonical ULEB128 that fits in a uint64_t but has redundant trailing zero bytes. Reviewed By: dblaikie, aardappel Differential Revision: https://reviews.llvm.org/D95510 --- include/llvm/Support/LEB128.h | 18 +++++++--- lib/ObjectYAML/WasmEmitter.cpp | 3 +- unittests/Support/LEB128Test.cpp | 62 ++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 5 deletions(-) diff --git a/include/llvm/Support/LEB128.h b/include/llvm/Support/LEB128.h index 8ab35431354..9ba5f29b7b4 100644 --- a/include/llvm/Support/LEB128.h +++ b/include/llvm/Support/LEB128.h @@ -142,14 +142,14 @@ inline uint64_t decodeULEB128(const uint8_t *p, unsigned *n = nullptr, return 0; } uint64_t Slice = *p & 0x7f; - if (Shift >= 64 || Slice << Shift >> Shift != Slice) { + if ((Shift >= 64 && Slice != 0) || Slice << Shift >> Shift != Slice) { if (error) *error = "uleb128 too big for uint64"; if (n) *n = (unsigned)(p - orig_p); return 0; } - Value += uint64_t(*p & 0x7f) << Shift; + Value += Slice << Shift; Shift += 7; } while (*p++ >= 128); if (n) @@ -175,9 +175,19 @@ inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr, *n = (unsigned)(p - orig_p); return 0; } - Byte = *p++; - Value |= (uint64_t(Byte & 0x7f) << Shift); + Byte = *p; + int64_t Slice = Byte & 0x7f; + if ((Shift >= 64 && Slice != (Value < 0 ? 0x7f : 0x00)) || + (Shift == 63 && Slice != 0 && Slice != 0x7f)) { + if (error) + *error = "sleb128 too big for int64"; + if (n) + *n = (unsigned)(p - orig_p); + return 0; + } + Value |= Slice << Shift; Shift += 7; + ++p; } while (Byte >= 128); // Sign extend negative numbers if needed. if (Shift < 64 && (Byte & 0x40)) diff --git a/lib/ObjectYAML/WasmEmitter.cpp b/lib/ObjectYAML/WasmEmitter.cpp index c09d7f1bc95..83d4701ed3e 100644 --- a/lib/ObjectYAML/WasmEmitter.cpp +++ b/lib/ObjectYAML/WasmEmitter.cpp @@ -575,7 +575,8 @@ void WasmWriter::writeRelocSection(raw_ostream &OS, WasmYAML::Section &Sec, case wasm::R_WASM_FUNCTION_OFFSET_I32: case wasm::R_WASM_FUNCTION_OFFSET_I64: case wasm::R_WASM_SECTION_OFFSET_I32: - encodeULEB128(Reloc.Addend, OS); + encodeSLEB128(Reloc.Addend, OS); + break; } } } diff --git a/unittests/Support/LEB128Test.cpp b/unittests/Support/LEB128Test.cpp index 9a5d4cbaba3..986d2935845 100644 --- a/unittests/Support/LEB128Test.cpp +++ b/unittests/Support/LEB128Test.cpp @@ -137,10 +137,37 @@ TEST(LEB128Test, DecodeULEB128) { EXPECT_DECODE_ULEB128_EQ(0x7fu, "\xff\x80\x00"); EXPECT_DECODE_ULEB128_EQ(0x80u, "\x80\x81\x00"); EXPECT_DECODE_ULEB128_EQ(0x80u, "\x80\x81\x80\x00"); + EXPECT_DECODE_ULEB128_EQ(0x80u, "\x80\x81\x80\x80\x80\x80\x80\x80\x80\x00"); + EXPECT_DECODE_ULEB128_EQ(0x80000000'00000000ul, + "\x80\x80\x80\x80\x80\x80\x80\x80\x80\x01"); #undef EXPECT_DECODE_ULEB128_EQ } +TEST(LEB128Test, DecodeInvalidULEB128) { +#define EXPECT_INVALID_ULEB128(VALUE, ERROR_OFFSET) \ + do { \ + const uint8_t *Value = reinterpret_cast(VALUE); \ + const char *Error = nullptr; \ + unsigned ErrorOffset = 0; \ + uint64_t Actual = \ + decodeULEB128(Value, &ErrorOffset, Value + strlen(VALUE), &Error); \ + EXPECT_NE(Error, nullptr); \ + EXPECT_EQ(0ul, Actual); \ + EXPECT_EQ(ERROR_OFFSET, ErrorOffset); \ + } while (0) + + // Buffer overflow. + EXPECT_INVALID_ULEB128("", 0u); + EXPECT_INVALID_ULEB128("\x80", 1u); + + // Does not fit in 64 bits. + EXPECT_INVALID_ULEB128("\x80\x80\x80\x80\x80\x80\x80\x80\x80\x02", 9u); + EXPECT_INVALID_ULEB128("\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x02", 10u); + +#undef EXPECT_INVALID_ULEB128 +} + TEST(LEB128Test, DecodeSLEB128) { #define EXPECT_DECODE_SLEB128_EQ(EXPECTED, VALUE) \ do { \ @@ -176,10 +203,45 @@ TEST(LEB128Test, DecodeSLEB128) { EXPECT_DECODE_SLEB128_EQ(0x7fL, "\xff\x80\x00"); EXPECT_DECODE_SLEB128_EQ(0x80L, "\x80\x81\x00"); EXPECT_DECODE_SLEB128_EQ(0x80L, "\x80\x81\x80\x00"); + EXPECT_DECODE_SLEB128_EQ(0x80L, "\x80\x81\x80\x80\x80\x80\x80\x80\x80\x00"); + EXPECT_DECODE_SLEB128_EQ(-2L, "\xFE\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x7F"); + EXPECT_DECODE_SLEB128_EQ(INT64_MIN, + "\x80\x80\x80\x80\x80\x80\x80\x80\x80\x7F"); + EXPECT_DECODE_SLEB128_EQ(INT64_MAX, + "\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x00"); #undef EXPECT_DECODE_SLEB128_EQ } +TEST(LEB128Test, DecodeInvalidSLEB128) { +#define EXPECT_INVALID_SLEB128(VALUE, ERROR_OFFSET) \ + do { \ + const uint8_t *Value = reinterpret_cast(VALUE); \ + const char *Error = nullptr; \ + unsigned ErrorOffset = 0; \ + uint64_t Actual = \ + decodeSLEB128(Value, &ErrorOffset, Value + strlen(VALUE), &Error); \ + EXPECT_NE(Error, nullptr); \ + EXPECT_EQ(0ul, Actual); \ + EXPECT_EQ(ERROR_OFFSET, ErrorOffset); \ + } while (0) + + // Buffer overflow. + EXPECT_INVALID_SLEB128("", 0u); + EXPECT_INVALID_SLEB128("\x80", 1u); + + // Does not fit in 64 bits. + EXPECT_INVALID_SLEB128("\x80\x80\x80\x80\x80\x80\x80\x80\x80\x01", 9u); + EXPECT_INVALID_SLEB128("\x80\x80\x80\x80\x80\x80\x80\x80\x80\x7E", 9u); + EXPECT_INVALID_SLEB128("\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x02", 10u); + EXPECT_INVALID_SLEB128("\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x7E", 9u); + EXPECT_INVALID_SLEB128("\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x01", 9u); + EXPECT_INVALID_SLEB128("\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x7E", 10u); + EXPECT_INVALID_SLEB128("\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x00", 10u); + +#undef EXPECT_INVALID_SLEB128 +} + TEST(LEB128Test, SLEB128Size) { // Positive Value Testing Plan: // (1) 128 ^ n - 1 ........ need (n+1) bytes