[llvm-readelf/llvm-readobj] - Test the case when e_shstrndx==SHN_XINDEX, but sec[0].sh_link is broken.

[grimar]

Summary

When e_shstrndx is equal to SHN_XINDEX,
the index of the section string table section should
be taken from zero section sh_link field.
If sh_link is broken, e.g. contains an index that is
larger than number of sections, then error is reported.

This error message was untested before.

[jhenderson]

It seems to me like the code here and the equivalent code in D65391 are basically identical and should be deduplicated, to avoid the need for two different test cases. Could you merge the two, by adding a "CheckType" parameter or similar to the getSectionName code in ELF.h? I was completely independently looking at this myself, after running into an issue with this code relating to some local patches we have, caused by the code duplication.

[grimar]

In D65388#1606137, @jhenderson wrote:

It seems to me like the code here and the equivalent code in D65391 are basically identical and should be deduplicated, to avoid the need for two different test cases. Could you merge the two, by adding a "CheckType" parameter or similar to the getSectionName code in ELF.h? I was completely independently looking at this myself, after running into an issue with this code relating to some local patches we have, caused by the code duplication.

This duplication was introduced in D63266. The intention was to skip non-critical errors to allow readelf to dump things.
I though about this and possible similar situations today too, btw. My idea was to add some kind of error policy for stuff in ELF.h.
For example ERR_MODE_STRICT which would act as now, reporting everything, and mode ERR_MODE_DUMP which dumpers and tools
like obj2yaml could use (obj2yaml can win from relaxing the error checking a lot).
I had no chance to investigate this, can take a look closer tomorrow.

[jhenderson]

In D65388#1606226, @grimar wrote:

This duplication was introduced in D63266. The intention was to skip non-critical errors to allow readelf to dump things.
I though about this and possible similar situations today too, btw. My idea was to add some kind of error policy for stuff in ELF.h.
For example ERR_MODE_STRICT which would act as now, reporting everything, and mode ERR_MODE_DUMP which dumpers and tools
like obj2yaml could use (obj2yaml can win from relaxing the error checking a lot).
I had no chance to investigate this, can take a look closer tomorrow.

Thanks, that sounds like a good plan to explore to me.

[grimar]

In D65388#1606277, @jhenderson wrote:

In D65388#1606226, @grimar wrote:

This duplication was introduced in D63266. The intention was to skip non-critical errors to allow readelf to dump things.
I though about this and possible similar situations today too, btw. My idea was to add some kind of error policy for stuff in ELF.h.
For example ERR_MODE_STRICT which would act as now, reporting everything, and mode ERR_MODE_DUMP which dumpers and tools
like obj2yaml could use (obj2yaml can win from relaxing the error checking a lot).
I had no chance to investigate this, can take a look closer tomorrow.

Thanks, that sounds like a good plan to explore to me.

I investigated this and ended up with a warning handler passed as a parameter.
It seems that making a global warning handler for ELFFile<ELFT> it too intrusive,
but such a local solution looks flexible and seems works good: D65515

[grimar]

I am suspending this. If we land D65515, this one will be excessive.

[grimar]

D65515 was landed.

test/tools/llvm-readobj/elf-invalid-sechdr-strtab-index.test

