[PATCH][GOLD] Avoid linker crashing when merge sections have uneven sizes.

[PATCH][GOLD] Avoid linker crashing when merge sections have uneven sizes.

[Doug Kwan (關振德)]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

Hi Ian,

    This patch fixes a problem exposed by bootstrapping gcc.  libjava
contains objects with unevenly sized merge sections, whose sections
sizes are not multiples of their entsizes.
Output_merge_data::do_add_input_section cannot handle these sections
and returns false but the result is ignored by
Output_section::add_merge_input_section, which updates the merge
section map incorrectly.  Unevenly sized merge sections also cause
another problem Output_merge_data::set_final_data_size because the
method assumes the data member Output_merge_data::p_ is always not
NULL.  However, that condition is possible is an Output_merge_data is
empty due to unevenly sized input sections.

-Doug

2010-04-08  Doug Kwan  <dougkwan@google.com>

        * merge.cc (Output_merge_data::set_final_data_size): Handle empty
        Output_merge_data.
        * output.cc (Output_section::add_merge_input_section): Simplify
        code and return status of Output_merge_base::add_input_section.
        Update merge section map only if Output_merge_base::add_input_section
        returns true.

[-- Attachment #2: patch-merge.txt --]
[-- Type: text/plain, Size: 4291 bytes --]

Index: gold/merge.cc
===================================================================
RCS file: /cvs/src/src/gold/merge.cc,v
retrieving revision 1.33
diff -u -u -p -r1.33 merge.cc
--- gold/merge.cc	14 Dec 2009 19:53:05 -0000	1.33
+++ gold/merge.cc	8 Apr 2010 23:28:20 -0000
@@ -425,7 +425,10 @@ Output_merge_data::set_final_data_size()
 {
   // Release the memory we don't need.
   this->p_ = static_cast<unsigned char*>(realloc(this->p_, this->len_));
-  gold_assert(this->p_ != NULL);
+  // An Output_merge_data object may be empty and realloc is allowed
+  // to return a NULL pointer in this case.  An Output_merge_data is empty
+  // if all its input sections have sizes that are not multiples of entsize.
+  gold_assert(this->p_ != NULL || this->len_ == 0);
   this->set_data_size(this->len_);
 }
 
Index: gold/output.cc
===================================================================
RCS file: /cvs/src/src/gold/output.cc,v
retrieving revision 1.122
diff -u -u -p -r1.122 output.cc
--- gold/output.cc	6 Mar 2010 02:34:13 -0000	1.122
+++ gold/output.cc	8 Apr 2010 23:28:20 -0000
@@ -2154,58 +2154,55 @@ Output_section::add_merge_input_section(
   gold_assert(this->checkpoint_ == NULL);
 
   // Look up merge sections by required properties.
+  Output_merge_base* pomb;
   Merge_section_properties msp(is_string, entsize, addralign);
   Merge_section_by_properties_map::const_iterator p =
     this->merge_section_by_properties_map_.find(msp);
   if (p != this->merge_section_by_properties_map_.end())
     {
-      Output_merge_base* merge_section = p->second;
-      merge_section->add_input_section(object, shndx);
-      gold_assert(merge_section->is_string() == is_string
-		  && merge_section->entsize() == entsize
-		  && merge_section->addralign() == addralign);
-
-      // Link input section to found merge section.
-      Const_section_id csid(object, shndx);
-      this->merge_section_map_[csid] = merge_section;
-      return true;
+      pomb = p->second;
+      gold_assert(pomb->is_string() == is_string
+		  && pomb->entsize() == entsize
+		  && pomb->addralign() == addralign);
     }
-
-  // We handle the actual constant merging in Output_merge_data or
-  // Output_merge_string_data.
-  Output_merge_base* pomb;
-  if (!is_string)
-    pomb = new Output_merge_data(entsize, addralign);
   else
     {
-      switch (entsize)
+      // Create a new Output_merge_data or Output_merge_string_data.
+      if (!is_string)
+	pomb = new Output_merge_data(entsize, addralign);
+      else
 	{
-        case 1:
-	  pomb = new Output_merge_string<char>(addralign);
-	  break;
-        case 2:
-	  pomb = new Output_merge_string<uint16_t>(addralign);
-	  break;
-        case 4:
-	  pomb = new Output_merge_string<uint32_t>(addralign);
-	  break;
-        default:
-	  return false;
+	  switch (entsize)
+	    {
+	    case 1:
+	      pomb = new Output_merge_string<char>(addralign);
+	      break;
+	    case 2:
+	      pomb = new Output_merge_string<uint16_t>(addralign);
+	      break;
+	    case 4:
+	      pomb = new Output_merge_string<uint32_t>(addralign);
+	      break;
+	    default:
+	      return false;
+	    }
 	}
+      // Add new merge section to this output section and link merge
+      // section properties to new merge section in map.
+      this->add_output_merge_section(pomb, is_string, entsize);
+      this->merge_section_by_properties_map_[msp] = pomb;
     }
 
-  // Add new merge section to this output section and link merge section
-  // properties to new merge section in map.
-  this->add_output_merge_section(pomb, is_string, entsize);
-  this->merge_section_by_properties_map_[msp] = pomb;
-
-  // Add input section to new merge section and link input section to new
-  // merge section in map.
-  pomb->add_input_section(object, shndx);
-  Const_section_id csid(object, shndx);
-  this->merge_section_map_[csid] = pomb;
-
-  return true;
+  if (pomb->add_input_section(object, shndx))
+    {
+      // Add input section to new merge section and link input section to new
+      // merge section in map.
+      Const_section_id csid(object, shndx);
+      this->merge_section_map_[csid] = pomb;
+      return true;
+    }
+  else
+    return false;
 }
 
 // Build a relaxation map to speed up relaxation of existing input sections.

Re: [PATCH][GOLD] Avoid linker crashing when merge sections have uneven sizes.

[Ian Lance Taylor]

"Doug Kwan (關振德)" <dougkwan@google.com> writes:

>     This patch fixes a problem exposed by bootstrapping gcc.  libjava
> contains objects with unevenly sized merge sections, whose sections
> sizes are not multiples of their entsizes.

That sounds like a bug somewhere.


> 2010-04-08  Doug Kwan  <dougkwan@google.com>
>
>         * merge.cc (Output_merge_data::set_final_data_size): Handle empty
>         Output_merge_data.
>         * output.cc (Output_section::add_merge_input_section): Simplify
>         code and return status of Output_merge_base::add_input_section.
>         Update merge section map only if Output_merge_base::add_input_section
>         returns true.

This is OK.

Thanks.

Ian

Re: [PATCH][GOLD] Avoid linker crashing when merge sections have uneven sizes.

[Doug Kwan (關振德)]

Yes, it looks fishy but I did not dig into the gcc sources.  The
sections are .rodata sections for UTF8 strings for various sizes.  It
looks like the entries are aligned to 32-bit boundaries, not to the
entsize of a section.

-Doug

2010/4/8 Ian Lance Taylor <iant@google.com>:
> "Doug Kwan (關振德)" <dougkwan@google.com> writes:
>
>>     This patch fixes a problem exposed by bootstrapping gcc.  libjava
>> contains objects with unevenly sized merge sections, whose sections
>> sizes are not multiples of their entsizes.
>
> That sounds like a bug somewhere.
>
>
>> 2010-04-08  Doug Kwan  <dougkwan@google.com>
>>
>>         * merge.cc (Output_merge_data::set_final_data_size): Handle empty
>>         Output_merge_data.
>>         * output.cc (Output_section::add_merge_input_section): Simplify
>>         code and return status of Output_merge_base::add_input_section.
>>         Update merge section map only if Output_merge_base::add_input_section
>>         returns true.
>
> This is OK.
>
> Thanks.
>
> Ian
>

Re: [PATCH][GOLD] Avoid linker crashing when merge sections have uneven sizes.

[David Daney]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=Big5, Size: 1203 bytes --]

