From bbfbcfbfb280951d9c81b42252c5d8a7d0f1646e Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Mon, 20 Apr 2020 17:28:15 +0200 Subject: [PATCH] [Support] Make DataExtractor error messages more clear Summary: This is a result of the discussion at D78113. Previously we would be only giving the current offset at which the error was detected. However, this was phrased somewhat ambiguously (as it could also mean that end of data was at that offset). The new error message includes the current offset as well as the extent of the data being read. I've changed a couple of file-level static functions into private member functions in order to avoid passing a bunch of new arguments everywhere. Reviewers: dblaikie, jhenderson Subscribers: hiraditya, MaskRay, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D78558 --- include/llvm/Support/DataExtractor.h | 10 +++ lib/Support/DataExtractor.cpp | 88 +++++++++---------- .../X86/dwarfdump-debug-loc-error-cases2.s | 4 +- .../dwarfdump-debug-loclists-error-cases2.s | 4 +- ...addr_too_small_for_extended_length_field.s | 2 +- .../debug_addr_too_small_for_length_field.s | 2 +- .../X86/debug_rnglists_invalid.s | 2 +- .../DWARF/DWARFAcceleratorTableTest.cpp | 2 +- .../DWARF/DWARFDataExtractorTest.cpp | 36 +++++--- .../DWARF/DWARFDebugArangeSetTest.cpp | 4 +- .../DebugInfo/DWARF/DWARFDebugLineTest.cpp | 5 +- unittests/DebugInfo/DWARF/DWARFDieTest.cpp | 3 +- unittests/Support/DataExtractorTest.cpp | 11 ++- 13 files changed, 102 insertions(+), 71 deletions(-) diff --git a/include/llvm/Support/DataExtractor.h b/include/llvm/Support/DataExtractor.h index eb0edd11aa9..f9335c16156 100644 --- a/include/llvm/Support/DataExtractor.h +++ b/include/llvm/Support/DataExtractor.h @@ -689,6 +689,16 @@ protected: // public. static uint64_t &getOffset(Cursor &C) { return C.Offset; } static Error &getError(Cursor &C) { return C.Err; } + +private: + /// If it is possible to read \a Size bytes at offset \a Offset, returns \b + /// true. Otherwise, returns \b false. If \a E is not nullptr, also sets the + /// error object to indicate an error. + bool prepareRead(uint64_t Offset, uint64_t Size, Error *E) const; + + template T getU(uint64_t *OffsetPtr, Error *Err) const; + template + T *getUs(uint64_t *OffsetPtr, T *Dst, uint32_t Count, Error *Err) const; }; } // namespace llvm diff --git a/lib/Support/DataExtractor.cpp b/lib/Support/DataExtractor.cpp index 99265c4c001..133d674275e 100644 --- a/lib/Support/DataExtractor.cpp +++ b/lib/Support/DataExtractor.cpp @@ -15,30 +15,40 @@ using namespace llvm; -static void unexpectedEndReached(Error *E, uint64_t Offset) { - if (E) - *E = createStringError(errc::illegal_byte_sequence, - "unexpected end of data at offset 0x%" PRIx64, - Offset); +bool DataExtractor::prepareRead(uint64_t Offset, uint64_t Size, + Error *E) const { + if (isValidOffsetForDataOfSize(Offset, Size)) + return true; + if (E) { + if (Offset <= Data.size()) + *E = createStringError( + errc::illegal_byte_sequence, + "unexpected end of data at offset 0x%zx while reading [0x%" PRIx64 + ", 0x%" PRIx64 ")", + Data.size(), Offset, Offset + Size); + else + *E = createStringError(errc::invalid_argument, + "offset 0x%" PRIx64 + " is beyond the end of data at 0x%zx", + Offset, Data.size()); + } + return false; } static bool isError(Error *E) { return E && *E; } template -static T getU(uint64_t *offset_ptr, const DataExtractor *de, - bool isLittleEndian, const char *Data, llvm::Error *Err) { +T DataExtractor::getU(uint64_t *offset_ptr, Error *Err) const { ErrorAsOutParameter ErrAsOut(Err); T val = 0; if (isError(Err)) return val; uint64_t offset = *offset_ptr; - if (!de->isValidOffsetForDataOfSize(offset, sizeof(T))) { - unexpectedEndReached(Err, offset); + if (!prepareRead(offset, sizeof(T), Err)) return val; - } - std::memcpy(&val, &Data[offset], sizeof(val)); - if (sys::IsLittleEndianHost != isLittleEndian) + std::memcpy(&val, &Data.data()[offset], sizeof(val)); + if (sys::IsLittleEndianHost != IsLittleEndian) sys::swapByteOrder(val); // Advance the offset @@ -47,22 +57,19 @@ static T getU(uint64_t *offset_ptr, const DataExtractor *de, } template -static T *getUs(uint64_t *offset_ptr, T *dst, uint32_t count, - const DataExtractor *de, bool isLittleEndian, const char *Data, - llvm::Error *Err) { +T *DataExtractor::getUs(uint64_t *offset_ptr, T *dst, uint32_t count, + Error *Err) const { ErrorAsOutParameter ErrAsOut(Err); if (isError(Err)) return nullptr; uint64_t offset = *offset_ptr; - if (!de->isValidOffsetForDataOfSize(offset, sizeof(*dst) * count)) { - unexpectedEndReached(Err, offset); + if (!prepareRead(offset, sizeof(*dst) * count, Err)) return nullptr; - } for (T *value_ptr = dst, *end = dst + count; value_ptr != end; ++value_ptr, offset += sizeof(*dst)) - *value_ptr = getU(offset_ptr, de, isLittleEndian, Data, Err); + *value_ptr = getU(offset_ptr, Err); // Advance the offset *offset_ptr = offset; // Return a non-NULL pointer to the converted data as an indicator of @@ -71,55 +78,49 @@ static T *getUs(uint64_t *offset_ptr, T *dst, uint32_t count, } uint8_t DataExtractor::getU8(uint64_t *offset_ptr, llvm::Error *Err) const { - return getU(offset_ptr, this, IsLittleEndian, Data.data(), Err); + return getU(offset_ptr, Err); } -uint8_t * -DataExtractor::getU8(uint64_t *offset_ptr, uint8_t *dst, uint32_t count) const { - return getUs(offset_ptr, dst, count, this, IsLittleEndian, - Data.data(), nullptr); +uint8_t *DataExtractor::getU8(uint64_t *offset_ptr, uint8_t *dst, + uint32_t count) const { + return getUs(offset_ptr, dst, count, nullptr); } uint8_t *DataExtractor::getU8(Cursor &C, uint8_t *Dst, uint32_t Count) const { - return getUs(&C.Offset, Dst, Count, this, IsLittleEndian, - Data.data(), &C.Err); + return getUs(&C.Offset, Dst, Count, &C.Err); } uint16_t DataExtractor::getU16(uint64_t *offset_ptr, llvm::Error *Err) const { - return getU(offset_ptr, this, IsLittleEndian, Data.data(), Err); + return getU(offset_ptr, Err); } uint16_t *DataExtractor::getU16(uint64_t *offset_ptr, uint16_t *dst, uint32_t count) const { - return getUs(offset_ptr, dst, count, this, IsLittleEndian, - Data.data(), nullptr); + return getUs(offset_ptr, dst, count, nullptr); } uint32_t DataExtractor::getU24(uint64_t *OffsetPtr, Error *Err) const { - uint24_t ExtractedVal = - getU(OffsetPtr, this, IsLittleEndian, Data.data(), Err); + uint24_t ExtractedVal = getU(OffsetPtr, Err); // The 3 bytes are in the correct byte order for the host. return ExtractedVal.getAsUint32(sys::IsLittleEndianHost); } uint32_t DataExtractor::getU32(uint64_t *offset_ptr, llvm::Error *Err) const { - return getU(offset_ptr, this, IsLittleEndian, Data.data(), Err); + return getU(offset_ptr, Err); } uint32_t *DataExtractor::getU32(uint64_t *offset_ptr, uint32_t *dst, uint32_t count) const { - return getUs(offset_ptr, dst, count, this, IsLittleEndian, - Data.data(), nullptr); + return getUs(offset_ptr, dst, count, nullptr); } uint64_t DataExtractor::getU64(uint64_t *offset_ptr, llvm::Error *Err) const { - return getU(offset_ptr, this, IsLittleEndian, Data.data(), Err); + return getU(offset_ptr, Err); } uint64_t *DataExtractor::getU64(uint64_t *offset_ptr, uint64_t *dst, uint32_t count) const { - return getUs(offset_ptr, dst, count, this, IsLittleEndian, - Data.data(), nullptr); + return getUs(offset_ptr, dst, count, nullptr); } uint64_t DataExtractor::getUnsigned(uint64_t *offset_ptr, uint32_t byte_size, @@ -163,7 +164,10 @@ StringRef DataExtractor::getCStrRef(uint64_t *OffsetPtr, Error *Err) const { *OffsetPtr = Pos + 1; return StringRef(Data.data() + Start, Pos - Start); } - unexpectedEndReached(Err, Start); + if (Err) + *Err = createStringError(errc::illegal_byte_sequence, + "no null terminated string at offset 0x%" PRIx64, + Start); return StringRef(); } @@ -180,10 +184,8 @@ StringRef DataExtractor::getBytes(uint64_t *OffsetPtr, uint64_t Length, if (isError(Err)) return StringRef(); - if (!isValidOffsetForDataOfSize(*OffsetPtr, Length)) { - unexpectedEndReached(Err, *OffsetPtr); + if (!prepareRead(*OffsetPtr, Length, Err)) return StringRef(); - } StringRef Result = Data.substr(*OffsetPtr, Length); *OffsetPtr += Length; @@ -229,8 +231,6 @@ void DataExtractor::skip(Cursor &C, uint64_t Length) const { if (isError(&C.Err)) return; - if (isValidOffsetForDataOfSize(C.Offset, Length)) + if (prepareRead(C.Offset, Length, &C.Err)) C.Offset += Length; - else - unexpectedEndReached(&C.Err, C.Offset); } diff --git a/test/DebugInfo/X86/dwarfdump-debug-loc-error-cases2.s b/test/DebugInfo/X86/dwarfdump-debug-loc-error-cases2.s index 8a1abd6684d..b6eae6f1114 100644 --- a/test/DebugInfo/X86/dwarfdump-debug-loc-error-cases2.s +++ b/test/DebugInfo/X86/dwarfdump-debug-loc-error-cases2.s @@ -9,11 +9,11 @@ # CHECK: DW_AT_name ("x1") # CHECK-NEXT: DW_AT_location (0xdeadbeef: ) -# ERR: error: unexpected end of data at offset 0xdeadbeef +# ERR: error: offset 0xdeadbeef is beyond the end of data at 0x48 # CHECK: DW_AT_name ("x2") # CHECK-NEXT: DW_AT_location (0x00000036: ) -# ERR: error: unexpected end of data at offset 0x48 +# ERR: error: unexpected end of data at offset 0x48 while reading [0x48, 0xdef5) .type f,@function diff --git a/test/DebugInfo/X86/dwarfdump-debug-loclists-error-cases2.s b/test/DebugInfo/X86/dwarfdump-debug-loclists-error-cases2.s index 14dc7f49d68..39bbeeb9d0c 100644 --- a/test/DebugInfo/X86/dwarfdump-debug-loclists-error-cases2.s +++ b/test/DebugInfo/X86/dwarfdump-debug-loclists-error-cases2.s @@ -9,11 +9,11 @@ # CHECK: DW_AT_name ("x1") # CHECK-NEXT: DW_AT_location (0xdeadbeef: ) -# ERR: error: unexpected end of data at offset 0xdeadbeef +# ERR: error: offset 0xdeadbeef is beyond the end of data at 0x34 # CHECK: DW_AT_name ("x2") # CHECK-NEXT: DW_AT_location (0x00000025 -# ERR: error: unexpected end of data at offset 0x34 +# ERR: error: unexpected end of data at offset 0x34 while reading [0x34, 0xdeadbf23) .type f,@function diff --git a/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_extended_length_field.s b/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_extended_length_field.s index 1034c97f764..74f1e09f93d 100644 --- a/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_extended_length_field.s +++ b/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_extended_length_field.s @@ -4,7 +4,7 @@ # CHECK: .debug_addr contents: # CHECK-NOT: {{.}} -# ERR: parsing address table at offset 0x0: unexpected end of data at offset 0x4 +# ERR: parsing address table at offset 0x0: unexpected end of data at offset 0xb while reading [0x4, 0xc) # ERR-NOT: {{.}} # too small section to contain an extended length field of a DWARF64 address table. diff --git a/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_length_field.s b/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_length_field.s index bc840ebf7fa..72774494ca1 100644 --- a/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_length_field.s +++ b/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_length_field.s @@ -4,7 +4,7 @@ # CHECK: .debug_addr contents: # CHECK-NOT: {{.}} -# ERR: parsing address table at offset 0x0: unexpected end of data at offset 0x0 +# ERR: parsing address table at offset 0x0: unexpected end of data at offset 0x2 while reading [0x0, 0x4) # ERR-NOT: {{.}} # too small section to contain length field diff --git a/test/tools/llvm-dwarfdump/X86/debug_rnglists_invalid.s b/test/tools/llvm-dwarfdump/X86/debug_rnglists_invalid.s index d176363bebb..ad4d7dd5765 100644 --- a/test/tools/llvm-dwarfdump/X86/debug_rnglists_invalid.s +++ b/test/tools/llvm-dwarfdump/X86/debug_rnglists_invalid.s @@ -2,7 +2,7 @@ # RUN: not llvm-dwarfdump --debug-rnglists - 2>&1 | FileCheck %s --check-prefix=SHORT # SHORT-NOT: error: # SHORT-NOT: range list header -# SHORT: error: parsing .debug_rnglists table at offset 0x0: unexpected end of data at offset 0x0 +# SHORT: error: parsing .debug_rnglists table at offset 0x0: unexpected end of data at offset 0x3 # SHORT-NOT: range list header # SHORT-NOT: error: diff --git a/unittests/DebugInfo/DWARF/DWARFAcceleratorTableTest.cpp b/unittests/DebugInfo/DWARF/DWARFAcceleratorTableTest.cpp index 0d68083407d..38f3e076ace 100644 --- a/unittests/DebugInfo/DWARF/DWARFAcceleratorTableTest.cpp +++ b/unittests/DebugInfo/DWARF/DWARFAcceleratorTableTest.cpp @@ -43,7 +43,7 @@ TEST(DWARFDebugNames, TooSmallForDWARF64) { ExtractDebugNames(StringRef(NamesSecData, sizeof(NamesSecData)), StringRef()), FailedWithMessage("parsing .debug_names header at 0x0: unexpected end of " - "data at offset 0x28")); + "data at offset 0x2b while reading [0x28, 0x2c)")); } } // end anonymous namespace diff --git a/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp b/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp index d6b13c773d6..449442f36cf 100644 --- a/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp +++ b/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp @@ -61,8 +61,10 @@ Symbols: DataExtractor::Cursor C(0); EXPECT_EQ(0x42u, Data.getRelocatedAddress(C)); EXPECT_EQ(0u, Data.getRelocatedAddress(C)); - EXPECT_THAT_ERROR(C.takeError(), - FailedWithMessage("unexpected end of data at offset 0x4")); + EXPECT_THAT_ERROR( + C.takeError(), + FailedWithMessage( + "unexpected end of data at offset 0x6 while reading [0x4, 0x8)")); } TEST(DWARFDataExtractorTest, getInitialLength) { @@ -94,13 +96,15 @@ TEST(DWARFDataExtractorTest, getInitialLength) { // Empty data. EXPECT_THAT_EXPECTED( GetWithError({}), - FailedWithMessage("unexpected end of data at offset 0x0")); + FailedWithMessage( + "unexpected end of data at offset 0x0 while reading [0x0, 0x4)")); EXPECT_EQ(GetWithoutError({}), ErrorResult); // Not long enough for the U32 field. EXPECT_THAT_EXPECTED( GetWithError({0x00, 0x01, 0x02}), - FailedWithMessage("unexpected end of data at offset 0x0")); + FailedWithMessage( + "unexpected end of data at offset 0x3 while reading [0x0, 0x4)")); EXPECT_EQ(GetWithoutError({0x00, 0x01, 0x02}), ErrorResult); EXPECT_THAT_EXPECTED( @@ -127,13 +131,15 @@ TEST(DWARFDataExtractorTest, getInitialLength) { // DWARF64 marker without the subsequent length field. EXPECT_THAT_EXPECTED( GetWithError({0xff, 0xff, 0xff, 0xff}), - FailedWithMessage("unexpected end of data at offset 0x4")); + FailedWithMessage( + "unexpected end of data at offset 0x4 while reading [0x4, 0xc)")); EXPECT_EQ(GetWithoutError({0xff, 0xff, 0xff, 0xff}), ErrorResult); // Not enough data for the U64 length. EXPECT_THAT_EXPECTED( GetWithError({0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0x02, 0x03}), - FailedWithMessage("unexpected end of data at offset 0x4")); + FailedWithMessage( + "unexpected end of data at offset 0x8 while reading [0x4, 0xc)")); EXPECT_EQ(GetWithoutError({0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0x02, 0x03}), ErrorResult); @@ -195,21 +201,27 @@ Symbols: EXPECT_EQ(0x64636261u, Truncated8.getRelocatedAddress(C)); EXPECT_EQ(0x42u, Truncated8.getRelocatedAddress(C)); EXPECT_EQ(0x0u, Truncated8.getRelocatedAddress(C)); - EXPECT_THAT_ERROR(C.takeError(), - FailedWithMessage("unexpected end of data at offset 0x8")); + EXPECT_THAT_ERROR( + C.takeError(), + FailedWithMessage( + "unexpected end of data at offset 0x8 while reading [0x8, 0xc)")); C = DataExtractor::Cursor{0}; DWARFDataExtractor Truncated6(Data, 6); EXPECT_EQ(0x64636261u, Truncated6.getRelocatedAddress(C)); EXPECT_EQ(0x0u, Truncated6.getRelocatedAddress(C)); - EXPECT_THAT_ERROR(C.takeError(), - FailedWithMessage("unexpected end of data at offset 0x4")); + EXPECT_THAT_ERROR( + C.takeError(), + FailedWithMessage( + "unexpected end of data at offset 0x6 while reading [0x4, 0x8)")); C = DataExtractor::Cursor{0}; DWARFDataExtractor Truncated2(Data, 2); EXPECT_EQ(0x0u, Truncated2.getRelocatedAddress(C)); - EXPECT_THAT_ERROR(C.takeError(), - FailedWithMessage("unexpected end of data at offset 0x0")); + EXPECT_THAT_ERROR( + C.takeError(), + FailedWithMessage( + "unexpected end of data at offset 0x2 while reading [0x0, 0x4)")); } } // namespace diff --git a/unittests/DebugInfo/DWARF/DWARFDebugArangeSetTest.cpp b/unittests/DebugInfo/DWARF/DWARFDebugArangeSetTest.cpp index 5629a0d071c..4ec9c5d1c0b 100644 --- a/unittests/DebugInfo/DWARF/DWARFDebugArangeSetTest.cpp +++ b/unittests/DebugInfo/DWARF/DWARFDebugArangeSetTest.cpp @@ -121,7 +121,7 @@ TEST(DWARFDebugArangeSet, SectionTooShort) { static const char DebugArangesSecRaw[11 + 1] = {0}; ExpectExtractError(DebugArangesSecRaw, "parsing address ranges table at offset 0x0: unexpected " - "end of data at offset 0xb"); + "end of data at offset 0xb while reading [0xb, 0xc)"); } TEST(DWARFDebugArangeSet, SectionTooShortDWARF64) { @@ -130,7 +130,7 @@ TEST(DWARFDebugArangeSet, SectionTooShortDWARF64) { "\xff\xff\xff\xff"; // DWARF64 mark ExpectExtractError(DebugArangesSecRaw, "parsing address ranges table at offset 0x0: unexpected " - "end of data at offset 0x17"); + "end of data at offset 0x17 while reading [0x17, 0x18)"); } TEST(DWARFDebugArangeSet, NoSpaceForEntries) { diff --git a/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp b/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp index be61c03b779..7ceb6561dc7 100644 --- a/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp +++ b/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp @@ -211,8 +211,9 @@ TEST_F(DebugLineBasicFixture, GetOrParseLineTableAtInvalidOffsetAfterData) { EXPECT_THAT_EXPECTED( getOrParseLineTableFatalErrors(0), - FailedWithMessage("parsing line table prologue at offset 0x00000000: " - "unexpected end of data at offset 0x0")); + FailedWithMessage( + "parsing line table prologue at offset 0x00000000: " + "unexpected end of data at offset 0x1 while reading [0x0, 0x4)")); EXPECT_THAT_EXPECTED( getOrParseLineTableFatalErrors(1), diff --git a/unittests/DebugInfo/DWARF/DWARFDieTest.cpp b/unittests/DebugInfo/DWARF/DWARFDieTest.cpp index b662965a482..ad00537c2a4 100644 --- a/unittests/DebugInfo/DWARF/DWARFDieTest.cpp +++ b/unittests/DebugInfo/DWARF/DWARFDieTest.cpp @@ -107,7 +107,8 @@ TEST(DWARFDie, getLocations) { EXPECT_THAT_EXPECTED( Die.getLocations(DW_AT_call_data_location), - FailedWithMessage("unexpected end of data at offset 0x20")); + FailedWithMessage( + "unexpected end of data at offset 0x20 while reading [0x20, 0x21)")); EXPECT_THAT_EXPECTED( Die.getLocations(DW_AT_call_data_value), diff --git a/unittests/Support/DataExtractorTest.cpp b/unittests/Support/DataExtractorTest.cpp index cec08554e8f..278e5885916 100644 --- a/unittests/Support/DataExtractorTest.cpp +++ b/unittests/Support/DataExtractorTest.cpp @@ -102,8 +102,9 @@ TEST(DataExtractorTest, Strings) { EXPECT_EQ(11U, C.tell()); EXPECT_EQ(nullptr, DE.getCStr(C)); EXPECT_EQ(11U, C.tell()); - EXPECT_THAT_ERROR(C.takeError(), - FailedWithMessage("unexpected end of data at offset 0xb")); + EXPECT_THAT_ERROR( + C.takeError(), + FailedWithMessage("no null terminated string at offset 0xb")); } TEST(DataExtractorTest, LEB128) { @@ -270,6 +271,12 @@ TEST(DataExtractorTest, getU8_vector) { DE.getU8(C, S, 2); EXPECT_THAT_ERROR(C.takeError(), Succeeded()); EXPECT_EQ("AB", toStringRef(S)); + + C = DataExtractor::Cursor(0x47); + DE.getU8(C, S, 2); + EXPECT_THAT_ERROR( + C.takeError(), + FailedWithMessage("offset 0x47 is beyond the end of data at 0x2")); } TEST(DataExtractorTest, getU24) {