RFA: Your patch from 2004-06-29 (bfd/linker.c rev 1.37) [was RE: COMMON symbols...]

["Dave Korn" <dave dot korn at artimi dot com>]

----Original Message----
>From: Dave Korn
>Sent: 01 April 2005 15:10


    Hi Alan,

  You may or may not have noticed this exchange between Nick and me last
week:
(http://sourceware.org/ml/binutils/2005-04/msg00025.html and follow-ups)


> ----Original Message----
>> From: Nick Clifton
>> Sent: 01 April 2005 14:41
> 
>> Hi Dave,
>> 
>>>   So I think that ld isn't zero-filling the space it allocates to the
>>> common symbols, but just letting them take whatever values happen to be
>>> in the host memory space that has been allocated by the linker to build
>>> up the output section.
> 
>> I strongly suspect that this is a linker "feature" in that it has never
>> had to zero-fill the file space that is going to be used for common
>> symbols because it assumes that they are always going to be going into a
>>   section with the SHT_NOBITS flag set.  ie this is a bug and the linker
>> ought to check for this and zero the memory it writes out if necessary.
> 
>   Right, it's no great surprise that it hasn't been tried a whole lot
> before, although I'm surprised it works for .bss but not .common - they
> both have SHT_NOBITS, don't they?  I wondered perhaps if there's some
> special-cased handling of the COMMON (pseudo-)section.  I'll take a closer
> look.


  Well, when I looked into it, it turned out to be because bfd_section_size
(...) returns zero for the COMMON section.  That was causing
default_indirect_link_order (...) to allocate zero space to read the input
section contents into, but then since link_order->size was non-zero, when it
came to call bfd_set_section_contents (...) to write the output, with a
non-zero size, it was reading past the end of the (zero-sized) allocation
and filling the output with junk contents from the host heap memory.

  This looks like it's already been addressed in mainline, as part of your
section-size-related cleanups from last June, but you took a somewhat
different approach to the way I fixed it, so I thought I'd discuss it with
you and see what you think:

(refs:
http://sourceware.org/ml/binutils-cvs/2004-06/msg00118.html
http://sourceware.org/ml/binutils/2004-06/msg00396.html
http://sourceware.org/ml/binutils/2004-06/msg00564.html
)

------------------snip------------------
 Revision 1.37 / (download) - annotate - [select for diffs] , Tue Jun 29
14:13:44 2004 UTC (9 months ago) by amodra
Branch: MAIN 
------------------snip------------------
bfd/
	* elf64-mmix.c (mmix_set_relaxable_size): Save original size in
	rawsize.
	(mmix_elf_perform_relocation): Adjust for above change.
	(mmix_elf_relocate_section): Likewise.
	(mmix_elf_relax_section): Likewise.  Use output_section->rawsize.
	(mmix_elf_get_section_contents): Delete.
	(bfd_elf64_get_section_contents): Delete.
	(mmix_elf_relocate_section): Zero stub area.
	* linker.c (default_indirect_link_order): Alloc max of section size
	and rawsize.
	* simple.c (bfd_simple_get_relocated_section_contents): Likewise.
	* section.c (bfd_malloc_and_get_section): Likewise.
	(struct bfd_section): Update rawsize comment.
	* bfd-in2.h: Regenerate.
ld/
	* ldlang.c (lang_reset_memory_regions): Save last relax pass section
	size in rawsize.
------------------snip------------------

  Anyway, here's the hunk where you patched linker.c:

===================================================================
RCS file: /cvs/src/src/bfd/linker.c,v
retrieving revision 1.36
retrieving revision 1.37
diff -u -r1.36 -r1.37
--- src/bfd/linker.c	2004/06/24 04:46:24	1.36
+++ src/bfd/linker.c	2004/06/29 14:13:44	1.37
@@ -2756,7 +2756,9 @@
     }
 
   /* Get and relocate the section contents.  */
-  sec_size = input_section->size;
+  sec_size = (input_section->rawsize > input_section->size
+	      ? input_section->rawsize
+	      : input_section->size);
   contents = bfd_malloc (sec_size);
   if (contents == NULL && sec_size != 0)
     goto error_return;
------------------snip------------------

... and here's what I did, since I'm running from somewhat older sources
(2.13.90 20030308, LOL!) ...

===================================================================
RCS file: /cvsroot/tools/binutils/bfd/linker.c,v
retrieving revision 1.1
retrieving revision 1.2
diff -u -p -r1.1 -r1.2
--- tools/binutils/bfd/linker.c	2004/05/20 16:21:10	1.1
+++ tools/binutils/bfd/linker.c	2005/04/04 10:33:24	1.2
@@ -2838,7 +2838,23 @@ default_indirect_link_order (output_bfd,
 
   /* Get and relocate the section contents.  */
   sec_size = bfd_section_size (input_bfd, input_section);
-  contents = ((bfd_byte *) bfd_malloc (sec_size));
+  /*  It is possible that link_order->size (the output size) may be greater
+  than sec_size (the size of the input section), because (for example) the
+  COMMON section has a size of zero.  Since we need to write the full
+  link_order->size number of bytes into the output bfd, we must allocate
+  enough memory for the output data, zero padding the end if the input is
+  smaller, otherwise the call to bfd_set_section_contents below will read
+  past the end of the allocated memory and fill the output with junk. */
+  if (sec_size < link_order->size)
+  {
+    contents = (bfd_byte *) bfd_malloc (link_order->size);
+    if (contents != NULL)
+      memset (contents + sec_size, 0, link_order->size - sec_size);
+  }
+  else
+  {
+    contents = (bfd_byte *) bfd_malloc (sec_size);
+  }
   if (contents == NULL && sec_size != 0)
     goto error_return;
   new_contents = (bfd_get_relocated_section_contents
------------------snip------------------


  Anyway, the things that I wondered about are:

1)  You use the maximum of the input section's raw size and cooked size.
Are these values still zero for the COMMON section?  Or does
bfd_get_relocated_section_contents now return a real block of zeroed memory
when we try it with the COMMON section?

2)  The subsequent call to bfd_set_section_contents uses link_order->offset
and link_order->size, are we always 100% guaranteed that this will never
exceed the sec_size calculated for the input section?

3)  bfd_malloc doesn't zero the allocated memory; in your patch, if the raw
size is less than the nominal cooked size, shouldn't the end of the
allocation (the difference between the size we allocate and the amount we
actually read in with bfd_get_relocated_section_contents) be cleared to
zero? 


    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....