Re: two patches for bugs in BFD/peXXigen.c

[Kai Tietz <ktietz70 at googlemail dot com>]

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