[llvm-readelf/obj] - Fix the possible crash when dumping group sections.

[grimar]

Summary

It is possible to trigger a crash/misbehavior when the st_name field of
the signature symbol goes past the end of the string table.

This patch fixes it.

[jhenderson]

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

390

I'm confused. Why do we need to do this, rather than just specifying an arbitrarily large StName value greater than the string table size?

llvm/tools/llvm-readobj/ELFDumper.cpp

3613

Perhaps this makes a bit more sense, since st_name is a value/pointer, rather than a range, so it doesn't "go" anywhere.

[grimar]

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

390

I am testing 2 cases here:

1. (OK) When the StName == [size of string table - 1]
2. (FAIL) When StName == size of string table

I think we should know its size for this test and test values on border.
It can be done easier when the content of the string table is known.

Also in the test below I am changing the content to remove the zero terminator, keeping st_name values the same.
This proves that we can never read past the end of the string table and that way we can't have a situation of reading
past the EOF.

[jhenderson]

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

390

Okay, perhaps worth adding to the commetn that setting the string table contents explicitly makes it easier to show the boundary conditions, or something like effect.

[grimar]

• Addressed review comments.

[jhenderson]

LGTM, with nit.

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

391

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

- Name:   .text.bar
     ShName: [[TEXTBARSHNAME=<none>]]
   - Name: .rela.text.bar
     Type: SHT_RELA
     Link: .symtab
     Info: .text.bar
   - Name: .symtab
     Type: SHT_SYMTAB
     Link: [[SYMTABLINK=.strtab]]
+  - Name:    .strtab
+    Type:    SHT_STRTAB
+    Content: [[STRTABCONTENT=<none>]]
 Symbols:
   - Name:    foo
     Section: .text.foo
+    StName:  [[SYM1STNAME=<none>]]
   - Name:    bar
     Section: .text.bar
+    StName:  [[SYM2STNAME=<none>]]
 
 ## Check that we report a warning and continue dumping when a section is included
 ## in two group sections at the same time.
 
 # RUN: yaml2obj %s -DMEMBER2=.text.foo -o %t.dup.o
 # RUN: llvm-readobj --elf-section-groups %t.dup.o 2>&1 | FileCheck %s -DFILE=%t.dup.o --check-prefix=DUP-LLVM
 # RUN: llvm-readelf --elf-section-groups %t.dup.o 2>&1 | FileCheck %s -DFILE=%t.dup.o --check-prefix=DUP-GNU
 

 # MEMBER-GNU-NEXT:     [Index]    Name
 # MEMBER-GNU-NEXT:     [    3]   .text.foo
 # MEMBER-GNU-NEXT:     [  238]   <?>
 # MEMBER-GNU-EMPTY:
 # MEMBER-GNU-NEXT:  COMDAT group section [    2] `.group1' [bar] contains 2 sections:
 # MEMBER-GNU-NEXT:     [Index]    Name
 # MEMBER-GNU-NEXT:     [  255]   <?>
 # MEMBER-GNU-NEXT:     [    6]   .rela.text.bar
