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

["Marcus Brinkmann" <marcus dot brinkmann at ruhr-uni-bochum dot de>]

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

Index: peXXigen.c
===================================================================
RCS file: /cvs/src/src/bfd/peXXigen.c,v
retrieving revision 1.64
diff -p -u -r1.64 peXXigen.c
--- peXXigen.c	27 Jun 2010 04:07:53 -0000	1.64
+++ peXXigen.c	6 Sep 2010 11:16:00 -0000
@@ -1226,6 +1226,7 @@ pe_print_idata (bfd * abfd, void * vfile
 	  bfd_size_type ft_datasize;
 	  int ft_idx;
 	  int ft_allocated = 0;
+	  bfd_size_type limit_size;
 
 	  fprintf (file, _("\tvma:  Hint/Ord Member-Name Bound-To\n"));
 
@@ -1234,6 +1235,7 @@ pe_print_idata (bfd * abfd, void * vfile
 	  ft_addr = first_thunk + extra->ImageBase;
 	  ft_data = data;
 	  ft_idx = first_thunk - adj;
+	  limit_size = section->size - ft_idx;
 	  ft_allocated = 0; 
 
 	  if (first_thunk != hint_addr)
@@ -1262,17 +1264,19 @@ pe_print_idata (bfd * abfd, void * vfile
 		{
 		  ft_data = data;
 		  ft_idx = first_thunk - adj;
+		  limit_size = section->size - ft_idx;
 		}
 	      else
 		{
 		  ft_idx = first_thunk - (ft_section->vma - extra->ImageBase);
-		  ft_data = (bfd_byte *) bfd_malloc (datasize);
+		  limit_size = ft_section->size - ft_idx;
+		  ft_data = (bfd_byte *) bfd_malloc (limit_size);
 		  if (ft_data == NULL)
 		    continue;
 
-		  /* Read datasize bfd_bytes starting at offset ft_idx.  */
+		  /* Read limit_size bfd_bytes starting at offset ft_idx.  */
 		  if (! bfd_get_section_contents
-		      (abfd, ft_section, ft_data, (bfd_vma) ft_idx, datasize))
+		      (abfd, ft_section, ft_data, (bfd_vma) ft_idx, limit_size))
 		    {
 		      free (ft_data);
 		      continue;
@@ -1285,7 +1289,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);
@@ -1316,7 +1320,7 @@ pe_print_idata (bfd * abfd, void * vfile
 	      fprintf (file, "\n");
 	    }
 #else
-	  for (j = 0; j < datasize; j += 4)
+	  for (j = 0; j < limit_size; j += 4)
 	    {
 	      unsigned long member = bfd_get_32 (abfd, data + idx + j);
 

Index: peXXigen.c
===================================================================
RCS file: /cvs/src/src/bfd/peXXigen.c,v
retrieving revision 1.64
diff -u -p -r1.64 peXXigen.c
--- peXXigen.c	27 Jun 2010 04:07:53 -0000	1.64
+++ peXXigen.c	6 Sep 2010 11:19:38 -0000
@@ -1828,7 +1828,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,36 +1859,30 @@ _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 = 0;
 
-	      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 && bfd_get_section_contents (abfd, tsection, tdata, (bfd_vma) xx, 8))
+	    {
+	      bfd_vma eh, eh_data;
+	      
+	      eh = bfd_get_32 (abfd, tdata);
+	      eh_data = bfd_get_32 (abfd, tdata + 4);
+	      fprintf (file, "%08x  ", (unsigned int) eh);
+	      fprintf (file, "%08x", (unsigned int) eh_data);
+	      if (eh != 0)
 		{
-		  bfd_vma eh, eh_data;
-
-		  eh = bfd_get_32 (abfd, tdata);
-		  eh_data = bfd_get_32 (abfd, tdata + 4);
-		  fprintf (file, "%08x  ", (unsigned int) eh);
-		  fprintf (file, "%08x", (unsigned int) eh_data);
-		  if (eh != 0)
-		    {
-		      const char *s = my_symbol_for_address (abfd, eh, &cache);
-
-		      if (s)
-			fprintf (file, " (%s) ", s);
-		    }
+		  const char *s = my_symbol_for_address (abfd, eh, &cache);
+		  
+		  if (s)
+		    fprintf (file, " (%s) ", s);
 		}
-	      free (tdata);
-	    }
-	  else
-	    {
-	      if (tdata)
-		free (tdata);
 	    }
+	  if (tdata)
+	    free (tdata);
 	}
-
+      
       fprintf (file, "\n");
     }