[PATCH] D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

In https://reviews.llvm.org/D40029#927209, @ruiu wrote:

> > Hmmm, I think that approach will be difficult for a couple of reasons. The first is that I think it will be difficult to get all the existing dynamic loaders to change unless we could think of an important use-case that can't be solved by just a single segment. I think that if I were a dynamic loader author I'd say "Why do you want multiple non-contiguous RELRO segments in an ELF file designed for an OS with a standard image layout? Why do I need to add complexity for an edge case?". The second is that there is a large lag time between getting a patch accepted in glibc and it getting deployed, for LTS releases we could be looking at several years before loaders that could support multiple RELRO segments.
>
> I don't know if it is actually a problem. In this patch, you are attempting to add a RELRO segment only for the first contiguous regions that should be RELRO. My understanding is that the existing loader works OK with multiple RELRO segments, but only the first RELRO segment takes effect. That means, if you create multiple RELRO segments, that effectively works the same as creating a RELRO for the first contiguous region, no? If that's the case, creating multiple RELRO should fix the crash bug that you are trying to fix.
>
> By doing that, we can buy time to fix dynamic loaders. As a result, the situation will be (1) if you are using an old loader, your binary still work, but the second and further RELRO segments are ignored, and (2) if you are using a newer loader, all RELRO segments are processed.
>
> The above was my plan. What am I missing?


The only thing that I think that you are missing is that with at least the current state of glibc, there is at least one case where the PT_GNU_RELRO PHDR is searched for from the end of the table, the comment alongside the code says, the PT_GNU_RELRO PHDR is usually last. So if we output more than one we can't guarantee that the first PHDR will be selected, this could be dangerous.

My suggestion is to:

- Change lld to use .data.rel.ro instead of .bss.rel.ro; this will sort out the non-user error case.
- I would prefer to emit a warning and only output one PT_GNU_RELRO header, although I concede that putting out as many PT_GNU_RELRO header as we need is valid response to a linker script containing in effect a user-error, however I think we should only do this until we've made the .bss.rel.ro change.


https://reviews.llvm.org/D40029