[PATCH][GOLD] Fix dangling pointer bug due to premature unlock.

[PATCH][GOLD] Fix dangling pointer bug due to premature unlock.

[Doug Kwan (關振德)]

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

Hi

   This fixes a bug in which an object is released too early, causing
a pointer to point to unmapped memory.  My fix is to move the locking
code to the caller of get_section_contents() and replace the original
locking code with a check.  This has been tested on x86_64.

-Doug


2011-01-25  Doug Kwan  <dougkwan@google.com>

        * gold/icf.cc (get_section_contents): Instead of locking object in
        two places, ask caller to lock it before calling. Add an assert
        to check that object is locked in the first iteration.
        (match_sections): Lock object before calling get_section_contents()
        in the first iteration.

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

Index: gold/icf.cc
===================================================================
RCS file: /cvs/src/src/gold/icf.cc,v
retrieving revision 1.17
diff -u -u -p -r1.17 icf.cc
--- gold/icf.cc	14 Dec 2010 19:03:30 -0000	1.17
+++ gold/icf.cc	25 Jan 2011 08:36:15 -0000
@@ -221,6 +221,8 @@ preprocess_for_unique_sections(const std
 // Parameters  :
 // FIRST_ITERATION    : true if it is the first invocation.
 // SECN               : Section for which contents are desired.
+//			For the first iteration, caller needs to
+//			lock the section's object.
 // SECTION_NUM        : Unique section number of this section.
 // NUM_TRACKED_RELOCS : Vector reference to store the number of relocs
 //                      to ICF sections.
@@ -240,13 +242,13 @@ get_section_contents(bool first_iteratio
   section_size_type plen;
   const unsigned char* contents = NULL;
 
+  // We access the section's object in the first iteration and that requires
+  // locking.  Instead of locking and unlocking in two places here, we require
+  // a caller to lock the object before calling this for the first iteration.
+  gold_assert(!first_iteration || secn.first->is_locked());
+
   if (first_iteration)
     {
-      // Lock the object so we can read from it.  This is only called
-      // single-threaded from queue_middle_tasks, so it is OK to lock.
-      // Unfortunately we have no way to pass in a Task token.
-      const Task* dummy_task = reinterpret_cast<const Task*>(-1);
-      Task_lock_obj<Object> tl(dummy_task, secn.first);
       contents = secn.first->section_contents(secn.second,
                                               &plen,
                                               false);
@@ -373,12 +375,6 @@ get_section_contents(bool first_iteratio
               if (!first_iteration)
                 continue;
 
-              // Lock the object so we can read from it.  This is only called
-              // single-threaded from queue_middle_tasks, so it is OK to lock.
-              // Unfortunately we have no way to pass in a Task token.
-              const Task* dummy_task = reinterpret_cast<const Task*>(-1);
-              Task_lock_obj<Object> tl(dummy_task, it_v->first);
-
               uint64_t secn_flags = (it_v->first)->section_flags(it_v->second);
               // This reloc points to a merge section.  Hash the
               // contents of this section.
@@ -591,6 +587,12 @@ match_sections(unsigned int iteration_nu
       if (iteration_num == 1)
         {
           unsigned int num_relocs = 0;
+
+	  // Lock the object so we can read from it.  This is only called
+	  // single-threaded from queue_middle_tasks, so it is OK to lock.
+	  // Unfortunately we have no way to pass in a Task token.
+	  const Task* dummy_task = reinterpret_cast<const Task*>(-1);
+	  Task_lock_obj<Object> tl(dummy_task, secn.first);
           this_secn_contents = get_section_contents(true, secn, i, &num_relocs,
                                                     symtab, (*kept_section_id),
                                                     section_contents);

Re: [PATCH][GOLD] Fix dangling pointer bug due to premature unlock.

[Ian Lance Taylor]

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

>    This fixes a bug in which an object is released too early, causing
> a pointer to point to unmapped memory.  My fix is to move the locking
> code to the caller of get_section_contents() and replace the original
> locking code with a check.  This has been tested on x86_64.
>
> -Doug
>
>
> 2011-01-25  Doug Kwan  <dougkwan@google.com>
>
>         * gold/icf.cc (get_section_contents): Instead of locking object in
>         two places, ask caller to lock it before calling. Add an assert
>         to check that object is locked in the first iteration.
>         (match_sections): Lock object before calling get_section_contents()
>         in the first iteration.

This patch is fine.  However, it would be slightly simpler to just
always lock the object in get_section_contents, regardless of whether
first_iteration is true or not.  Task locks in gold are not mutexes or
anything, they are basically free.  There is no reason to be careful to
only do the lock on the first iteration.

The patch is OK either way.

Thanks.

Ian

Re: [PATCH][GOLD] Fix dangling pointer bug due to premature unlock.

[Doug Kwan (關振德)]

In the name of simplicity, I will change it to always lock then.

Thanks

-Doug

On Tue, Jan 25, 2011 at 7:26 AM, Ian Lance Taylor <iant@google.com> wrote:
> "Doug Kwan (關振德)" <dougkwan@google.com> writes:
>
>>    This fixes a bug in which an object is released too early, causing
>> a pointer to point to unmapped memory.  My fix is to move the locking
>> code to the caller of get_section_contents() and replace the original
>> locking code with a check.  This has been tested on x86_64.
>>
>> -Doug
>>
>>
>> 2011-01-25  Doug Kwan  <dougkwan@google.com>
>>
>>         * gold/icf.cc (get_section_contents): Instead of locking object in
>>         two places, ask caller to lock it before calling. Add an assert
>>         to check that object is locked in the first iteration.
>>         (match_sections): Lock object before calling get_section_contents()
>>         in the first iteration.
>
> This patch is fine.  However, it would be slightly simpler to just
> always lock the object in get_section_contents, regardless of whether
> first_iteration is true or not.  Task locks in gold are not mutexes or
> anything, they are basically free.  There is no reason to be careful to
> only do the lock on the first iteration.
>
> The patch is OK either way.
>
> Thanks.
>
> Ian
>