This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: eh-frame: CIE initial_instructions overflow


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]