[Ltrace-devel] [PATCH v2] xtensa: add xtensa support

[Petr Machata]

Max Filippov <jcmvbkbc at gmail.com> writes:

> On Sat, Jan 3, 2015 at 5:53 PM, Petr Machata <pmachata at redhat.com> wrote:
>> Max Filippov <jcmvbkbc at gmail.com> writes:
>>> On Fri, Jan 2, 2015 at 4:32 AM, Petr Machata <pmachata at redhat.com> wrote:
>>>>> +int
>>>>> +arch_elf_init(struct ltelf *lte, struct library *lib)
>>>>> +{
>>>>> +     Elf_Scn *scn;
>>>>> +     GElf_Shdr shdr;
>>>>> +     GElf_Addr relplt_addr;
>>>>> +     GElf_Xword relplt_size;
>>>>> +     GElf_Phdr phdr;
>>>>> +     size_t i;
>>>>> +
>>>>> +     for (i = 0; gelf_getphdr(lte->elf, i, &phdr) != NULL; ++i) {
>>>>> +             if (phdr.p_type == PT_LOAD) {
>>>>> +                     lib->arch.loadable_sz =
>>>>> +                             phdr.p_vaddr + phdr.p_memsz - lte->bias;
>
> [...]
>
>>>> Also, correct me if I'm wrong, but loadable_sz actually points to the
>>>> end of the region, and hence is not a size.  Would loadable_end be a
>>>> better name?
>>>
>>> The intention was to calculate the size of loadable part of a library, to be
>>> able to tell later, given library base address and a pointer, if it
>>> points inside
>>> the library or outside of it.
>>
>> Bias doesn't influence size, only the, um, "coordinate": where it's
>> mapped.
>>
>> But anyway: your formula is p_vaddr + p_memsz ± bias.  The first is a
>> coordinate, the second is size, adding the two together gives you
>> another coordinate (of the end of the buffer pointed to by p_vaddr).
>> Bias is again size, so the result is an address, not a size.  Hence why
>> I think it's a misnomer.
>
> Looks like I should have taken the minimal of phdr.p_vaddr and subtract
> it from the maximal phdr.p_vaddr + phdr.p_memsz. It works with bias
> instead of phdr.p_vaddr just because all my libs are not prelinked,
> and bias is in fact a library load address.

Right, highest PT_LOAD's p_vaddr + its p_memsz - lowest PT_LOAD's
p_vaddr.

However, there's this piece in your code:

+			/* Some references may be resolved at this point, they
+			 * will point outside the loadable area of their own
+			 * library.  */
+			if (libsym->arch.resolved_addr >= base &&
+			    libsym->arch.resolved_addr - base < sz) {
+				libsym->arch.type = XTENSA_PLT_UNRESOLVED;
+				proc_activate_delayed_symbol(proc, libsym);
+			} else {
+				libsym->arch.type = XTENSA_PLT_RESOLVED;
+			}

Now imagine a DSO, e.g. a very large DSO might exhibit this behavior,
which is biased by less than its size.  I.e. there's an overlap of
addresses, and if a symbol happens to fall into the overlap, your code
would assume it's unresolved even if in fact it already could have been
resolved.

I don't know how much of a problem this would be and don't know in any
case how to fix it, and anyway, it might be theoretical (I don't know
how xtensa maps heap, stack, main binary, DSO's etc., this might not
even come up in practice).  Just something I wanted to draw your
attention to.

Thanks,
Petr