+## Check the case when e_shstrndx contains an invalid section header table index.
+
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: not llvm-readelf --headers -S 2>&1 %t1 | FileCheck %s -DFILE=%t1 --check-prefixes=GNU,GNU-SHSTRNDX
+# RUN: not llvm-readobj --headers -S 2>&1 %t1 | FileCheck %s -DFILE=%t1 --check-prefixes=LLVM,LLVM-SHSTRNDX
+
+# GNU:          ELF Header:
+# GNU-SHSTRNDX:  Section header string table index: 255
+# GNU-LINK:      Section header string table index: 65535 (255)
+# GNU-NEXT:      There are 4 section headers, starting at offset 0x40:
+# GNU:          Section Headers:
+# GNU-NEXT:      [Nr] Name
+# GNU-EMPTY:
+# GNU-NEXT:      error: '[[FILE]]': section header string table index 255 does not exist
+
+# LLVM:          ElfHeader {
+# LLVM-SHSTRNDX:  StringTableSectionIndex: 255
+# LLVM-LINK:      StringTableSectionIndex: 65535 (255)
+# LLVM-NEXT:     }
+# LLVM-NEXT:     Sections [
+# LLVM-EMPTY:
+# LLVM-NEXT:     error: '[[FILE]]': section header string table index 255 does not exist
+
+--- !ELF
+FileHeader:
+  Class:     ELFCLASS64
+  Data:      ELFDATA2LSB
+  Type:      ET_REL
+  Machine:   EM_X86_64
+  SHStrNdx:  0xFF
+
+# RUN: yaml2obj --docnum=2 %s -o %t2
+# RUN: not llvm-readelf --headers -S 2>&1 %t2 | FileCheck %s -DFILE=%t2 --check-prefixes=GNU,GNU-LINK
+# RUN: not llvm-readobj --headers -S 2>&1 %t2 | FileCheck %s -DFILE=%t2 --check-prefixes=LLVM,LLVM-LINK
+
+--- !ELF
+FileHeader:
+  Class:    ELFCLASS64
+  Data:     ELFDATA2LSB
+  Type:     ET_REL
+  Machine:  EM_X86_64
+## SHN_XINDEX == 0xffff.
+  SHStrNdx: 0xffff
+Sections:
+  - Type: SHT_NULL
+    Link: 0xff

test/tools/llvm-readobj/elf-invalid-shstrndx.test

-# RUN: yaml2obj %s -o %t
-# RUN: not llvm-readelf --headers -S 2>&1 %t | FileCheck %s -DFILE=%t --check-prefix=GNU
-# RUN: not llvm-readobj --headers -S 2>&1 %t | FileCheck %s -DFILE=%t --check-prefix=LLVM
-
-# GNU:      ELF Header:
-# GNU:        Section header string table index: 255
-# GNU-NEXT:   There are 4 section headers, starting at offset 0x40:
-# GNU:      Section Headers:
-# GNU-NEXT:  [Nr] Name
-# GNU-EMPTY:
-# GNU-NEXT:  error: '[[FILE]]': section header string table index 255 does not exist
-
-# LLVM:      ElfHeader {
-# LLVM:        StringTableSectionIndex: 255
-# LLVM-NEXT: }
-# LLVM-NEXT: Sections [
-# LLVM-EMPTY:
-# LLVM-NEXT: error: '[[FILE]]': section header string table index 255 does not exist
-
---- !ELF
-FileHeader:
-  Class:     ELFCLASS64
-  Data:      ELFDATA2LSB
-  Type:      ET_REL
-  Machine:   EM_X86_64
-  SHStrNdx:  0xFF

tools/llvm-readobj/ELFDumper.cpp

static StringRef getSectionName(const typename ELFT::Shdr &Sec,
                                 const ELFObjectFile<ELFT> &ElfObj,
                                 ArrayRef<typename ELFT::Shdr> Sections) {
   const ELFFile<ELFT> &Obj = *ElfObj.getELFFile();
   uint32_t Index = Obj.getHeader()->e_shstrndx;
   if (Index == ELF::SHN_XINDEX)
     Index = Sections[0].sh_link;
   if (!Index) // no section string table.
     return "";
-  // TODO: Test a case when the sh_link of the section with index 0 is broken.
   if (Index >= Sections.size())
     reportError(ElfObj.getFileName(),
                 createError("section header string table index " +
                             Twine(Index) + " does not exist"));
   StringRef Data = toStringRef(unwrapOrError(
       Obj.template getSectionContentsAsArray<uint8_t>(&Sections[Index])));
   return unwrapOrError(Obj.getSectionName(&Sec, Data));
 }