On 04/08/2010 05:02 PM, Doug Kwan (Ãö®¶¼w) wrote:
> Yes, it looks fishy but I did not dig into the gcc sources.  The
> sections are .rodata sections for UTF8 strings for various sizes.  It
> looks like the entries are aligned to 32-bit boundaries, not to the
> entsize of a section.
> 

FWIW:  http://gcc.gnu.org/ml/java-patches/2009-q1/msg00044.html

David Daney

> -Doug
> 
> 2010/4/8 Ian Lance Taylor<iant@google.com>:
>> "Doug Kwan (Ãö®¶¼w)"<dougkwan@google.com>  writes:
>>
>>>      This patch fixes a problem exposed by bootstrapping gcc.  libjava
>>> contains objects with unevenly sized merge sections, whose sections
>>> sizes are not multiples of their entsizes.
>>
>> That sounds like a bug somewhere.
>>
>>
>>> 2010-04-08  Doug Kwan<dougkwan@google.com>
>>>
>>>          * merge.cc (Output_merge_data::set_final_data_size): Handle empty
>>>          Output_merge_data.
>>>          * output.cc (Output_section::add_merge_input_section): Simplify
>>>          code and return status of Output_merge_base::add_input_section.
>>>          Update merge section map only if Output_merge_base::add_input_section
>>>          returns true.
>>
>> This is OK.
>>
>> Thanks.
>>
>> Ian
>>
>