Page MenuHomePhabricator

[libObject, llvm-readobj] - Reimplement `ELFFile<ELFT>::getEntry`.
ClosedPublic

Authored by grimar on Dec 14 2020, 6:20 AM.

Details

Summary

Currently, ELFFile<ELFT>::getEntry does not check an index of
an entry. Because of that the code might read past the end of the symbol
table silently. I've added a test to llvm-readobj\ELF\relocations.test
to demonstrate the possible issue. Also, I've added a unit test for
this method.

After this change, getEntry stops reporting the section index and
reuses the getSectionContentsAsArray method, which already has
all the validation needed. Our related warnings now provide
more and better context sometimes.

Diff Detail

Event Timeline

grimar created this revision.Dec 14 2020, 6:20 AM
grimar requested review of this revision.Dec 14 2020, 6:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2020, 6:20 AM

Does this change impact reading a dynamic symbol table for an object with no section header table? In such a case, the size is derived from the hash table, so I imagine it'll work the same, but ideally it should be tested.

llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
6 ↗(On Diff #311564)

Seems to me like this change is a regression on the old behaviour? Same with llvm-objdump and llvm-size. Please don't land it with this.

llvm/test/tools/llvm-readobj/ELF/reloc-symbol-with-versioning.test
2–6

Why did this change?

llvm/test/tools/llvm-readobj/ELF/relocations.test
503

Although actually I think the comment would be better as "Check that we report a warning when a relocation has a symbol index past the end of the symbol table", to avoid confusion with an attempt to look up a symbol by name.

535–538

I'm not sure you really need these other relocations - just the one with the invalid symbol index would be enough.

llvm/unittests/Object/ELFObjectFileTest.cpp
353–354

This could be ASSERT_THAT_EXPECTED(SecOrErr, Succeeded()); right?

grimar added inline comments.Dec 15 2020, 12:48 AM
llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
6 ↗(On Diff #311564)

This is because of the following code in llvm/Object/ELFObjectFile.h

const Elf_Sym *getSymbol(DataRefImpl Sym) const {
  auto Ret = EF.template getEntry<Elf_Sym>(Sym.d.a, Sym.d.b);
  if (!Ret)
    report_fatal_error(Ret.takeError());
  return *Ret;
}

Previously getEntry<Elf_Sym> ignored this error
Now it started to call report_fatal_error right here.

Ideally, we should never call report_fatal_error. I am also not sure I can call it a regression,
because the code had this call before my change too. I.e. I believe it might report LLVM ERROR
for another broken input already. It is just a luck that it "worked".
And the root of the issue is in Elf_Sym *getSymbol, which should return Expected<> I think.

Do you want me to update the getSymbol first?

jhenderson added inline comments.Dec 15 2020, 1:06 AM
llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
6 ↗(On Diff #311564)

The way I see it, before we weren't getting crashes in the test and now we are, due to the reworking of the function, so it's a regression, at least for the cases that are tested. I think fixing getSymbol may be a prerequisite of this change then.

grimar added inline comments.
llvm/test/tools/llvm-nm/invalid-symbol-table-size.test
6 ↗(On Diff #311564)

I've prepared the patch: D93297

grimar updated this revision to Diff 312183.Dec 16 2020, 5:23 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/reloc-symbol-with-versioning.test
2–6

Left from debugging. I've added --implicit-check-not=warning: just in case,
as this test was updated to stop triggering the fixed issue.

llvm/test/tools/llvm-readobj/ELF/relocations.test
535–538

I think we need at least 2 to test the new "unable to read an entry with index X" part properly.

llvm/unittests/Object/ELFObjectFileTest.cpp
353–354

I think so. But this place is gone.

Does this change impact reading a dynamic symbol table for an object with no section header table? In such a case, the size is derived from the hash table, so I imagine it'll work the same, but ideally it should be tested.

Nope, this patch is unrelated to dynamic symbols. I am changing getEntry(const Elf_Shdr &Section, uint32_t Entry), it accepts the Elf_Shdr.
We print dynamic relocations with the use of DynRegionInfo::getAsArrayRef() to get entries.

template <class ELFT> void DumpStyle<ELFT>::printDynamicRelocationsHelper() {
  const bool IsMips64EL = this->Obj.isMips64EL();
  const DynRegionInfo &DynRelaRegion = this->dumper().getDynRelaRegion();
  if (DynRelaRegion.Size > 0) {
    printDynamicRelocHeader(ELF::SHT_RELA, "RELA", DynRelaRegion);
    for (const Elf_Rela &Rela : this->dumper().dyn_relas())
      printDynamicReloc(Relocation<ELFT>(Rela, IsMips64EL));
  }
...


template <typename ELFT>
typename ELFDumper<ELFT>::Elf_Rela_Range ELFDumper<ELFT>::dyn_relas() const {
  return DynRelaRegion.getAsArrayRef<Elf_Rela>();
}

LG.

The diagnostic in getSectionContentsAsArray has been improved

jhenderson accepted this revision.Dec 17 2020, 3:43 AM

LGTM, with nit.

FYI, tomorrow is my last day working before my Christmas break. I'll be back in from around the 4th of January. I'll try to clear as much of my review backlog as possible, but apologies if I fail to get to everything.

llvm/test/tools/llvm-readobj/ELF/relocations.test
503
This revision is now accepted and ready to land.Dec 17 2020, 3:43 AM

LGTM, with nit.

FYI, tomorrow is my last day working before my Christmas break. I'll be back in from around the 4th of January. I'll try to clear as much of my review backlog as possible, but apologies if I fail to get to everything.

Thanks and happy holidays!

hvdijk added a subscriber: hvdijk.Dec 20 2020, 10:25 AM
hvdijk added inline comments.
llvm/include/llvm/Object/ELF.h
629

This multiplication does not produce consistent results across hosts, as size_t may or may not be 64 bits so the result may or may not be truncated to 32 bits. It's fairly harmless since it's just the error message that's affected, but it causes test failures on some hosts. The original code cast Entry to uint64_t before multiplying, I think the new code needs to do the same.

grimar marked an inline comment as done.Dec 21 2020, 1:21 AM
grimar added inline comments.
llvm/include/llvm/Object/ELF.h
629

Thanks! This was already fixed in rG43327ba98da138d9d87e13f65675a0b600dae018