[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

336

Not terminated by a NUL.

or

Not null terminated

364

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

289

--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.

293

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.

384

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?

385

Delete one of the "as a".

llvm/tools/llvm-readobj/ELFDumper.cpp

2576–2578

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)"

2585

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

2592

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

[grimar]

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

384

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

375

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

2585

Fixed (to be consistent with new messages around).

[jhenderson]

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

375

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

377

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

384–385

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

384–385

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.
+
+## 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 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t6 --implicit-check-not=warning --check-prefixes=NOT-TERMINATED,NOT-TERMINATED-GREQ
+# RUN: llvm-readelf --dynamic-table %t6 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t6 --implicit-check-not=warning --check-prefixes=NOT-TERMINATED,NOT-TERMINATED-GREQ
+
+# NOT-TERMINATED:      warning: '[[FILE]]': string table at offset 0xb0: unable to read the string at 0xb4, the string table is not null-terminated
+# NOT-TERMINATED:      warning: '[[FILE]]': string table at offset 0xb0: unable to read the string at 0xb0, the string table is not null-terminated
+# NOT-TERMINATED-NEXT: {{[(]?}}NEEDED{{[)]?}}    Shared library: [<?>]
+# NOT-TERMINATED-NEXT: warning: '[[FILE]]': string table at offset 0xb0: unable to read the string at 0xb1, the string table is not null-terminated
+# NOT-TERMINATED-NEXT: {{[(]?}}FILTER{{[)]?}}    Filter library: [<?>]
+# NOT-TERMINATED-NEXT: warning: '[[FILE]]': string table at offset 0xb0: unable to read the string at 0xb2, the string table is not null-terminated
+# NOT-TERMINATED-NEXT: {{[(]?}}AUXILIARY{{[)]?}} Auxiliary library: [<?>]
+# NOT-TERMINATED-NEXT: warning: '[[FILE]]': string table at offset 0xb0: unable to read the string at 0xb3, the string table is not null-terminated
+# NOT-TERMINATED-NEXT: {{[(]?}}USED{{[)]?}}      Not needed object: [<?>]
+# NOT-TERMINATED-NEXT: {{[(]?}}SONAME{{[)]?}}    Library soname: [<?>]
+# NOT-TERMINATED-NEXT: warning: '[[FILE]]': string table at offset 0xb0: unable to read the string at 0xb5, the string table is not null-terminated
+# NOT-TERMINATED-NEXT: {{[(]?}}RPATH{{[)]?}}     Library rpath: [<?>]
+# NOT-TERMINATED-GREQ-NEXT: warning: '[[FILE]]': string table at offset 0xb0: unable to read the string at 0xb6, the string table is not null-terminated
+# NOT-TERMINATED-LESS-NEXT: warning: '[[FILE]]': string table at offset 0xb0: unable to read the string at 0xb6, it goes past the end of the table (0xb6)
+# NOT-TERMINATED-NEXT: {{[(]?}}RUNPATH{{[)]?}}   Library runpath: [<?>]
+# 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=6 --docnum=6 -o %t7
+# RUN: llvm-readobj --dynamic-table %t7 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t7 --implicit-check-not=warning --check-prefixes=NOT-TERMINATED,NOT-TERMINATED-LESS
+# RUN: llvm-readelf --dynamic-table %t7 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t7 --implicit-check-not=warning --check-prefixes=NOT-TERMINATED,NOT-TERMINATED-LESS
+
+## 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 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t8 --implicit-check-not=warning --check-prefixes=NOT-TERMINATED,NOT-TERMINATED-GREQ
+# RUN: llvm-readelf --dynamic-table %t8 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t8 --implicit-check-not=warning --check-prefixes=NOT-TERMINATED,NOT-TERMINATED-GREQ
+
+--- !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: 1
+      - Tag:   DT_AUXILIARY
+        Value: 2
+      - Tag:   DT_USED
+        Value: 3
+      - Tag:   DT_SONAME
+        Value: 4
+      - Tag:   DT_RPATH
+        Value: 5
+      - Tag:   DT_RUNPATH
+        Value: 6
+      - 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 | \
+# RUN:   FileCheck %s --implicit-check-not=warning --check-prefix=BEFORE-THE-EOF1
+# RUN: llvm-readelf --dynamic-table %t9.1 | \
+# RUN:   FileCheck %s --implicit-check-not=warning --check-prefix=BEFORE-THE-EOF
+
+## Note: strings are dumped because the file ends with the zero byte. Code reads the data
+## in [DT_STRTAB, DT_STRTAB + DT_STRSZ] range as a as a normal null-terminated string table.
+# BEFORE-THE-EOF:      {{[(]?}}NEEDED{{[)]?}}    Shared library: [test.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}FILTER{{[)]?}}    Filter library: [est.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}AUXILIARY{{[)]?}} Auxiliary library: [st.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}USED{{[)]?}}      Not needed object: [t.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}SONAME{{[)]?}}    Library soname: [.soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}RPATH{{[)]?}}     Library rpath: [soabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}RUNPATH{{[)]?}}   Library runpath: [oabc]
+# BEFORE-THE-EOF-NEXT: {{[(]?}}NULL{{[)]?}}      0x0
+
+## Case B: the value of DT_STRSZ tag is set so that the string table goes 1 byte past the EOF.
+# RUN: yaml2obj %s -DSTRSZ=0x279 --docnum=6 -o %t9.2
+# RUN: llvm-readobj --dynamic-table %t9.2 2>&1 | FileCheck %s -DFILE=%t9.2 --check-prefix=PAST-THE-EOF
+# RUN: llvm-readelf --dynamic-table %t9.2 2>&1 | FileCheck %s -DFILE=%t9.2 --check-prefix=PAST-THE-EOF
+
+# PAST-THE-EOF:      warning: '[[FILE]]': string table at offset 0xb0 has size (0x279) that goes past the end of the file (0x328)
+# PAST-THE-EOF:      {{[(]?}}NEEDED{{[)]?}}    Shared library: [<?>]
+# PAST-THE-EOF-NEXT: {{[(]?}}FILTER{{[)]?}}    Filter library: [<?>]
+# PAST-THE-EOF-NEXT: {{[(]?}}AUXILIARY{{[)]?}} Auxiliary library: [<?>]
+# PAST-THE-EOF-NEXT: {{[(]?}}USED{{[)]?}}      Not needed object: [<?>]
+# PAST-THE-EOF-NEXT: {{[(]?}}SONAME{{[)]?}}    Library soname: [<?>]
+# PAST-THE-EOF-NEXT: {{[(]?}}RPATH{{[)]?}}     Library rpath: [<?>]
+# PAST-THE-EOF-NEXT: {{[(]?}}RUNPATH{{[)]?}}   Library runpath: [<?>]
+# 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

