This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: eh-frame: CIE initial_instructions overflow
- From: Christophe Lyon <christophe dot lyon at linaro dot org>
- To: Christophe Lyon <christophe dot lyon at linaro dot org>, nick clifton <nickc at redhat dot com>, binutils at sourceware dot org
- Date: Fri, 20 Dec 2013 11:18:52 +0100
- Subject: Re: eh-frame: CIE initial_instructions overflow
- Authentication-results: sourceware.org; auth=none
- References: <CAKdteOa8FJo=zw33AJ_waUMeOWviTO9bU2GYbsESSSR9mPf76Q at mail dot gmail dot com> <52B2D925 dot 5090103 at redhat dot com> <CAKdteOa=pjCxo2HtT7vY6gY2qbv-V86UTOEzYp0MMgPekqbTxw at mail dot gmail dot com> <20131220054342 dot GC7521 at bubble dot grove dot modra dot org>
On 20 December 2013 06:43, Alan Modra <amodra@gmail.com> wrote:
> On Thu, Dec 19, 2013 at 04:46:32PM +0100, Christophe Lyon wrote:
>> On 19 December 2013 12:31, nick clifton <nickc@redhat.com> wrote:
>> > Hi Christophe,
>> >
>> >
>> >> Currently, in bfd/elf-eh-frame.c we have a definition of struct cie
>> >> which ends with:
>> >> unsigned char initial_instructions[50];
>> >
>> >
>> > This is a bug...
>> >
>> >
>> >> In _bfd_elf_parse_eh_frame(), we have:
>> >> initial_insn_length = end - buf;
>> >> if (initial_insn_length <= sizeof (cie->initial_instructions))
>> >> {
>> >> cie->initial_insn_length = initial_insn_length;
>> >> memcpy (cie->initial_instructions, buf, initial_insn_length);
>> >> }
>> >
>> >
>> > IMHO - there should be no fixed size for the initial_instructions buffer.
>> > Instead the code at this point should allocate and copy the buffer that has
>> > just been created.
>> >
>> That's my feeling too, but I asked because this is going to be a
>> rather intrusive change: the surrounding code allocates an array of
>> CIE after having counted them.
>> Having a dynamic size here means we have to parse all the CIEs once to
>> get the biggest size, then allocate local_cies accordingly, then
>> reparse the section contents.
>>
>> Not sure how costly it is to parse CIEs in general? Is there a risk to
>> increase link times significantly ?
>
> cie.initial_instructions[] is used when deciding whether CIEs can be
> merged. If CIEs with larger numbers of insns than we can handle are
> rare (and we might need to bump the size of cie.initial_instructions[]
> to make that true) then we can simply say that such CIEs can't be
> merged. I don't think I'd bother with variable size buffers.
> Something like the following ought to work.
>
> * elf-eh-frame.c (cie_eq): Return false when initial_insn_length
> is too large.
> (cie_compute_hash): Don't exceed bounds of initial_instructions.
> (_bfd_elf_parse_eh_frame): Always set initial_insn_length, and
> save as much of insns to initial_instructions[] as will fit.
>
Thanks for confirming my thoughts.
I have tried your patch and I can confirm it fixes the problem I was seeing.
Do you plan to commit it?
Thanks,
Christophe.