Your changes make sense to me, as do your concerns about accessing memory past
the end of the synthetic frame header.
The only change I can suggest is that you should take the opportunity to stop
the explicit bytes offsets in the code and write something like
struct dwarf_eh_frame_hdr *synth_eh_frame_hdr;
unsigned char
synth_eh_frame_space[sizeof(*synth_eh_frame_hdr)+sizeof(Elf_W(Addr))];
synth_eh_frame_hdr = (struct_dwarf_eh_frame_hdr *) synth_eh_frame_space;
…
synth_eh_frame_hdr->version = DW_EH_VERSION;
synth_eh_frame_hdr->eh_frame_ptr_enc = …
etc.
because I don’t understand why the original code used the struct member names
in comments and not in the actual code.
Doug
> On Jun 30, 2017, at 10:57 AM, Ben Avison <bavi...@riscosopen.org> wrote:
>
> Thanks, that does appear to fix things in my case. I thought I would do a
> little more investigating in case it is useful in justifying your proposed
> patch.
>
> Whenever we use synth_eh_frame_hdr, then hdr is pointed at it, so
> hdr->table_enc can only be DW_EH_PR_omit, and the FDE length word pointer
> which is ultimately dereferenced by dwarf_extract_proc_info_from_fde() is
> the one (returned from dwarf_find_eh_frame_section()) which
> dwarf_callback() sticks into the word after synth_eh_frame_hdr. In my case,
> it seems that whenever eh_frame_start is read from the word after
> synth_eh_frame_hdr like this, the FDE length word read by
> dwarf_extract_proc_info_from_fde() is always 0, irrespective of object
> file. As a result, it always returns -UNW_ENOINFO, so linear_search() also
> always fails. Does that make sense?
>
> In passing, I notice that at lines 547 and 584, we're referencing an
> address beyond the bounds of synth_eh_frame_hdr, which could cause stack
> corruption, depending on architecture and compiler. Wouldn't something like
> this be better?
>
>
> --- a/src/dwarf/Gfind_proc_info-lsb.c
> +++ b/src/dwarf/Gfind_proc_info-lsb.c
> @@ -482,7 +482,7 @@ dwarf_callback (struct dl_phdr_info *info, size_t size,
> void *ptr)
> unw_accessors_t *a;
> long n;
> int found = 0;
> - struct dwarf_eh_frame_hdr synth_eh_frame_hdr;
> + unsigned char synth_eh_frame_hdr[sizeof (struct dwarf_eh_frame_hdr) +
> sizeof (Elf_W (Addr))];
> #ifdef CONFIG_DEBUG_FRAME
> unw_word_t start, end;
> #endif /* CONFIG_DEBUG_FRAME*/
> @@ -537,15 +537,14 @@ dwarf_callback (struct dl_phdr_info *info, size_t size,
> void *ptr)
> eh_frame = dwarf_find_eh_frame_section (info);
> if (eh_frame)
> {
> - unsigned char *p = (unsigned char *) &synth_eh_frame_hdr;
> Debug (1, "using synthetic .eh_frame_hdr section for %s\n",
> info->dlpi_name);
> - /* synth_eh_frame_hdr.version */ p[0] = DW_EH_VERSION;
> - /* synth_eh_frame_hdr.eh_frame_ptr_enc */ p[1] = DW_EH_PE_absptr |
> ((sizeof(Elf_W (Addr)) == 4) ? DW_EH_PE_udata4 : DW_EH_PE_udata8);
> - /* synth_eh_frame_hdr.fde_count_enc */ p[2] = DW_EH_PE_omit;
> - /* synth_eh_frame_hdr.table_enc */ p[3] = DW_EH_PE_omit;
> - *(Elf_W (Addr) *)(&p[4]) = eh_frame;
> - hdr = &synth_eh_frame_hdr;
> + /* version */ synth_eh_frame_hdr[0] = DW_EH_VERSION;
> + /* eh_frame_ptr_enc */ synth_eh_frame_hdr[1] = DW_EH_PE_absptr |
> ((sizeof(Elf_W (Addr)) == 4) ? DW_EH_PE_udata4 : DW_EH_PE_udata8);
> + /* fde_count_enc */ synth_eh_frame_hdr[2] = DW_EH_PE_omit;
> + /* table_enc */ synth_eh_frame_hdr[3] = DW_EH_PE_omit;
> + hdr = (struct dwarf_eh_frame_hdr *)synth_eh_frame_hdr;
> + *(Elf_W (Addr) *)(hdr + 1) = eh_frame;
> }
> }
>
>
> I'm not 100% sure whether that last word shouldn't be accessed via the
> unw_word_t type though, as that's how it's accessed when the value is read
> back again.
>
> Ben
>
>
>
> On Fri, 30 Jun 2017 06:08:18 +0100, Doug Moore <do...@rice.edu
> <mailto:do...@rice.edu>> wrote:
>
>> I didn't invent the single_fde field, but I wonder is it was intended to be
>> set in a case where the linear search failed and no unwind info was
>> obtained. So, I wonder if this patch
>>
>> --- a/src/dwarf/Gfind_proc_info-lsb.c
>> +++ b/src/dwarf/Gfind_proc_info-lsb.c
>> @@ -618,12 +618,13 @@ dwarf_callback (struct dl_phdr_info *info, size_t
>> size, void *ptr)
>>
>> /* XXX we know how to build a local binary search table for
>> .debug_frame, so we could do that here too. */
>> - cb_data->single_fde = 1;
>> found = linear_search (unw_local_addr_space, ip,
>> eh_frame_start, eh_frame_end, fde_count,
>> pi, need_unwind_info, NULL);
>> if (found != 1)
>> found = 0;
>> + else
>> + cb_data->single_fde = 1;
>> }
>> else
>> {
>>
>> addresses the problem, while being consistent with the greater purpose of
>> single_fde.
>>
>> Doug Moore
>>
>>
>>
>> Quoting libunwind-devel-requ...@nongnu.org:
>>
>>> Send Libunwind-devel mailing list submissions to
>>> libunwind-devel@nongnu.org
>>>
>>> To subscribe or unsubscribe via the World Wide Web, visit
>>> https://lists.nongnu.org/mailman/listinfo/libunwind-devel
>>> or, via email, send a message with subject or body 'help' to
>>> libunwind-devel-requ...@nongnu.org
>>>
>>> You can reach the person managing the list at
>>> libunwind-devel-ow...@nongnu.org
>>>
>>> When replying, please edit your Subject line so it is more specific
>>> than "Re: Contents of Libunwind-devel digest..."
>>>
>>>
>>> Today's Topics:
>>>
>>> 1. Help please with debugging DWARF unwinding on ARM (Ben Avison)
>>>
>>>
>>> ----------------------------------------------------------------------
>>>
>>> Message: 1
>>> Date: Thu, 29 Jun 2017 01:56:06 +0100
>>> From: "Ben Avison" <bavi...@riscosopen.org>
>>> To: libunwind-devel@nongnu.org
>>> Subject: [Libunwind-devel] Help please with debugging DWARF unwinding
>>> on ARM
>>> Message-ID: <op.y2kkvsell0n...@amoe.lan>
>>> Content-Type: text/plain; charset=iso-8859-15; format=flowed;
>>> delsp=yes
>>>
>>> Hi all,
>>>
>>> First a quick typo to report: at src/dwarf/Gparser.c:281, the memcpy()
>>> statement is lacking a trailing semicolon - this doesn't become apparent
>>> until you enable debugging.
>>>
>>> I have previously used libunwind successfully as the backend for
>>> gperftools, however I just get segfaults with the latest version.
>>>
>>> Some background: I'm running on ARM, and I configure libunwind with
>>> --enable-debug-frame then run the executable with UNW_ARM_UNWIND_METHOD=1
>>> to force DWARF unwinding.
>>>
>>> The revision in git which introduced the bug is 25413c7 (dwarf: Improve
>>> support for binaries missing the GNU_EH_FRAME segment).
>>>
>>> Ultimately, the segfault is triggered at the end of fetch_proc_info() where
>>> c->pi is uninitialised when it tried to dereference it. However, I'm having
>>> a hard time understanding the code well enough to know how to ensure it is
>>> initialised correctly.
>>>
>>> Gperftools always starts skipping through the call stack from an initial
>>> cursor position set up by unw_init_local(). The very first call to
>>> unw_step() is the one that fails.
>>>
>>> If I back out the changes from that git revision, the pi struct is
>>> initialised towards the end of dwarf_extract_proc_info_from_fde(), which is
>>> called from dwarf_search_unwind_table(), in the second such call from
>>> dwarf_find_proc_info().
>>>
>>> But using the head of git, dwarf_find_proc_info() exits early because
>>> cb_data.single_fde is set, so it never calls dwarf_search_unwind_table().
>>> single_fde is only set in one place, in dwarf_callback() - in a bit of code
>>> that was never previously called when no .eh_frame_hdr section is found
>>> (which appears to be true in my case). However, if single_fde is set then
>>> control flow has unavoidably also passed through linear_search(), which
>>> also calls dwarf_extract_proc_info_from_fde().
>>>
>>> It appears to me that the problem is that sometimes, the first time
>>> dwarf_extract_proc_info_from_fde() is called for a given pi struct, it is
>>> called with need_unwind_info=0, but then dwarf_find_proc_info() is later
>>> called with need_unwind_info=1 but due to single_fde being set, we never
>>> get the pi struct initialised by anyone.
>>>
>>> It's possible I've got the wrong end of the stick here, but I'm also far
>>> from sure what the best way to fix this is. Any advice gratefully received!
>>> Please bear in mind I have rather limited working knowledge of the DWARF
>>> spec.
>>>
>>> Thanks in advance,
>>> Ben
>>>
>>>
>>>
>>> ------------------------------
>>>
>>> Subject: Digest Footer
>>>
>>> _______________________________________________
>>> Libunwind-devel mailing list
>>> Libunwind-devel@nongnu.org
>>> https://lists.nongnu.org/mailman/listinfo/libunwind-devel
>>>
>>>
>>> ------------------------------
>>>
>>> End of Libunwind-devel Digest, Vol 124, Issue 11
>>> ************************************************
>>>
>>
>>
>>
>>
>> _______________________________________________
>> Libunwind-devel mailing list
>> Libunwind-devel@nongnu.org <mailto:Libunwind-devel@nongnu.org>
>> https://lists.nongnu.org/mailman/listinfo/libunwind-devel
>> <https://lists.nongnu.org/mailman/listinfo/libunwind-devel>
>
> !DSPAM:10223,595678b5326342783720507!
_______________________________________________
Libunwind-devel mailing list
Libunwind-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/libunwind-devel