case DT_FLAGS_1:
     return FormatFlags(Value, makeArrayRef(ElfDynamicDTFlags1));
   default:
     return FormatHexValue(Value);
   }
 }
 
 template <class ELFT>
 StringRef ELFDumper<ELFT>::getDynamicString(uint64_t Value) const {
-  auto WarnAndReturn = [this](const Twine &Msg) {
-    reportUniqueWarning(createError(Msg));
+  if (DynamicStringTable.empty() && !DynamicStringTable.data()) {
+    reportUniqueWarning(createError("string table was not found"));
     return "<?>";
-  };
-
-  if (DynamicStringTable.empty() && !DynamicStringTable.data())
-    return WarnAndReturn("string table was not found");
+  }
 
-  if (Value < DynamicStringTable.size())
-    return DynamicStringTable.data() + Value;
+  auto WarnAndReturn = [this](const Twine &Msg, uint64_t Offset) {
+    reportUniqueWarning(createError("string table at offset 0x" +
+                                    Twine::utohexstr(Offset) + Msg));
+    return "<?>";
+  };
 
+  const uint64_t FileSize = ObjF->getELFFile()->getBufSize();
   const uint64_t Offset =
       (const uint8_t *)DynamicStringTable.data() - ObjF->getELFFile()->base();
+  if (DynamicStringTable.size() > FileSize - Offset)
+    return WarnAndReturn(" has size (0x" +
+                             Twine::utohexstr(DynamicStringTable.size()) +
+                             ") that goes past the end of the file (0x" +
+                             Twine::utohexstr(FileSize) + ")",
+                         Offset);
+
+  if (Value >= DynamicStringTable.size())
     return WarnAndReturn(
-      "string table at offset 0x" + Twine::utohexstr(Offset) +
         ": unable to read the string at 0x" + Twine::utohexstr(Offset + Value) +
             ", it goes past the end of the table (0x" +
-      Twine::utohexstr(Offset + DynamicStringTable.size()) + ")");
+            Twine::utohexstr(Offset + DynamicStringTable.size()) + ")",
+        Offset);
+
+  if (DynamicStringTable.back() != '\0')
+    return WarnAndReturn(": unable to read the string at 0x" +
+                             Twine::utohexstr(Offset + Value) +
+                             ", the string table is not null-terminated",
+                         Offset);
+
+  return DynamicStringTable.data() + Value;
 }
 
 template <class ELFT> void ELFDumper<ELFT>::printUnwindInfo() {
   DwarfCFIEH::PrinterContext<ELFT> Ctx(W, ObjF);
   Ctx.printUnwindInformation();
 }
 
 namespace {