Page MenuHomePhabricator

[obj2yaml] - Dump the `EShNum` key in some cases.
ClosedPublic

Authored by grimar on Nov 25 2020, 6:19 AM.

Details

Summary

This patch starts emitting the EShNum key, when the e_shnum = 0
and the section header table exists.

e_shnum might be 0, when the the number of entries in the section header
table is larger than or equal to SHN_LORESERVE (0xff00).
In this case the real number of entries
in the section header table is held in the sh_size
member of the initial entry in section header table.

Currently, obj2yaml crashes when an object has e_shoff != 0 and the sh_size
member of the initial entry in section header table is 0.
This patch fixes it.

Diff Detail

Event Timeline

grimar created this revision.Nov 25 2020, 6:19 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: emaste. · View Herald Transcript
grimar requested review of this revision.Nov 25 2020, 6:19 AM
MaskRay accepted this revision.Nov 25 2020, 11:54 AM
MaskRay added inline comments.
llvm/test/tools/obj2yaml/ELF/eshnum.yaml
28

The following may be slightly better:

--- !ELF
FileHeader:
  Class:  ELFCLASS64
  Data:   ELFDATA2LSB
  Type:   ET_REL
SectionHeaderTable:
  NoHeaders: true
This revision is now accepted and ready to land.Nov 25 2020, 11:54 AM
grimar marked an inline comment as done.Nov 26 2020, 12:12 AM
grimar added inline comments.
llvm/test/tools/obj2yaml/ELF/eshnum.yaml
28

We currently don't emit the SectionHeaderTable. Though I guess it might be reasonable to do it when sections are out of order or when there is no section header.

jhenderson added inline comments.Nov 26 2020, 12:28 AM
llvm/test/tools/obj2yaml/ELF/eshnum.yaml
10
llvm/tools/obj2yaml/elf2yaml.cpp
238

I take it V doesn't normally contain a chunk for the null (index 0) section header?

grimar updated this revision to Diff 307778.Nov 26 2020, 12:47 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
llvm/tools/obj2yaml/elf2yaml.cpp
238

V contains all sections that are dumped from an object. The SHT_NULL section is
normally present, it dumped as a regular section, and so V usually contains it. This is assumed below:

for (const std::unique_ptr<ELFYAML::Chunk> &C :
     makeArrayRef(V).drop_front()) {

In the case revealed by this patch we have no section header and no sections are dumped,
that's why V is empty.

grimar added inline comments.Nov 26 2020, 12:51 AM
llvm/tools/obj2yaml/elf2yaml.cpp
238

"we have no section header" -> "we have 0 sections"

grimar added inline comments.Nov 26 2020, 12:56 AM
llvm/test/tools/obj2yaml/ELF/eshnum.yaml
28

BTW,

--- !ELF
FileHeader:
  Class:  ELFCLASS64
  Data:   ELFDATA2LSB
  Type:   ET_REL
SectionHeaderTable:
  NoHeaders: true

is not correct. In this case we have e_shnum = 0. So we should read the number of sections from
the sh_size member of the initial entry in section header table (see test case header comment).

I.e. I think the output here should be the same as YAML input.

jhenderson added inline comments.Nov 26 2020, 1:10 AM
llvm/test/tools/obj2yaml/ELF/eshnum.yaml
16–21

If you feed this output of obj2yaml back into yaml2obj, the final object doesn't really reflect what you had originally - it generates three section headers (.strtab and .shstrtab, plus the null one). Seems like we need to fix something, but not entirely sure what.

llvm/tools/obj2yaml/elf2yaml.cpp
238

You've lost me a little. In the case when e_shnum and sh_size of section table index 0 are both 0, obj2yaml doesn't populate V with anything? So there is a section header table, it's just that no sections are read, because we say there are none. Is that right?

grimar marked an inline comment as done.Nov 26 2020, 1:29 AM
grimar added inline comments.
llvm/test/tools/obj2yaml/ELF/eshnum.yaml
16–21

Ideally we might want to produce the same output as YAML input I think.
At the same time this is a corner case that I am not sure is worth to support.
I've tried to expain in the comment below.

llvm/tools/obj2yaml/elf2yaml.cpp
238

You've lost me a little. In the case when e_shnum and sh_size of section table index 0 are both 0, obj2yaml doesn't populate V with anything?

Yes.

So there is a section header table, it's just that no sections are read, because we say there are none. Is that right?

Yes.

The responsible code that returns us the sections list is:

template <class ELFT>
Expected<typename ELFT::ShdrRange> ELFFile<ELFT>::sections() const {
  const uintX_t SectionTableOffset = getHeader().e_shoff;
  if (SectionTableOffset == 0)
    return ArrayRef<Elf_Shdr>();

....

  const Elf_Shdr *First =
      reinterpret_cast<const Elf_Shdr *>(base() + SectionTableOffset);

  uintX_t NumSections = getHeader().e_shnum;
  if (NumSections == 0)
    NumSections = First->sh_size;

....
  return makeArrayRef(First, NumSections);
}

When e_shoff != 0 and e_shnum==0, it reads the number of sections from First->sh_size (which is 0) and returns us an empty array.
This is why no chunks are dumped.

To be able to support the case of this patch properly I think we should check the e_shnum manually. When it is 0, and the e_shoff != 0,
then we should dump the first section header table entry (manually). After that we should emit the EShNum = 0 key to preserve e_shnum.
With all of these steps done, the output of the test case will be the same as input.

But do we want all this complexity to support this particular corner case? I believe having all this conditions in the real life is unrealistic. I've only tried to fix the crash I've found accidentally.

grimar planned changes to this revision.Nov 26 2020, 4:18 AM
grimar marked an inline comment as done.

I am experimenting with the patch which preserves the EShNum key when e_shnum == 0. It is usefull for the normal scenario when
the number of sections is greater than or equal to the SHN_LORESERVE .

I'll rebase this patch later. I think that after we start emitting the EShNum = 0, this patch will produce an equivalent output for the input given.

grimar updated this revision to Diff 307843.Nov 26 2020, 5:30 AM
grimar retitled this revision from [obj2yaml] - Don't crash when dumping an object with no sections. to [obj2yaml] - Dump the `EShNum` key in some cases..
grimar edited the summary of this revision. (Show Details)

I am experimenting with the patch which preserves the EShNum key when e_shnum == 0. It is usefull for the normal scenario when
the number of sections is greater than or equal to the SHN_LORESERVE .

I'll rebase this patch later. I think that after we start emitting the EShNum = 0, this patch will produce an equivalent output for the input given.

It turns out this is impossible to prepare a different patch because of this crash.
So updating this one with the new functionality.

This revision is now accepted and ready to land.Nov 26 2020, 5:30 AM
grimar requested review of this revision.Nov 26 2020, 5:30 AM
jhenderson accepted this revision.Nov 27 2020, 12:14 AM

LGTM, with the suggested comment fixes.

llvm/test/tools/obj2yaml/ELF/eshnum.yaml
2
8
11
41–43
57
llvm/tools/obj2yaml/elf2yaml.cpp
297–299

I think you can make this more concise like this.

300
This revision is now accepted and ready to land.Nov 27 2020, 12:14 AM
This revision was automatically updated to reflect the committed changes.
grimar marked 6 inline comments as done.