Patch to detect invalid mergeable string sections

Patch to detect invalid mergeable string sections

[Joseph S. Myers]

GCC versions before 4.1.2 could generate object files with invalid
mergeable string sections containing unterminated strings.  This was
fixed by:

2006-06-21  Jakub Jelinek  <jakub@redhat.com>

        * varasm.c (mergeable_string_section): Check for embedded NULs and
        NUL termination in the first int_size_in_bytes (TREE_TYPE (decl))
        rather than TREE_STRING_LENGTH bytes.

When linking such object files, the linker can give internal errors
from _bfd_merged_section_offset (or possibly fail in other ways;
looking for the end of such a string will run off the end of the
section and start reading uninitialized memory).  Invalid input should
never cause internal errors from the linker; it should give normal
non-internal errors to the user diagnosing the invalid input instead.

This patch adds a check that the strings found in such sections in
input files do not run off the end of their sections.  A failure at
this point in turn requires two other checks for NULL secinfo to be
inserted so later stages in the attempted merging don't dereference a
NULL pointer.  Tested on i686-pc-linux-gnu (native).  OK to commit?

bfd:
2007-10-30  Joseph Myers  <joseph@codesourcery.com>

	* merge.c (sec_merge_hash_lookup): Add parameter sec_end.  Check
	for unterminated strings.  All callers changed.
	(_bfd_write_merged_section, _bfd_merged_section_offset): Handle
	NULL secinfo from merge failures.

ld/testsuite:
2007-10-30  Joseph Myers  <joseph@codesourcery.com>

	* ld-elf/merge3.d, ld-elf/merge3.s: New.

Index: bfd/merge.c
===================================================================
RCS file: /cvs/src/src/bfd/merge.c,v
retrieving revision 1.33
diff -u -r1.33 merge.c
--- bfd/merge.c	19 Sep 2007 12:08:34 -0000	1.33
+++ bfd/merge.c	30 Oct 2007 16:00:19 -0000
@@ -133,6 +133,7 @@
 
 static struct sec_merge_hash_entry *
 sec_merge_hash_lookup (struct sec_merge_hash *table, const char *string,
+		       const unsigned char *sec_end,
 		       unsigned int alignment, bfd_boolean create)
 {
   register const unsigned char *s;
@@ -154,6 +155,12 @@
 	      hash += c + (c << 17);
 	      hash ^= hash >> 2;
 	      ++len;
+	      if (sec_end && s >= sec_end)
+		{
+		  (*_bfd_error_handler)
+		    (_("unterminated string in section marked for merging"));
+		  return NULL;
+		}
 	    }
 	  hash += len + (len << 17);
 	}
