[Mesa-dev] [Mesa-stable] [PATCH] util/build-id: Fix address comparison for binaries with LOAD vaddr > 0

[Emil Velikov]

On 1 February 2018 at 11:35, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 25 January 2018 at 15:11, Stephan Gerhold <stephan at gerhold.net> wrote:
>> On Thu, Jan 25, 2018 at 11:22:10AM +0000, Emil Velikov wrote:
>>> On 24 January 2018 at 14:13, Stephan Gerhold <stephan at gerhold.net> wrote:
>>> > build_id_find_nhdr_for_addr() fails to find the build-id if the first LOAD
>>> > segment has a virtual address other than 0x0.
>>> >
>>> > For most shared libraries, the first LOAD segment has vaddr=0x0:
>>> >
>>> >     Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>>> >     LOAD           0x000000 0x00000000 0x00000000 0x2d2e26 0x2d2e26 R E 0x1000
>>> >     LOAD           0x2d2e54 0x002d3e54 0x002d3e54 0x2e248 0x2f148 RW  0x1000
>>> >
>>> > However, compiling the Intel Vulkan driver as 32-bit binary on Android produces
>>> > the following ELF header with vaddr=0x8000 instead:
>>> >
>>> >     Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>>> >     PHDR           0x000034 0x00008034 0x00008034 0x00100 0x00100 R   0x4
>>> >     LOAD           0x000000 0x00008000 0x00008000 0x224a04 0x224a04 R E 0x1000
>>> >     LOAD           0x225710 0x0022e710 0x0022e710 0x25988 0x27364 RW  0x1000
>>> >
>>> > build_id_find_nhdr_callback() compares the address of dli_fbase from dladdr()
>>> > and dlpi_addr from dl_iterate_phdr(). With vaddr > 0, these point to a
>>> > different memory address, e.g.:
>>> >
>>> >     dli_fbase=0xd8395000 (offset 0x8000)
>>> >     dlpi_addr=0xd838d000
>>> >
>>> > At least on glibc and bionic (Android) dli_fbase refers to the address where
>>> > the shared object is mapped into the process space, whereas dlpi_addr is just
>>> > the base address for the vaddrs declared in the ELF header.
>>> >
>>> > To compare them correctly, we need to calculate the start of the mapping
>>> > by adding the vaddr of the first LOAD segment to the base address.
>>> >
>>> > Cc: Chad Versace <chadversary at chromium.org>
>>> > Cc: Emil Velikov <emil.velikov at collabora.com>
>>> > Cc: Tapani Pälli <tapani.palli at intel.com>
>>> > Cc: <mesa-stable at lists.freedesktop.org>
>>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104642
>>> > Fixes: 5c98d38 "util: Query build-id by symbol address, not library name"
>>> > ---
>>> Based on my observation of glibc code and reading at the spec, I think
>>> this is correct.
>>> Admittedly the man page could be improved.
>>>
>>> FWIW I've poked the #musl people about this change last night, and
>>> haven't heard any feedback yet.
>>> Be that about a) our understanding of how it should work or b) musl's
>>> implementation on the topic.
>>
>> I found a related discussion about the implementation of dli_fbase on the
>> musl mailing list[1]. The FreeBSD man page for dladdr()[2] linked in the
>> message on the musl mailing list is a bit more specific about dli_fbase:
>>
>>         "The base address at which the shared object is mapped into the
>>         address space of the calling process."
>>
>> ... which is - at least as far as I understand it - exactly how glibc and
>> bionic behave and the reason why we need this patch for LOAD vaddrs != 0.
>>
>> However, from what I've noticed when testing with musl, they seem to handle
>> it unlike glibc/bionic/the FreeBSD man page. musl always returns the base
>> address without the offset where the shared object is mapped.
>>
>> Technically, this means that this patch will break on musl in the rare
>> situation that you actually link a shared library with LOAD vaddr != 0.
>> However, considering that only they seem to handle it differently, this might
>> be worth reporting to them instead?
>>
> musl does not have a bug tracker - according to the musl FAQ [A] and a
> quick search.
> Instead people should report bugs on IRC or ML :-\
>
> I've poked the for the second time yesterday [on IRC] and will do a
> final one tomorrow.
> Modulo any objections (mesa or musl devs) I'll push this.
>
The musl people have confirmed the inconsistency and pushed a fix on
their end [1].
Added a reference to it and pushed this patch to master.

-Emil

https://git.musl-libc.org/cgit/musl/commit/?id=b3ae7beabb9f0c219bb8a8b63567a01c6530c1ac