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

Thu Jun 9 21:18:56 PDT 2022

mstorsjo added a comment.

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

>> 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.
>
> From a cursory look, it seems to come from NSIS itself not handing BaseRelocationTableRVA. Its prebuild binaries come with stubs that actually have it set to 0 and thus don't have a .reloc section. The stubs we're using in the case of that helper.exe were compiled with mingw-clang and linked with mingw-lld, and do contain a .reloc section. The helper.exe binary ends up having the same BaseRelocationTableRVA as the stub, but NSIS moved the .reloc section, so it's not correct anymore.

Ouch, that does indeed sound like something that should be fixed.

(I don’t remember offhand, but it might be possible to omit the reloc section for an executable by linking with ‘-Wl,-Xlink=-fixed` and maybe `-Wl,--disable-dynamicbase`. Not sure if it’s actually left out, or if it is just not used.)

Fixing NSIS definitely sounds like something that should be done here.

> So... maybe you shouldn't actually relax things as per D127345 <https://reviews.llvm.org/D127345>, because it seems like a genuine bug in NSIS that should be fixed rather than worked around.

Well, while the NSIS bug is real, and it’s great that the restrictive object library caught it, I think we still should go with that patch too anyway - the PE executable isn’t entirely broken structurally, the table just lies in zero-initialized, allocated memory. And that lets the llvm tools operate on files that are potentially valid, instead of flat out rejecting them.

@rnk - what’s your take on this?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126898