@@ -161,6 +168,12 @@
 	{
 	  for (;;)
 	    {
+	      if (sec_end && s + table->entsize > sec_end)
+		{
+		  (*_bfd_error_handler)
+		    (_("unterminated string in section marked for merging"));
+		  return NULL;
+		}
 	      for (i = 0; i < table->entsize; ++i)
 		if (s[i] != '\0')
 		  break;
@@ -264,7 +277,9 @@
 {
   register struct sec_merge_hash_entry *entry;
 
-  entry = sec_merge_hash_lookup (tab, str, alignment, TRUE);
+  entry = sec_merge_hash_lookup (tab, str,
+				 secinfo->contents + secinfo->sec->size,
+				 alignment, TRUE);
   if (entry == NULL)
     return NULL;
 
@@ -779,6 +794,9 @@
 
   secinfo = (struct sec_merge_sec_info *) psecinfo;
 
+  if (!secinfo)
+    return FALSE;
+
   if (secinfo->first_str == NULL)
     return TRUE;
 
@@ -807,6 +825,9 @@
 
   secinfo = (struct sec_merge_sec_info *) psecinfo;
 
+  if (!secinfo)
+    return 0;
+
   if (offset >= sec->rawsize)
     {
       if (offset > sec->rawsize)
@@ -849,7 +870,7 @@
     {
       p = secinfo->contents + (offset / sec->entsize) * sec->entsize;
     }
-  entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, 0, FALSE);
+  entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, NULL, 0, FALSE);
   if (!entry)
     {
       if (! secinfo->htab->strings)
Index: ld/testsuite/ld-elf/merge3.d
===================================================================
RCS file: ld/testsuite/ld-elf/merge3.d
diff -N ld/testsuite/ld-elf/merge3.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-elf/merge3.d	30 Oct 2007 16:00:20 -0000
@@ -0,0 +1,3 @@
+#source: merge3.s
+#ld: -T merge.ld
+#error: unterminated string in section marked for merging
Index: ld/testsuite/ld-elf/merge3.s
===================================================================
RCS file: ld/testsuite/ld-elf/merge3.s
diff -N ld/testsuite/ld-elf/merge3.s
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-elf/merge3.s	30 Oct 2007 16:00:20 -0000
@@ -0,0 +1,7 @@
+	.section .rodata.str,"aMS","progbits",1
+.LC0:	
+	.ascii	"abcd"
+	.text
+	.global _start
+_start:	
+	.long	.LC0

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Patch to detect invalid mergeable string sections

[Alan Modra]

On Tue, Oct 30, 2007 at 04:10:18PM +0000, Joseph S. Myers wrote:
> +	      if (sec_end && s >= sec_end)
> +		{
> +		  (*_bfd_error_handler)
> +		    (_("unterminated string in section marked for merging"));
> +		  return NULL;
> +		}

It would be nice if this error specified the section.  I think you can
do that by simply returning NULL here and calling _bfd_error_handler
at error_return in record_section after testing for bfd_get_error
returning anything but bfd_error_no_memory.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Patch to detect invalid mergeable string sections

[Joseph S. Myers]

On Wed, 31 Oct 2007, Alan Modra wrote:

> On Tue, Oct 30, 2007 at 04:10:18PM +0000, Joseph S. Myers wrote:
> > +	      if (sec_end && s >= sec_end)
> > +		{
> > +		  (*_bfd_error_handler)
> > +		    (_("unterminated string in section marked for merging"));
> > +		  return NULL;
> > +		}
> 
> It would be nice if this error specified the section.  I think you can
> do that by simply returning NULL here and calling _bfd_error_handler
> at error_return in record_section after testing for bfd_get_error
> returning anything but bfd_error_no_memory.

How about this revised patch which does that?

bfd:
2007-10-31  Joseph Myers  <joseph@codesourcery.com>

	* merge.c (sec_merge_hash_lookup): Add parameter sec_end.  Check
	for unterminated strings.  All callers changed.
	(record_section): Add parameter abfd.  Give error message for
	unterminated strings.
	(_bfd_merge_sections): Update call to record_section.
	(_bfd_write_merged_section, _bfd_merged_section_offset): Handle
	NULL secinfo from merge failures.

ld/testsuite:
2007-10-31  Joseph Myers  <joseph@codesourcery.com>

	* ld-elf/merge3.d, ld-elf/merge3.s: New.

Index: bfd/merge.c
===================================================================
RCS file: /cvs/src/src/bfd/merge.c,v
retrieving revision 1.33
diff -u -p -r1.33 merge.c
--- bfd/merge.c	19 Sep 2007 12:08:34 -0000	1.33
+++ bfd/merge.c	31 Oct 2007 17:58:02 -0000
@@ -133,6 +133,7 @@ sec_merge_hash_newfunc (struct bfd_hash_
 
 static struct sec_merge_hash_entry *
 sec_merge_hash_lookup (struct sec_merge_hash *table, const char *string,
+		       const unsigned char *sec_end,
 		       unsigned int alignment, bfd_boolean create)
 {
   register const unsigned char *s;
@@ -154,6 +155,8 @@ sec_merge_hash_lookup (struct sec_merge_
 	      hash += c + (c << 17);
 	      hash ^= hash >> 2;
 	      ++len;
+	      if (sec_end && s >= sec_end)
+		return NULL;
 	    }
 	  hash += len + (len << 17);
 	}
@@ -161,6 +164,8 @@ sec_merge_hash_lookup (struct sec_merge_
 	{
 	  for (;;)
 	    {
+	      if (sec_end && s + table->entsize > sec_end)
+		return NULL;
 	      for (i = 0; i < table->entsize; ++i)
 		if (s[i] != '\0')
 		  break;
@@ -264,7 +269,9 @@ sec_merge_add (struct sec_merge_hash *ta
 {
   register struct sec_merge_hash_entry *entry;
 
-  entry = sec_merge_hash_lookup (tab, str, alignment, TRUE);
+  entry = sec_merge_hash_lookup (tab, str,
+				 secinfo->contents + secinfo->sec->size,
+				 alignment, TRUE);
   if (entry == NULL)
     return NULL;
 
@@ -436,7 +443,8 @@ _bfd_add_merge_section (bfd *abfd, void 
 
 /* Record one section into the hash table.  */
 static bfd_boolean
-record_section (struct sec_merge_info *sinfo,
+record_section (bfd *abfd,
+		struct sec_merge_info *sinfo,
 		struct sec_merge_sec_info *secinfo)
 {
   asection *sec = secinfo->sec;
@@ -513,6 +521,10 @@ record_section (struct sec_merge_info *s
   return TRUE;
 
 error_return:
+  if (bfd_get_error () != bfd_error_no_memory)
+    (*_bfd_error_handler)
+      (_("%B: unterminated string in section `%A' marked for merging"),
+       abfd, sec);
   for (secinfo = sinfo->chain; secinfo; secinfo = secinfo->next)
     *secinfo->psecinfo = NULL;
   return FALSE;
@@ -695,7 +707,7 @@ alloc_failure:
    with _bfd_merge_section.  */
 
 bfd_boolean
-_bfd_merge_sections (bfd *abfd ATTRIBUTE_UNUSED,
+_bfd_merge_sections (bfd *abfd,
 		     struct bfd_link_info *info ATTRIBUTE_UNUSED,
 		     void *xsinfo,
 		     void (*remove_hook) (bfd *, asection *))
@@ -722,7 +734,7 @@ _bfd_merge_sections (bfd *abfd ATTRIBUTE
 	    if (remove_hook)
 	      (*remove_hook) (abfd, secinfo->sec);
 	  }
-	else if (! record_section (sinfo, secinfo))
+	else if (! record_section (abfd, sinfo, secinfo))
 	  break;
 
       if (secinfo)
@@ -779,6 +791,9 @@ _bfd_write_merged_section (bfd *output_b
 
   secinfo = (struct sec_merge_sec_info *) psecinfo;
 
+  if (!secinfo)
+    return FALSE;
+
   if (secinfo->first_str == NULL)
     return TRUE;
 
@@ -807,6 +822,9 @@ _bfd_merged_section_offset (bfd *output_
 
   secinfo = (struct sec_merge_sec_info *) psecinfo;
 
+  if (!secinfo)
+    return 0;
+
   if (offset >= sec->rawsize)
     {
       if (offset > sec->rawsize)
@@ -849,7 +867,7 @@ _bfd_merged_section_offset (bfd *output_
     {
       p = secinfo->contents + (offset / sec->entsize) * sec->entsize;
     }
-  entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, 0, FALSE);
+  entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, NULL, 0, FALSE);
   if (!entry)
     {
       if (! secinfo->htab->strings)
Index: ld/testsuite/ld-elf/merge3.d
===================================================================
RCS file: ld/testsuite/ld-elf/merge3.d
diff -N ld/testsuite/ld-elf/merge3.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-elf/merge3.d	31 Oct 2007 17:58:03 -0000
@@ -0,0 +1,3 @@
+#source: merge3.s
+#ld: -T merge.ld
+#error: unterminated string in section `.rodata.str' marked for merging
Index: ld/testsuite/ld-elf/merge3.s
===================================================================
RCS file: ld/testsuite/ld-elf/merge3.s
diff -N ld/testsuite/ld-elf/merge3.s
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-elf/merge3.s	31 Oct 2007 17:58:03 -0000
@@ -0,0 +1,7 @@
+	.section .rodata.str,"aMS","progbits",1
+.LC0:	
+	.ascii	"abcd"
+	.text
+	.global _start
+_start:	
+	.long	.LC0

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Patch to detect invalid mergeable string sections

[Alan Modra]

On Wed, Oct 31, 2007 at 06:07:56PM +0000, Joseph S. Myers wrote:
> bfd:
> 2007-10-31  Joseph Myers  <joseph@codesourcery.com>
> 
> 	* merge.c (sec_merge_hash_lookup): Add parameter sec_end.  Check
> 	for unterminated strings.  All callers changed.
> 	(record_section): Add parameter abfd.  Give error message for
> 	unterminated strings.
> 	(_bfd_merge_sections): Update call to record_section.
> 	(_bfd_write_merged_section, _bfd_merged_section_offset): Handle
> 	NULL secinfo from merge failures.
> 
> ld/testsuite:
> 2007-10-31  Joseph Myers  <joseph@codesourcery.com>
> 
> 	* ld-elf/merge3.d, ld-elf/merge3.s: New.

OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Patch to detect invalid mergeable string sections

[Alan Modra]

Joseph, I'm reverting most of your patch and instead providing the
missing zero terminator at the end of a string merge section.  (My
SLES 10 box system compiler happens to be a 4.1.2 prerelease without
Jakub's fix and I hit your error message on the ld bootstrap tests..)

Since versions of gcc with the bug are widely used, I think ld should
fix the problem, especially since the fix is easy.  From Jakub's
description of the problem I'm fairly certain that ld can produce a
valid program.

Apologies for not picking this up earlier when I reviewed your patch.

bfd/
	* merge.c (sec_merge_hash_lookup): Revert last change.
	(record_section): Likewise.
	(_bfd_merge_sections): Likewise.
	(_bfd_merged_section_offset): Properly handle NULL secinfo.
	(_bfd_add_merge_section): Allocate extra space for a zero
	terminator on SEC_STRINGS sections.
ld/testsuite/
	* ld-elf/merge3.d, ld-elf/merge3.s: Delete.

Index: bfd/merge.c
===================================================================
RCS file: /cvs/src/src/bfd/merge.c,v
retrieving revision 1.34
diff -u -p -r1.34 merge.c
--- bfd/merge.c	1 Nov 2007 11:45:20 -0000	1.34
+++ bfd/merge.c	5 Nov 2007 02:07:05 -0000
@@ -133,7 +133,6 @@ sec_merge_hash_newfunc (struct bfd_hash_
 
 static struct sec_merge_hash_entry *
 sec_merge_hash_lookup (struct sec_merge_hash *table, const char *string,
-		       const unsigned char *sec_end,
 		       unsigned int alignment, bfd_boolean create)
 {
   register const unsigned char *s;
@@ -155,8 +154,6 @@ sec_merge_hash_lookup (struct sec_merge_
 	      hash += c + (c << 17);
 	      hash ^= hash >> 2;
 	      ++len;
-	      if (sec_end && s >= sec_end)
-		return NULL;
 	    }
 	  hash += len + (len << 17);
 	}
@@ -164,8 +161,6 @@ sec_merge_hash_lookup (struct sec_merge_
 	{
 	  for (;;)
 	    {
-	      if (sec_end && s + table->entsize > sec_end)
-		return NULL;
 	      for (i = 0; i < table->entsize; ++i)
 		if (s[i] != '\0')
 		  break;
@@ -269,9 +264,7 @@ sec_merge_add (struct sec_merge_hash *ta
 {
   register struct sec_merge_hash_entry *entry;
 
-  entry = sec_merge_hash_lookup (tab, str,
-				 secinfo->contents + secinfo->sec->size,
-				 alignment, TRUE);
+  entry = sec_merge_hash_lookup (tab, str, alignment, TRUE);
   if (entry == NULL)
     return NULL;
 
@@ -410,7 +403,12 @@ _bfd_add_merge_section (bfd *abfd, void 
 
   /* Read the section from abfd.  */
 
-  amt = sizeof (struct sec_merge_sec_info) + sec->size - 1;
+  amt = sizeof (struct sec_merge_sec_info) - 1 + sec->size;
+  if (sec->flags & SEC_STRINGS)
+    /* Some versions of gcc may emit a string without a zero terminator.
+       See http://gcc.gnu.org/ml/gcc-patches/2006-06/msg01004.html
+       Allocate space for an extra zero.  */
+    amt += sec->entsize;
   *psecinfo = bfd_alloc (abfd, amt);
   if (*psecinfo == NULL)
     goto error_return;
@@ -430,6 +428,8 @@ _bfd_add_merge_section (bfd *abfd, void 
   secinfo->first_str = NULL;
 
   sec->rawsize = sec->size;
+  if (sec->flags & SEC_STRINGS)
+    memset (secinfo->contents + sec->size, 0, sec->entsize);
   if (! bfd_get_section_contents (sec->owner, sec, secinfo->contents,
 				  0, sec->size))
     goto error_return;
@@ -443,8 +443,7 @@ _bfd_add_merge_section (bfd *abfd, void 
 
 /* Record one section into the hash table.  */
 static bfd_boolean
-record_section (bfd *abfd,
-		struct sec_merge_info *sinfo,
+record_section (struct sec_merge_info *sinfo,
 		struct sec_merge_sec_info *secinfo)
 {
   asection *sec = secinfo->sec;
@@ -521,10 +520,6 @@ record_section (bfd *abfd,
   return TRUE;
 
 error_return:
-  if (bfd_get_error () != bfd_error_no_memory)
-    (*_bfd_error_handler)
-      (_("%B: unterminated string in section `%A' marked for merging"),
-       abfd, sec);
   for (secinfo = sinfo->chain; secinfo; secinfo = secinfo->next)
     *secinfo->psecinfo = NULL;
   return FALSE;
@@ -734,7 +729,7 @@ _bfd_merge_sections (bfd *abfd,
 	    if (remove_hook)
 	      (*remove_hook) (abfd, secinfo->sec);
 	  }
-	else if (! record_section (abfd, sinfo, secinfo))
+	else if (! record_section (sinfo, secinfo))
 	  break;
 
       if (secinfo)
@@ -823,7 +818,7 @@ _bfd_merged_section_offset (bfd *output_
   secinfo = (struct sec_merge_sec_info *) psecinfo;
 
   if (!secinfo)
-    return 0;
+    return offset;
 
   if (offset >= sec->rawsize)
     {
@@ -867,7 +862,7 @@ _bfd_merged_section_offset (bfd *output_
     {
       p = secinfo->contents + (offset / sec->entsize) * sec->entsize;
     }
-  entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, NULL, 0, FALSE);
+  entry = sec_merge_hash_lookup (secinfo->htab, (char *) p, 0, FALSE);
   if (!entry)
     {
       if (! secinfo->htab->strings)

-- 
Alan Modra
Australia Development Lab, IBM

Re: Patch to detect invalid mergeable string sections

[Joseph S. Myers]

On Mon, 5 Nov 2007, Alan Modra wrote:

> ld/testsuite/
> 	* ld-elf/merge3.d, ld-elf/merge3.s: Delete.

It seems to me tests are still needed to make sure the linker doesn't 
crash (or indeed to verify that the unterminated strings now get properly 
merged).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Patch to detect invalid mergeable string sections

[Alan Modra]

On Mon, Nov 05, 2007 at 02:31:18AM +0000, Joseph S. Myers wrote:
> On Mon, 5 Nov 2007, Alan Modra wrote:
> 
> > ld/testsuite/
> > 	* ld-elf/merge3.d, ld-elf/merge3.s: Delete.
> 
> It seems to me tests are still needed to make sure the linker doesn't 
> crash (or indeed to verify that the unterminated strings now get properly 
> merged).

That could be quite challenging.  A meaningful test should fail on a
20071031 ld and pass with today's ld.  I think you'd need to replace
ld's memory allocation functions with ones that filled memory with
some non-zero pattern, or run ld under valgrind or somesuch to
reliably detect an error caused by buffer overrun.

-- 
Alan Modra
Australia Development Lab, IBM