[llvm-readobj] - Fix crashes and misbehaviors when reading strings from broken string tables.

[grimar]

Summary

There are cases when we either might print garbage or crash when
reading strings for dumping dynamic tags.

For example when a string table is not null-terminated or goes past the EOF.
This patch fixes issues mentioned.

[jhenderson]

Hmmm... my first instinct here is that we should print a single warning out-of-line, and then use the "<?>" pattern in place of the invalid strings. That probably goes for the existing messages too. What do you think about that idea?

[grimar]

In D77216#1954873, @jhenderson wrote:

Hmmm... my first instinct here is that we should print a single warning out-of-line, and then use the "<?>" pattern in place of the invalid strings. That probably goes for the existing messages too. What do you think about that idea?

Yeah, I've though about that we might want to change the code to this behavior as it would be more consistent with all kinds of other errors we have.
I'll prepare a patch to change the existent behavior first then.

[grimar]

• Updated after recent changes done by D77399.

[MaskRay]

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

302

Not terminated by a NUL.

or

Not null terminated

330

This can be omitted

[grimar]

• Addressed review comments.

[MaskRay]

Thanks! Please wait a bit for @jhenderson's opinions (I think today is a public holiday (Easter Monday))

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

255

--implicit-check-not=warning: is probably better to avoid weird failures with path names involving "warning" (there's bound to be somebody out there...).

Same below.

259

I might be inclined to move these checks to below all the RUN lines for the different cases, as the use of NOT-TERMINATED-LESS is a little confusing when nothing up to that point in the test file had actually speciifed it.

350

strings are dumped because the file ends with the zero byte

What guarantees this? Should we add something to either a) validate it (to make it easier to identify the problem if it doesn't end with a zero byte) or b) force it to always be zero?

351

Delete one of the "as a".

llvm/tools/llvm-readobj/ELFDumper.cpp

2565–2567

I think you can make the message slightly more concise:

"string table at offset 0x12345678 with size 0x87654321 goes past the end of the file (0x22222222)"

2574

Unrelated I know, but this should be ':' not ',' for more correct grammar.

2581

I'd change ',' to ':' here.

[grimar]

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

350

But should we care/do something more? The intention of this test is to document the behavior when "the value of DT_STRSZ tag is set so that the string table ends right before the EOF". In this case dumping of strings is a side effect, so I've put a "Note". If the file had no zero byte at the end, we would dump "<?>", but what we really want to check is that we do not crash here and in the "Case B" below.

[grimar]

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

341

I guess this is misleading. What about I change this to:

"Document the behaviort when the dynamic string table ends past the end of the file. Check we do not crash."

[grimar]

• Addressed review comments.

llvm/tools/llvm-readobj/ELFDumper.cpp

2574

Fixed (to be consistent with new messages around).

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

341

Maybe this could be "Check that we emit an appropriate warning when the dynamic string table ends past the end of the file."

343

Add "No warning should be emitted." here, I think.

350–351

I think if we rewrite this note slightly, we could clarify more that it's not important what the final byte is:

"Note: The code reads the data in [DT_STRTAB, DT_STRTAB + DT_STRSZ] as the string table as normal. Since the file ends with a zero byte, strings are dumped, but if it didn't, we'd get <?> printed instead. The important bit is that we don't get the past the end warning."

[grimar]

• Addressed review comments.

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

350–351

Reads better, thanks.

[jhenderson]

LGTM.

llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test

ProgramHeaders:
   - Type: PT_LOAD
     VAddr: 0x1000
     Sections:
       - Section: .dynamic
   - Type: PT_DYNAMIC
     VAddr: 0x1000
     Sections:
       - Section: .dynamic
+
+## Check how we handle cases when the dynamic string table is not null-terminated.
+## Check that we are still trying to dump strings and that we do not dump bytes past the
+## string table bounds specified by the DT_STRSZ tag.
+
+## Case A: the value of the DT_STRSZ tag is equal to the size of
+##         the not null-terminated dynamic string table.
+# RUN: yaml2obj %s -DSTRSZ=7 --docnum=6 -o %t6
+# RUN: llvm-readobj --dynamic-table %t6 | FileCheck %s --check-prefix=NOT-TERMINATED -DSTR=test.so
+# RUN: llvm-readelf --dynamic-table %t6 | FileCheck %s --check-prefix=NOT-TERMINATED -DSTR=test.so
+
+# NOT-TERMINATED:      {{[(]?}}NEEDED{{[)]?}}    Shared library: [<String table at offset 0xb0 is not null-terminated: [[STR]]>]
+# NOT-TERMINATED-NEXT: {{[(]?}}FILTER{{[)]?}}    Filter library: [<String table at offset 0xb0 is not null-terminated: [[STR]]>]
+# NOT-TERMINATED-NEXT: {{[(]?}}AUXILIARY{{[)]?}} Auxiliary library: [<String table at offset 0xb0 is not null-terminated: [[STR]]>]
+# NOT-TERMINATED-NEXT: {{[(]?}}USED{{[)]?}}      Not needed object: [<String table at offset 0xb0 is not null-terminated: [[STR]]>]
+# NOT-TERMINATED-NEXT: {{[(]?}}SONAME{{[)]?}}    Library soname: [<String table at offset 0xb0 is not null-terminated: [[STR]]>]
+# NOT-TERMINATED-NEXT: {{[(]?}}RPATH{{[)]?}}     Library rpath: [<String table at offset 0xb0 is not null-terminated: [[STR]]>]
+# NOT-TERMINATED-NEXT: {{[(]?}}RUNPATH{{[)]?}}   Library runpath: [<String table at offset 0xb0 is not null-terminated: [[STR]]>]
+# NOT-TERMINATED-NEXT: {{[(]?}}NULL{{[)]?}}      0x0
+
+## Case B: the value of the DT_STRSZ tag is less than the size of
+##         the not null-terminated dynamic string table.
+# RUN: yaml2obj %s -DSTRSZ=3 --docnum=6 -o %t7
+# RUN: llvm-readobj --dynamic-table %t7 | FileCheck %s --check-prefix=NOT-TERMINATED -DSTR=tes
+# RUN: llvm-readelf --dynamic-table %t7 | FileCheck %s --check-prefix=NOT-TERMINATED -DSTR=tes
+
+## Case C: the value of the DT_STRSZ tag is one byte larger than the size of
+##         the not null-terminated dynamic string table.
+# RUN: yaml2obj %s -DSTRSZ=8 --docnum=6 -o %t8
+# RUN: llvm-readobj --dynamic-table %t8 | FileCheck %s --check-prefix=NOT-TERMINATED -DSTR=test.soa
+# RUN: llvm-readelf --dynamic-table %t8 | FileCheck %s --check-prefix=NOT-TERMINATED -DSTR=test.soa
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    .dynstr
+    Type:    SHT_STRTAB
+    Address: 0x1000
+    Content: '746573742e736f' ## "test.so", there is no expected null symbol at the end.
+  - Type:    Fill
+    Pattern: "61626300" ## 'a', 'b', 'c', '\0'.
+    Size:    "4"
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Address: 0x1100
+    Entries:
+      - Tag:   DT_STRTAB
+        Value: 0x1000
+      - Tag:   DT_STRSZ
+        Value: [[STRSZ]]
+      - Tag:   DT_NEEDED
+        Value: 0
+      - Tag:   DT_FILTER
+        Value: 0
+      - Tag:   DT_AUXILIARY
+        Value: 0
+      - Tag:   DT_USED
+        Value: 0
+      - Tag:   DT_SONAME
+        Value: 0
+      - Tag:   DT_RPATH
+        Value: 0
+      - Tag:   DT_RUNPATH
+        Value: 0
+      - Tag:   DT_NULL
+        Value: 0
+Symbols: []
+ProgramHeaders:
+  - Type: PT_LOAD
+    VAddr: 0x1000
+    Sections:
+      - Section: .dynstr
+      - Section: .dynamic
+  - Type: PT_DYNAMIC
+    VAddr: 0x1100
+    Sections:
+      - Section: .dynamic
+
+## Check what we print when the dynamic string table ends past the end of the file.
+
+## Case A: the value of DT_STRSZ tag is set so that the string table ends right before the EOF.
+# RUN: yaml2obj %s -DSTRSZ=0x278 --docnum=6 -o %t9.1
+# RUN: llvm-readobj --dynamic-table %t9.1 | FileCheck %s --check-prefix=BEFORE-THE-EOF
+# RUN: llvm-readelf --dynamic-table %t9.1 | FileCheck %s --check-prefix=BEFORE-THE-EOF
+
+# BEFORE-THE-EOF:      {{[(]?}}NEEDED{{[)]?}}    Shared library: [test.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}FILTER{{[)]?}}    Filter library: [test.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}AUXILIARY{{[)]?}} Auxiliary library: [test.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}USED{{[)]?}}      Not needed object: [test.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}SONAME{{[)]?}}    Library soname: [test.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}RPATH{{[)]?}}     Library rpath: [test.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}RUNPATH{{[)]?}}   Library runpath: [test.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}NULL{{[)]?}}      0x0
+
+# RUN: yaml2obj %s -DSTRSZ=0x279 --docnum=6 -o %t9.2
+# RUN: llvm-readobj --dynamic-table %t9.2 | FileCheck %s --check-prefix=PAST-THE-EOF
+# RUN: llvm-readelf --dynamic-table %t9.2 | FileCheck %s --check-prefix=PAST-THE-EOF
+
+## Case B: the value of DT_STRSZ tag is set so that the string table goes 1 byte past the EOF.
+# PAST-THE-EOF:      {{[(]?}}NEEDED{{[)]?}}    Shared library: [<String table at offset 0xb0 has size 0x279 and goes past the end of the file (0x328)>]
+# PAST-THE-EOF-NEXT: {{[(]?}}FILTER{{[)]?}}    Filter library: [<String table at offset 0xb0 has size 0x279 and goes past the end of the file (0x328)>]
+# PAST-THE-EOF-NEXT: {{[(]?}}AUXILIARY{{[)]?}} Auxiliary library: [<String table at offset 0xb0 has size 0x279 and goes past the end of the file (0x328)>]
+# PAST-THE-EOF-NEXT: {{[(]?}}USED{{[)]?}}      Not needed object: [<String table at offset 0xb0 has size 0x279 and goes past the end of the file (0x328)>]
+# PAST-THE-EOF-NEXT: {{[(]?}}SONAME{{[)]?}}    Library soname: [<String table at offset 0xb0 has size 0x279 and goes past the end of the file (0x328)>]
+# PAST-THE-EOF-NEXT: {{[(]?}}RPATH{{[)]?}}     Library rpath: [<String table at offset 0xb0 has size 0x279 and goes past the end of the file (0x328)>]
+# PAST-THE-EOF-NEXT: {{[(]?}}RUNPATH{{[)]?}}   Library runpath: [<String table at offset 0xb0 has size 0x279 and goes past the end of the file (0x328)>]
+# PAST-THE-EOF-NEXT: {{[(]?}}NULL{{[)]?}}      0x0

llvm/test/tools/llvm-readobj/ELF/loadname.test

 ## Check we are able to dump library soname properly.
 
 ## Test a valid object case first. We set 'FileSize' to 0x48, because this is a no-op,
 ## i.e. this value would be set if we had no 'FileSize' at all.
 # RUN: yaml2obj -DDTSTRTABVAL=0x0 -DPHDRFILESIZE="0x48" %s -o %t.o
 # RUN: llvm-readobj %t.o | FileCheck %s --check-prefix LLVM
 # RUN: llvm-readelf --dynamic-table %t.o | FileCheck %s --check-prefix GNU
 
 # LLVM:      Format: elf64-x86-64
 # LLVM-NEXT: Arch: x86_64
 # LLVM-NEXT: AddressSize: 64bit
 # LLVM-NEXT: LoadName: test.so
 
 # GNU:       Dynamic section at offset 0x80 contains 4 entries:
 # GNU-NEXT:   Tag                Type     Name/Value
 # GNU-NEXT:   0x0000000000000005 (STRTAB) 0x0
-# GNU-NEXT:   0x000000000000000a (STRSZ)  7 (bytes)
+# GNU-NEXT:   0x000000000000000a (STRSZ)  8 (bytes)
 # GNU-NEXT:   0x000000000000000e (SONAME) Library soname: [test.so]
 # GNU-NEXT:   0x0000000000000000 (NULL)   0x0
 
 !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
   - Name:  .dynstr
     Type:  SHT_STRTAB
     Flags: [ SHF_ALLOC ]
     Content: '746573742e736f00' ## "test.so"
   - Name:  .dynamic
     Type:  SHT_DYNAMIC
     Flags: [ SHF_ALLOC ]
     Link:  .dynstr
     Entries:
       - Tag:   DT_STRTAB
         Value: [[DTSTRTABVAL]]
       - Tag:   DT_STRSZ
-        Value: 0x0000000000000007
+        Value: 0x0000000000000008
       - Tag:   DT_SONAME
         Value: 0x0000000000000000
       - Tag:   DT_NULL
         Value: 0x0000000000000000
 ProgramHeaders:
   - Type:     PT_LOAD
     Flags:    [ PF_R ]
     VAddr:    0x0000

llvm/tools/llvm-readobj/ELFDumper.cpp

default:
     break;
   }
 }
 
 template <class ELFT>
 std::string ELFDumper<ELFT>::getDynamicString(uint64_t Value) const {
   if (DynamicStringTable.empty())
     return "<String table is empty or was not found>";
-  if (Value < DynamicStringTable.size())
-    return DynamicStringTable.data() + Value;
+  if (Value >= DynamicStringTable.size())
     return Twine("<Invalid offset 0x" + utohexstr(Value) + ">").str();
+
+  const uint64_t FileSize = ObjF->getELFFile()->getBufSize();
+  const uint64_t TableOffset =
+      (const uint8_t *)DynamicStringTable.data() - ObjF->getELFFile()->base();
+  if (DynamicStringTable.size() > FileSize - TableOffset)
+    return ("<String table at offset 0x" + Twine::utohexstr(TableOffset) +
+            " has size 0x" + Twine::utohexstr(DynamicStringTable.size()) +
+            " and goes past the end of the file (0x" +
+            Twine::utohexstr(FileSize) + ")>")
+        .str();
+
+  if (DynamicStringTable.back() != '\0')
+    return ("<String table at offset 0x" + Twine::utohexstr(TableOffset) +
+            " is not null-terminated: " +
+            StringRef(DynamicStringTable.data() + Value,
+                      DynamicStringTable.size() - Value) +
+            ">")
+        .str();
+
+  return DynamicStringTable.data() + Value;
 }
 
 template <class ELFT> void ELFDumper<ELFT>::printUnwindInfo() {
   DwarfCFIEH::PrinterContext<ELFT> Ctx(W, ObjF);
   Ctx.printUnwindInformation();
 }
 
 namespace {