+
+## Check warnings that are reported when the st_name field of the signature symbol goes past the end of the string table.
+
+## We set the content of the string table to '0061626300' ('\0abc\0') to fixup the size of the string table.
+## This makes it easier to test the boundary conditions.
+# RUN: yaml2obj %s -DSTRTABCONTENT="0061626300" -DSYM1STNAME=4 -DSYM2STNAME=5 -o %t.signame.o
+# RUN: llvm-readobj --elf-section-groups %t.signame.o 2>&1 | \
+# RUN:   FileCheck -DFILE=%t.signame.o %s --check-prefixes=SIGNAME1-WARN,SIGNAME1-LLVM --implicit-check-not=warning:
+# RUN: llvm-readelf --elf-section-groups %t.signame.o 2>&1 | \
+# RUN:   FileCheck -DFILE=%t.signame.o %s --check-prefixes=SIGNAME1-WARN,SIGNAME1-GNU --implicit-check-not=warning:
+
+# SIGNAME1-WARN: warning: '[[FILE]]': unable to get the name of the symbol with index 2: st_name (0x5) is past the end of the string table of size 0x5
+
+# SIGNAME1-LLVM: Signature: {{$}}
+# SIGNAME1-LLVM: Signature: <?>
+
+# SIGNAME1-GNU: COMDAT group section [    1] `.group' [] contains 2 sections:
+# SIGNAME1-GNU: COMDAT group section [    2] `.group1' [<?>] contains 2 sections:
+
+## Chech we report a warning when the string table that contains the signature symbol name is not null-terminated.
+
+# RUN: yaml2obj %s -DSTRTABCONTENT="0061626361" -DSYM1STNAME=4 -DSYM2STNAME=5 -o %t.signame2.o
+# RUN: llvm-readobj --elf-section-groups %t.signame2.o 2>&1 | \
+# RUN:   FileCheck -DFILE=%t.signame2.o %s --check-prefixes=SIGNAME2-WARN,SIGNAME2-LLVM --implicit-check-not=warning:
+# RUN: llvm-readelf --elf-section-groups %t.signame2.o 2>&1 | \
+# RUN:   FileCheck -DFILE=%t.signame2.o %s --check-prefixes=SIGNAME2-WARN,SIGNAME2-GNU --implicit-check-not=warning:
+
+# SIGNAME2-WARN: warning: '[[FILE]]': unable to get the string table for SHT_SYMTAB section with index 7: SHT_STRTAB string table section [index 8] is non-null terminated
+
+# SIGNAME2-LLVM: Signature: <?>
+# SIGNAME2-LLVM: Signature: <?>
+
+# SIGNAME2-GNU: COMDAT group section [    1] `.group' [<?>] contains 2 sections:
+# SIGNAME2-GNU: COMDAT group section [    2] `.group1' [<?>] contains 2 sections:

llvm/tools/llvm-readobj/ELFDumper.cpp

static StringRef tryGetSectionName(const ELFFile<ELFT> &Obj,
   else
     Dump.reportUniqueWarning(createError("unable to get the name of the " +
                                          describe(Obj, Sec) + ": " +
                                          toString(SecNameOrErr.takeError())));
   return "<?>";
 }
 
 template <class ELFT> std::vector<GroupSection> DumpStyle<ELFT>::getGroups() {
-  auto GetSignature = [&](const Elf_Sym &Sym,
+  auto GetSignature = [&](const Elf_Sym &Sym, unsigned SymNdx,
                           const Elf_Shdr &Symtab) -> StringRef {
     Expected<StringRef> StrTableOrErr = Obj.getStringTableForSymtab(Symtab);
     if (!StrTableOrErr) {
       reportUniqueWarning(createError("unable to get the string table for " +
                                       describe(Obj, Symtab) + ": " +
                                       toString(StrTableOrErr.takeError())));
       return "<?>";
     }
 
-    // TODO: this might lead to a crash or produce a wrong result, when the
-    // st_name goes past the end of the string table.
+    StringRef Strings = *StrTableOrErr;
+    if (Sym.st_name >= Strings.size()) {
+      reportUniqueWarning(createError(
+          "unable to get the name of the symbol with index " + Twine(SymNdx) +
+          ": st_name (0x" + Twine::utohexstr(Sym.st_name) +
+          ") is past the end of the string table of size 0x" +
+          Twine::utohexstr(Strings.size())));
+      return "<?>";
+    }
+
     return StrTableOrErr->data() + Sym.st_name;
   };
 
   std::vector<GroupSection> Ret;
   uint64_t I = 0;
   for (const Elf_Shdr &Sec : cantFail(Obj.sections())) {
     ++I;
     if (Sec.sh_type != ELF::SHT_GROUP)
       continue;
 
     StringRef Signature = "<?>";
     if (Expected<const Elf_Shdr *> SymtabOrErr = Obj.getSection(Sec.sh_link)) {
       if (Expected<const Elf_Sym *> SymOrErr =
               Obj.template getEntry<Elf_Sym>(**SymtabOrErr, Sec.sh_info))
-        Signature = GetSignature(**SymOrErr, **SymtabOrErr);
+        Signature = GetSignature(**SymOrErr, Sec.sh_info, **SymtabOrErr);
       else
         reportUniqueWarning(createError(
             "unable to get the signature symbol for " + describe(Obj, Sec) +
             ": " + toString(SymOrErr.takeError())));
     } else {
       reportUniqueWarning(createError("unable to get the symbol table for " +
                                       describe(Obj, Sec) + ": " +
                                       toString(SymtabOrErr.takeError())));