[PATCH] D111181: [llvm-objcopy][ELF] Don't assume RELA sections are dynamic if they carry the SHF_ALLOC flag

Thu Oct 7 02:00:47 PDT 2021

ardb added a comment.

In D111181#3047338 <https://reviews.llvm.org/D111181#3047338>, @jhenderson wrote:

> If there's a real use-case, I'm not opposed to handling more unusual code, such as SHF_ALLOC relocations that are not intended for patching by the dynamic loader - if there's a still a need for it, I'd be happy to support working on a solution in llvm-objcopy/llvm-strip. The problem as I see it is that the only true way of determining whether a relocation section is really a static or dynamic relocation section is the dynamic table, which llvm-objcopy traditionally has not had a need of. We can't rely on the ELF type for two reasons:
>
> 1. Custom ELF types could be either relocatable or non-relocatable (this is less important - producers of such custom ELF objects usually have to rely on downstream patches to keep things working for them anyway).
> 2. Static relocations may exist in fully linked objects.

This case is actually covered by my latest revision: SHF_ALLOC is taken into account as before for non-ET_REL type files.

> A simpler approach than inspecting the dynamic table might be to inspect the type of the sh_link-ed section: SHT_SYMTAB implies a static relocation section, and SHT_DYNSYM a dynamic one. There is a small corner case this approach doesn't cover, namely relocation sections with sh_link=0, but in that case, falling back to another heuristic like SHF_ALLOC or parsing the dynamic section may be sufficient.

In that case, does it even matter? If there is no symtab to begin with, there is no risk of misidentifying a static one as a dynamic one, righr?

>> I guess you are saying that llvm-objcopy only targets the subset of well-formed, compliant ELF files that are likely to be emitted by tools created for building user space programs.
>
> As the person who reviewed 99% of the original ELF llvm-objcopy implementation, I can safely say that the intent of llvm-objcopy was to handle as many cases as made sense. At the time, Jake and I were unaware of any static relocation sections with SHF_ALLOC, hence it was implemented this way. Had we known of actual situations, we would have implemented it differently from the get-go: it certainly wasn't the intent for llvm-objcopy to be limited to handling a specific subset, but at the same time, we couldn't invest the time to handle every single corner case.
>
> There's another side-issue with SHF_ALLOC static relocations, at least in llvm-objcopy: llvm-objcopy tries very hard to not change contents of memory-allocated data, because it can impact address references. Having said that, I think in an ET_REL case, we can probably ignore this specific issue. It's therefore probably safe to assume that if the type is ET_REL, all relocations are static, but not necessarily otherwise. (I'm assuming the partially linked ET_RELs in the Linux kernel still have relocations to patch all inter-section references?).

Yes, the Linux kernel module loader relies on the relocation,. symtab and strtab sections to fix up all references (including undefined ones) once the ordinary allocatable sections have been copied to memory.

>> I think we can all agree that the error message here could be more helpful to the end user.
>
> I'd be happy to review a simple patch that simply added the words "static" or "dynamic" as appropriate to the message.

Thanks.

But if we disregard this patch, there is still an issue where llvm-objcopy is able to generate ELF files that are malformed according to its own standards. I.e.,

$ llvm-objcopy --set-section-flags=.rela.text=alloc,readonly <object file>

will generate a file that llvm-objcopy itself is not able to read.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111181