From 32e98f05fe108e7b25dcf2031c499b96a1436e1d 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 --- llvm/include/llvm/Support/LEB128.h | 18 +++++++--- llvm/lib/ObjectYAML/WasmEmitter.cpp | 3 +- llvm/unittests/Support/LEB128Test.cpp | 62 +++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 5 deletions(-) diff --git a/llvm/include/llvm/Support/LEB128.h b/llvm/include/llvm/Support/LEB128.h index 8ab3543..9ba5f29 100644 --- a/llvm/include/llvm/Support/LEB128.h +++ b/llvm/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/llvm/lib/ObjectYAML/WasmEmitter.cpp b/llvm/lib/ObjectYAML/WasmEmitter.cpp index c09d7f1..83d4701 100644 --- a/llvm/lib/ObjectYAML/WasmEmitter.cpp +++ b/llvm/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/llvm/unittests/Support/LEB128Test.cpp b/llvm/unittests/Support/LEB128Test.cpp index 9a5d4cb..986d293 100644 --- a/llvm/unittests/Support/LEB128Test.cpp +++ b/llvm/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 -- 2.7.4