[llvm-readobj] - Improve dumping of the SHT_LLVM_LINKER_OPTIONS sections.

[grimar]

Summary

I've added a few tests that shows how the current code could overrun the section data
buffer while dumping. I had to rewrite the code to fix this.
Main problem was that the current code assumed that the section data is correct.
But when it is not - it failed.

[grimar]

• Added a test for "empty section" case.

[jhenderson]

llvm/test/tools/llvm-readobj/elf-linker-options.test

61

attemp -> attempt
a data -> data
outside of -> outside

68

Should we make these INCOMPLETE-NEXT to show that nothing is printed for the entry? In that case, what do you think about adding a valid pair before to show that earlier valid entries are (not) printed?

85

Same comments for this case as above (both comments and check).

llvm/tools/llvm-readobj/ELFDumper.cpp

6042

found, the -> found. The

[MaskRay]

llvm/tools/llvm-readobj/ELFDumper.cpp

6025

Should this be continue?

[compnerd]

llvm/tools/llvm-readobj/ELFDumper.cpp

6022

Contents is the correct word for this.

6031

This should probably be a continue.

[grimar]

Thanks for all the comments. I am going to be off until the next week, so will update this patch after 18th of november.

[grimar]

• Addressed review comments and reimplemented.

llvm/test/tools/llvm-readobj/elf-linker-options.test

68

Yeah, I had to significantly change the structure here.

Using returns in the code was not correct I think,
then, if we want to continue dumping sections after reporting a warning, it is probably worth
to show how we dump the possible mixed cases too. So I've combined the valid/invalid cases and improved the error messages displayed.

llvm/tools/llvm-readobj/ELFDumper.cpp

6025

Right. I've changed all the logic here to continue dumping sections even after reporting an error.

6031

Yes.

[jhenderson]

LGTM with two nits.

llvm/test/tools/llvm-readobj/elf-linker-options.test

11

'"c' -> "c"?

49

a one more -> another

[grimar]

@MaskRay, @compnerd, do you have more comments?

[MaskRay]

LG once @jhenderson's comments are addressed.

llvm/test/tools/llvm-readobj/elf-linker-options.test

 ## Check that we can use the --elf-linker-options option
 ## to dump SHT_LLVM_LINKER_OPTIONS sections.
 
-# RUN: yaml2obj %s -o %t1
-# RUN: llvm-readobj --elf-linker-options %t1 | FileCheck %s
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj --elf-linker-options %t1 2>&1 | FileCheck %s --check-prefix=CHECK -DFILE=%t1
 
-# CHECK:      LinkerOptions [
-# CHECK-NEXT:  option 0: value 0
-# CHECK-NEXT:  option 1: value 1
-# CHECK-NEXT: ]
+# CHECK:       LinkerOptions [
+# CHECK:         option 0: value 0
+# CHECK:         option 1: value 1
+# CHECK-EMPTY:
+# CHECK-NEXT:    warning: '[[FILE]]': SHT_LLVM_LINKER_OPTIONS section at index 2 is broken: an incomplete key-value pair was found. The last possible key was: "c
+# CHECK-EMPTY:
+# CHECK-NEXT:    warning: '[[FILE]]': SHT_LLVM_LINKER_OPTIONS section at index 4 is broken: the content is not null-terminated
+# CHECK-NEXT:    option 3: value 3
+# CHECK-NEXT:  ]
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
-  - Name: .linker-options
+## Case 1: a correct case.
+  - Name: .linker-options.valid1
     Type: SHT_LLVM_LINKER_OPTIONS
     Options:
       - Name:  option 0
         Value: value 0
       - Name:  option 1
         Value: value 1
+## Case 2: check we do not attempt to dump data from outside the SHT_LLVM_LINKER_OPTIONS section
+##         when it contains an incomplete key-value pair.
+  - Name:    .linker-options.incomplete
+    Type:    SHT_LLVM_LINKER_OPTIONS
+    Content: "610062006300" ## 'a', '\0', 'b', '\0', 'c', '\0'
+  - Type:    Fill
+    Pattern: "FF"
+    Size:    "1"
+## Case 3: in case of an empty section we dump nothing.
+  - Name:    .linker-options.empty
+    Type:    SHT_LLVM_LINKER_OPTIONS
+    Content: ""
+## Case 4: check we do not attempt to dump data from outside the SHT_LLVM_LINKER_OPTIONS section
+##         when it is not null-terminated.
+  - Name:    .linker-options.nonul
+    Type:    SHT_LLVM_LINKER_OPTIONS
+    Content: "61"
+## Case 5: a one more correct case to show we do not stop dumping after reporting a warning.
+  - Name: .linker-options.valid2
+    Type: SHT_LLVM_LINKER_OPTIONS
+    Options:
+      - Name:  option 3
+        Value: value 3
 
 ## llvm-readelf doesn't support --elf-linker-options yet.
 # RUN: llvm-readelf --elf-linker-options %t1 2>&1 | FileCheck %s --check-prefix=READELF
 
 # READELF: printELFLinkerOptions not implemented!

llvm/tools/llvm-readobj/ELFDumper.cpp

if (Obj->getHeader()->e_type != ELF::ET_CORE && !Sections.empty()) {
     }
   }
 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printELFLinkerOptions(const ELFFile<ELFT> *Obj) {
   ListScope L(W, "LinkerOptions");
 
+  unsigned I = -1;
   for (const Elf_Shdr &Shdr : unwrapOrError(this->FileName, Obj->sections())) {
+    ++I;
     if (Shdr.sh_type != ELF::SHT_LLVM_LINKER_OPTIONS)
       continue;
 
     ArrayRef<uint8_t> Contents =
         unwrapOrError(this->FileName, Obj->getSectionContents(&Shdr));
-    for (const uint8_t *P = Contents.begin(), *E = Contents.end(); P < E; ) {
-      StringRef Key = StringRef(reinterpret_cast<const char *>(P));
-      StringRef Value =
-          StringRef(reinterpret_cast<const char *>(P) + Key.size() + 1);
+    if (Contents.empty())
+      continue;
 
-      W.printString(Key, Value);
+    if (Contents.back() != 0) {
+      reportWarning(createError("SHT_LLVM_LINKER_OPTIONS section at index " +
+                                Twine(I) +
+                                " is broken: the "
+                                "content is not null-terminated"),
+                    this->FileName);
+      continue;
+    }
 
-      P = P + Key.size() + Value.size() + 2;
+    SmallVector<StringRef, 16> Strings;
+    toStringRef(Contents.drop_back()).split(Strings, '\0');
+    if (Strings.size() % 2 != 0) {
+      reportWarning(
+          createError(
+              "SHT_LLVM_LINKER_OPTIONS section at index " + Twine(I) +
+              " is broken: an incomplete "
+              "key-value pair was found. The last possible key was: \"" +
+              Strings.back() + "\""),
+          this->FileName);
+      continue;
     }
+
+    for (size_t I = 0; I < Strings.size(); I += 2)
+      W.printString(Strings[I], Strings[I + 1]);
   }
 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printStackSizes(const ELFObjectFile<ELFT> *Obj) {
   ListScope L(W, "StackSizes");
   if (Obj->isRelocatableObject())
     this->printRelocatableStackSizes(Obj, []() {});