[PATCH] D126898: [COFF] Check table ptr more thoroughly and ignore empty sections

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D126898#3566111 <https://reviews.llvm.org/D126898#3566111>, @glandium wrote:

> In D126898#3565873 <https://reviews.llvm.org/D126898#3565873>, @mstorsjo wrote:
>
>> Can you upload that binary somewhere?
>
> This is not the exact binary this happened with, but it's reproducible with this one:
>
> - download https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/CVYLgJ7JTzKIZqeMnxZaXA/runs/0/artifacts/public/build/target.zip
> - unzip `firefox/uninstall/helper.exe` from that zip
> - run `llvm-strip` on it.

Thanks! I can see what's happening here. Unfortunately, it seems like that binary is somewhat broken, and libObject used to accept it silently before.

Looking at the binary with `llvm-readobj --sections --file-headers`, I see this:

    BaseRelocationTableRVA: 0x43000
    BaseRelocationTableSize: 0xB68
  
  Section {
    Number: 4
    Name: .ndata (2E 6E 64 61 74 61 00 00)
    VirtualSize: 0x1C000
    VirtualAddress: 0x40000
    RawDataSize: 512
  
  Section {
    Number: 6
    Name: .reloc (2E 72 65 6C 6F 63 00 00)
    VirtualSize: 0xB68
    VirtualAddress: 0x64000
    RawDataSize: 3072

The base relocation table usually resides in the `.reloc` section, and as the exact size (`0xB68`) matches, it looks like it just has got the wrong RVA in the header. With the new, more thorough checks, libObject notices that the RVA points into the `.ndata` section (within `0x40000 - 0x5C000`) but there's only raw data covering `0x40000 - 0x40200`, and `0x43000` is outside of that. Previously we'd just set up a pointer into bogus/out of range data in these cases, now we error out.

So the file is clearly buggy here, but it could, in some cases, be quite valid to have RVAs pointing outside of the allocated raw data. (Normally, the data size outside of `RawDataSize` up until `VirtualSize` is allocated and readable at runtime, but zero-initialized instead of initialized with data from the file.)

In this case, the check was added to avoid crashes, and for the specific case of `SizeOfRawData == 0` we return `SectionStrippedError`, which then is silently treated as not an error (but not initializing e.g. the `BaseRelocHeader` pointer, to avoid trying to read from it).

To avoid crashing from RVAs outside of the allocated bounds, but still not flat out erroring out on binaries that are somewhat broken (the same issue causes llvm-readobj to reject the binary, so I had to use an older llvm-readobj binary to look at it), should we just treat all those cases as a silent "section stripped" case?

E.g. this fixes the issue:

  diff --git a/llvm/lib/Object/COFFObjectFile.cpp b/llvm/lib/Object/COFFObjectFile.cpp
  index 65166b3710a0..f7d957ee2682 100644
  --- a/llvm/lib/Object/COFFObjectFile.cpp
  +++ b/llvm/lib/Object/COFFObjectFile.cpp
  @@ -483,18 +483,12 @@ Error COFFObjectFile::getRvaPtr(uint32_t Addr, uintptr_t &Res,
         // fail, otherwise it will be impossible to use this object as debug info
         // in LLDB. Return SectionStrippedError here so that
         // COFFObjectFile::initialize can ignore the error.
  -      if (Section->SizeOfRawData == 0)
  -        return make_error<SectionStrippedError>();
  +      // Somewhat common binaries may have RVAs pointing outside of the
  +      // provided raw data. Instead of rejecting the binaries, just
  +      // treat the section as stripped for these purposes.
         if (Section->SizeOfRawData < Section->VirtualSize &&
             Addr >= SectionStart + Section->SizeOfRawData) {
  -        if (ErrorContext)
  -          return createStringError(object_error::parse_failed,
  -                                   "RVA 0x%" PRIx32
  -                                   " for %s found but data is incomplete",
  -                                   Addr, ErrorContext);
  -        return createStringError(
  -            object_error::parse_failed,
  -            "RVA 0x%" PRIx32 " found but data is incomplete", Addr);
  +        return make_error<SectionStrippedError>();
         }
         uint32_t Offset = Addr - SectionStart;
         Res = reinterpret_cast<uintptr_t>(base()) + Section->PointerToRawData +


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126898/new/

https://reviews.llvm.org/D126898