two patches for bugs in BFD/peXXigen.c

[Marcus Brinkmann]

On 09/06/2010 06:53 AM, Alan Modra wrote:
> On Fri, Sep 03, 2010 at 01:24:34AM +0200, Marcus Brinkmann wrote:
>> 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.

I'm embarrassed!

> 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

limit_size should be initialized to section->size - ft_idx.  Note that the
code as it is written now contains redundant initialization in the first_thunk
!= hint_addr && section == ft_section case.  I initialized limit_size in the
second of these redundant initializations, but that has narrower scope than
the first.  So my defense is that the code was confusing in the first case :)
 (The whole if (section == ft_section) branch in that code path can be
eliminated).

> The second results in
> peigen.c: In function ‘_bfd_pe_print_ce_compressed_pdata’:
> peigen.c:1876: error: expected expression before ‘sym_cache’

I got confused with the version drift between cegcc and binutils, one uses
cache and one uses sym_cache, and it seems I made some manual adjustments to
the patch :(.

> 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"?

That's the whole point of the change.  datasize is not a valid limit for the
thunk table, at all.  Actually, in valid PE files, all these tables are NULL
terminated.  But, all loops in binutils protect against invalid files and
never read beyond the current section.  However, datasize is the number of
bytes in the .idata section following the base of the import table.  It is NOT
number of bytes in the section that contains the thunk table, following the
base of that table. They don't even need to be related to the same section.
The code that I patched deals with the case in which the thunk table is not
contained in the .idata section.  For this, datasize is clearly wrong.  But
even in the case where section == ft_section, datasize is relative to the
beginning of the import table, and not relative to the beginning of the thunk
table, thus the original loop terminated wrongly.  I guess it just worked by
plain chance because usually the thunks follow the import table and then
limit_size < datasize and the loop terminates in valid PE files due to the
NULL terminator in the table itself.

> In the second patch, you still leave a potential memory leak if tdata
> is allocated but bfd_get_section_contents returns an error.

Sorry about that.

Here are my patches again with the minimum changes to fix these problems.

Thanks,
Marcus

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01-idata-r2.patch
Type: text/x-diff
Size: 2185 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20100906/36bf5c2c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02-pdata-r2.patch
Type: text/x-diff
Size: 2085 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20100906/36bf5c2c/attachment-0001.bin>