two patches for bugs in BFD/peXXigen.c
Kai Tietz
ktietz70@googlemail.com
Mon Sep 6 08:55:00 GMT 2010
2010/9/6 Alan Modra <amodra@gmail.com>:
> On Fri, Sep 03, 2010 at 01:24:34AM +0200, Marcus Brinkmann wrote:
>> Hi,
>>
>> while working on free software ports to Windows CE I noticed two bugs in
>> binutils' BFD support for some PE files (for example, kmail-mobile.exe built
>> with MSVC). Fixes for both are included below. Copyright assignments by g10
>> Code GmbH are on file at the FSF. If you need anything else, just let me
>> know. The file that triggers these bugs can be found at:
>>
>> ftp://ftp.g10code.com/people/marcus/kmail-mobile-binutils-test.exe.gz
>>
>> The first issue concerns import tables where the thunk table is found in a
>> different section. In this case, BFD tries to load DATASIZE bytes from that
>> section at the beginning of the thunk array, but DATASIZE is the remaining
>> bytes in the import table section starting from the beginning of the import
>> table. This number is in no way related to the size of the thunk table or the
>> section in which this thunk table is to be found. So, my patch introduces a
>> new size, limit_size, which is correctly calculated and used in the
>> appropriate places. Without the patch, no import symbols would be shown for
>> kmail-mobile.exe (Visual Studio 2008, Windows CE), because the data section is
>> in this case not large enough to read DATASIZE bytes from it. With the patch,
>> loading LIMIT_SIZE bytes succeeds and all import symbols are shown correctly.
>>
>> The second issue concerns the support for compressed pdata support for Windows
>> CE. In this code is a simple memory leak. First, the whole section is
>> malloced and copied to TDATA, then immediately TDATA is overwritten with a
>> much smaller buffer to which only the required section data is copied, leaking
>> memory in the size of the section for each entry in the table. For
>> kmail-mobile.exe, the table is very large (hundreds of entries), leaking
>> Gigabytes of memory quickly and basically creating denial of service attack.
> [snip]
>> 2010-09-03 Marcus Brinkmann <marcus@g10code.de>
>>
>> * peXXigen.c (pe_print_idata): Use correct size limit to load
>> thunk table from different section.
>>
> [snip]
>> 2010-09-03 Marcus Brinkmann <marcus@g10code.de>
>>
>> * peXXigen.c (_bfd_XX_print_ce_compressed_pdata): Fix memory leak.
>>
>
> Thank you for reporting these problems, but both of your patches
> contain errors. The first one results in
> peigen.c: In function ‘pe_print_idata’:
> peigen.c:1229: error: ‘limit_size’ may be used uninitialized in this function
> The second results in
> peigen.c: In function ‘_bfd_pe_print_ce_compressed_pdata’:
> peigen.c:1876: error: expected expression before ‘sym_cache’
>
> In your first patch I think this hunk, and the following one, is
> incorrect
> @@ -1285,7 +1288,7 @@ pe_print_idata (bfd * abfd, void * vfile
>
> /* Print HintName vector entries. */
> #ifdef COFF_WITH_pex64
> - for (j = 0; j < datasize; j += 8)
> + for (j = 0; j < limit_size; j += 8)
> {
> unsigned long member = bfd_get_32 (abfd, data + idx + j);
> unsigned long member_high = bfd_get_32 (abfd, data + idx + j + 4);
>
> Won't changing the loop endpoint possibly affect reading "member"?
>
> In the second patch, you still leave a potential memory leak if tdata
> is allocated but bfd_get_section_contents returns an error.
>
> The following is a revision of your patch with these problems fixed,
> but even though I'm a blanket write binutils maintainer I probably
> shouldn't be let loose in peXXigen.c! So I'll leave this for one of
> the PE/COFF maintainers to handle.
>
> Index: bfd/peXXigen.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/peXXigen.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 peXXigen.c
> --- bfd/peXXigen.c 27 Jun 2010 04:07:53 -0000 1.64
> +++ bfd/peXXigen.c 6 Sep 2010 04:39:44 -0000
> @@ -550,7 +550,7 @@ _bfd_XXi_swap_aouthdr_out (bfd * abfd, v
> PEAOUTHDR *aouthdr_out = (PEAOUTHDR *) out;
> bfd_vma sa, fa, ib;
> IMAGE_DATA_DIRECTORY idata2, idata5, tls;
> -
> +
> sa = extra->SectionAlignment;
> fa = extra->FileAlignment;
> ib = extra->ImageBase;
> @@ -558,7 +558,7 @@ _bfd_XXi_swap_aouthdr_out (bfd * abfd, v
> idata2 = pe->pe_opthdr.DataDirectory[PE_IMPORT_TABLE];
> idata5 = pe->pe_opthdr.DataDirectory[PE_IMPORT_ADDRESS_TABLE];
> tls = pe->pe_opthdr.DataDirectory[PE_TLS_TABLE];
> -
> +
> if (aouthdr_in->tsize)
> {
> aouthdr_in->text_start -= ib;
> @@ -615,7 +615,7 @@ _bfd_XXi_swap_aouthdr_out (bfd * abfd, v
> /* Until other .idata fixes are made (pending patch), the entry for
> .idata is needed for backwards compatibility. FIXME. */
> add_data_entry (abfd, extra, 1, ".idata", ib);
> -
> +
> /* For some reason, the virtual size (which is what's set by
> add_data_entry) for .reloc is not the same as the size recorded
> in this slot by MSVC; it doesn't seem to cause problems (so far),
> @@ -926,7 +926,7 @@ _bfd_XXi_swap_scnhdr_out (bfd * abfd, vo
> (0x02000000). Also, the resource data should also be read and
> writable. */
>
> - /* FIXME: Alignment is also encoded in this field, at least on PPC and
> + /* FIXME: Alignment is also encoded in this field, at least on PPC and
> ARM-WINCE. Although - how do we get the original alignment field
> back ? */
>
> @@ -936,7 +936,7 @@ _bfd_XXi_swap_scnhdr_out (bfd * abfd, vo
> unsigned long must_have;
> }
> pe_required_section_flags;
> -
> +
> pe_required_section_flags known_sections [] =
> {
> { ".arch", IMAGE_SCN_MEM_READ | IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_DISCARDABLE | IMAGE_SCN_ALIGN_8BYTES },
> @@ -1223,18 +1223,19 @@ pe_print_idata (bfd * abfd, void * vfile
> bfd_byte *ft_data;
> asection *ft_section;
> bfd_vma ft_addr;
> - bfd_size_type ft_datasize;
> + bfd_size_type ft_datasize = 0;
> int ft_idx;
> int ft_allocated = 0;
>
> fprintf (file, _("\tvma: Hint/Ord Member-Name Bound-To\n"));
>
> idx = hint_addr - adj;
> -
> +
> ft_addr = first_thunk + extra->ImageBase;
> ft_data = data;
> ft_idx = first_thunk - adj;
> - ft_allocated = 0;
> + ft_datasize = section->size - ft_idx;
> + ft_allocated = 0;
>
> if (first_thunk != hint_addr)
> {
> @@ -1242,12 +1243,9 @@ pe_print_idata (bfd * abfd, void * vfile
> for (ft_section = abfd->sections;
> ft_section != NULL;
> ft_section = ft_section->next)
> - {
> - ft_datasize = ft_section->size;
> - if (ft_addr >= ft_section->vma
> - && ft_addr < ft_section->vma + ft_datasize)
> - break;
> - }
> + if (ft_addr >= ft_section->vma
> + && ft_addr < ft_section->vma + ft_section->size)
> + break;
>
> if (ft_section == NULL)
> {
> @@ -1258,21 +1256,17 @@ pe_print_idata (bfd * abfd, void * vfile
>
> /* Now check to see if this section is the same as our current
> section. If it is not then we will have to load its data in. */
> - if (ft_section == section)
> - {
> - ft_data = data;
> - ft_idx = first_thunk - adj;
> - }
> - else
> + if (ft_section != section)
> {
> ft_idx = first_thunk - (ft_section->vma - extra->ImageBase);
> - ft_data = (bfd_byte *) bfd_malloc (datasize);
> + ft_datasize = ft_section->size - ft_idx;
> + ft_data = (bfd_byte *) bfd_malloc (ft_datasize);
> if (ft_data == NULL)
> continue;
>
> - /* Read datasize bfd_bytes starting at offset ft_idx. */
> - if (! bfd_get_section_contents
> - (abfd, ft_section, ft_data, (bfd_vma) ft_idx, datasize))
> + /* Read ft_datasize bytes starting at offset ft_idx. */
> + if (!bfd_get_section_contents (abfd, ft_section, ft_data,
> + (bfd_vma) ft_idx, ft_datasize))
> {
> free (ft_data);
> continue;
> @@ -1310,7 +1304,8 @@ pe_print_idata (bfd * abfd, void * vfile
> table holds actual addresses. */
> if (time_stamp != 0
> && first_thunk != 0
> - && first_thunk != hint_addr)
> + && first_thunk != hint_addr
> + && j + 4 <= ft_datasize)
> fprintf (file, "\t%04lx",
> (unsigned long) bfd_get_32 (abfd, ft_data + ft_idx + j));
> fprintf (file, "\n");
> @@ -1320,7 +1315,7 @@ pe_print_idata (bfd * abfd, void * vfile
> {
> unsigned long member = bfd_get_32 (abfd, data + idx + j);
>
> - /* Print single IMAGE_IMPORT_BY_NAME vector. */
> + /* Print single IMAGE_IMPORT_BY_NAME vector. */
> if (member == 0)
> break;
>
> @@ -1342,7 +1337,8 @@ pe_print_idata (bfd * abfd, void * vfile
> table holds actual addresses. */
> if (time_stamp != 0
> && first_thunk != 0
> - && first_thunk != hint_addr)
> + && first_thunk != hint_addr
> + && j + 4 <= ft_datasize)
> fprintf (file, "\t%04lx",
> (unsigned long) bfd_get_32 (abfd, ft_data + ft_idx + j));
>
> @@ -1583,7 +1579,7 @@ pe_print_edata (bfd * abfd, void * vfile
> /* This really is architecture dependent. On IA-64, a .pdata entry
> consists of three dwords containing relative virtual addresses that
> specify the start and end address of the code range the entry
> - covers and the address of the corresponding unwind info data.
> + covers and the address of the corresponding unwind info data.
>
> On ARM and SH-4, a compressed PDATA structure is used :
> _IMAGE_CE_RUNTIME_FUNCTION_ENTRY, whereas MIPS is documented to use
> @@ -1828,7 +1824,6 @@ _bfd_XX_print_ce_compressed_pdata (bfd *
> bfd_vma other_data;
> bfd_vma prolog_length, function_length;
> int flag32bit, exception_flag;
> - bfd_byte *tdata = 0;
> asection *tsection;
>
> if (i + PDATA_ROW_SIZE > stop)
> @@ -1860,12 +1855,14 @@ _bfd_XX_print_ce_compressed_pdata (bfd *
> if (tsection && coff_section_data (abfd, tsection)
> && pei_section_data (abfd, tsection))
> {
> - if (bfd_malloc_and_get_section (abfd, tsection, & tdata))
> - {
> - int xx = (begin_addr - 8) - tsection->vma;
> + int xx = (begin_addr - 8) - tsection->vma;
> + bfd_byte *tdata;
>
> - tdata = (bfd_byte *) bfd_malloc (8);
> - if (bfd_get_section_contents (abfd, tsection, tdata, (bfd_vma) xx, 8))
> + tdata = (bfd_byte *) bfd_malloc (8);
> + if (tdata)
> + {
> + if (bfd_get_section_contents (abfd, tsection,
> + tdata, (bfd_vma) xx, 8))
> {
> bfd_vma eh, eh_data;
>
> @@ -1883,11 +1880,6 @@ _bfd_XX_print_ce_compressed_pdata (bfd *
> }
> free (tdata);
> }
> - else
> - {
> - if (tdata)
> - free (tdata);
> - }
> }
>
> fprintf (file, "\n");
> @@ -2194,7 +2186,7 @@ _bfd_XX_bfd_copy_private_bfd_data_common
>
> ipe = pe_data (ibfd);
> ope = pe_data (obfd);
> -
> +
> /* pe_opthdr is copied in copy_object. */
> ope->dll = ipe->dll;
>
> @@ -2288,7 +2280,7 @@ _bfd_XXi_final_link_postscript (bfd * ab
> ".idata$2", FALSE, FALSE, TRUE);
> if (h1 != NULL)
> {
> - /* PR ld/2729: We cannot rely upon all the output sections having been
> + /* PR ld/2729: We cannot rely upon all the output sections having been
> created properly, so check before referencing them. Issue a warning
> message for any sections tht could not be found. */
> if ((h1->root.type == bfd_link_hash_defined
> @@ -2302,7 +2294,7 @@ _bfd_XXi_final_link_postscript (bfd * ab
> else
> {
> _bfd_error_handler
> - (_("%B: unable to fill in DataDictionary[1] because .idata$2 is missing"),
> + (_("%B: unable to fill in DataDictionary[1] because .idata$2 is missing"),
> abfd);
> result = FALSE;
> }
> @@ -2322,7 +2314,7 @@ _bfd_XXi_final_link_postscript (bfd * ab
> else
> {
> _bfd_error_handler
> - (_("%B: unable to fill in DataDictionary[1] because .idata$4 is missing"),
> + (_("%B: unable to fill in DataDictionary[1] because .idata$4 is missing"),
> abfd);
> result = FALSE;
> }
> @@ -2343,7 +2335,7 @@ _bfd_XXi_final_link_postscript (bfd * ab
> else
> {
> _bfd_error_handler
> - (_("%B: unable to fill in DataDictionary[12] because .idata$5 is missing"),
> + (_("%B: unable to fill in DataDictionary[12] because .idata$5 is missing"),
> abfd);
> result = FALSE;
> }
> @@ -2359,11 +2351,11 @@ _bfd_XXi_final_link_postscript (bfd * ab
> ((h1->root.u.def.value
> + h1->root.u.def.section->output_section->vma
> + h1->root.u.def.section->output_offset)
> - - pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_ADDRESS_TABLE].VirtualAddress);
> + - pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_ADDRESS_TABLE].VirtualAddress);
> else
> {
> _bfd_error_handler
> - (_("%B: unable to fill in DataDictionary[PE_IMPORT_ADDRESS_TABLE (12)] because .idata$6 is missing"),
> + (_("%B: unable to fill in DataDictionary[PE_IMPORT_ADDRESS_TABLE (12)] because .idata$6 is missing"),
> abfd);
> result = FALSE;
> }
> @@ -2385,7 +2377,7 @@ _bfd_XXi_final_link_postscript (bfd * ab
> else
> {
> _bfd_error_handler
> - (_("%B: unable to fill in DataDictionary[9] because __tls_used is missing"),
> + (_("%B: unable to fill in DataDictionary[9] because __tls_used is missing"),
> abfd);
> result = FALSE;
> }
>
> --
> Alan Modra
> Australia Development Lab, IBM
>
Hi,
I am no PE-COFF maintainer, but I have one annotation to this patch.
Would it be possible to get it without whitespace changes? 80% of it
are just whitespace changes about trailing whitespaces, which makes it
hard to see here real differences.
Regards,
Kai
--
| (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination
More information about the Binutils
mailing list