[Patch]: Fix ld pr11138 FAILures on mips*.

[Patch]: Fix ld pr11138 FAILures on mips*.

[David Daney]

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

The pr11138 testcase links an executable with a version script.  On 
mips64-linux the presence of a version script was causing the 
MIPS_RLD_MAP dynamic tag to be populated with a NULL value.  When such 
an executable was run ld.so would try to dereference this and receive 
SIGSEGV, thus killing the process.

The root cause of this is that the mips linker synthesizes a special 
symbol "__RLD_MAP", and then sets MIPS_RLD_MAP to point to it.  When a 
version script is present, this symbol gets versioned along with all the 
rest, and when it is time to take its address, the symbol can no longer 
be found as it has had version information appended to its name.

Since "__RLD_MAP" is really part of the ABI, we want to exclude it from 
symbol versioning.  To this end, I introduced a new symbol flag 
'no_sym_version' to tag this type of symbol.  When the "__RLD_MAP" 
symbol is created, we set this flag.

In _bfd_elf_link_assign_sym_version, we then skip all symbols that have 
'no_sym_version' set, and everything now works.

This problem has also been reported in the wild when linking the firefox 
executable.

Tested on mips64-linux-gnu and x86_64-linux-gnu

Ok to commit?

2011-12-05  David Daney  <david.daney@cavium.com>

	* elf-bfd.h (elf_link_hash_entry): Add no_sym_version field.
	* elflink.c (_bfd_elf_link_assign_sym_version): Don't assign a
	version if no_sym_version is set.
	* elfxx-mips.c (_bfd_mips_elf_create_dynamic_sections): Set
	no_sym_version for "__RLD_MAP".

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

Index: bfd/elf-bfd.h
===================================================================
RCS file: /cvs/src/src/bfd/elf-bfd.h,v
retrieving revision 1.329
diff -u -p -r1.329 elf-bfd.h
--- bfd/elf-bfd.h	17 Aug 2011 00:39:38 -0000	1.329
+++ bfd/elf-bfd.h	5 Dec 2011 20:15:49 -0000
@@ -198,6 +198,8 @@ struct elf_link_hash_entry
   unsigned int pointer_equality_needed : 1;
   /* Symbol is a unique global symbol.  */
   unsigned int unique_global : 1;
+  /* Symbol should not be versioned.  It is part of the ABI */
+  unsigned int no_sym_version : 1;
 
   /* String table index in .dynstr if this is a dynamic symbol.  */
   unsigned long dynstr_index;
Index: bfd/elflink.c
===================================================================
RCS file: /cvs/src/src/bfd/elflink.c,v
retrieving revision 1.430
diff -u -p -r1.430 elflink.c
--- bfd/elflink.c	15 Nov 2011 11:33:57 -0000	1.430
+++ bfd/elflink.c	5 Dec 2011 20:15:50 -0000
@@ -1946,6 +1946,9 @@ _bfd_elf_link_assign_sym_version (struct
   if (!h->def_regular)
     return TRUE;
 
+  if (h->no_sym_version)
+    return TRUE;
+
   bed = get_elf_backend_data (info->output_bfd);
   p = strchr (h->root.root.string, ELF_VER_CHR);
   if (p != NULL && h->verinfo.vertree == NULL)
Index: bfd/elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.296
diff -u -p -r1.296 elfxx-mips.c
--- bfd/elfxx-mips.c	29 Nov 2011 20:28:54 -0000	1.296
+++ bfd/elfxx-mips.c	5 Dec 2011 20:15:50 -0000
@@ -7260,6 +7260,7 @@ _bfd_mips_elf_create_dynamic_sections (b
 	  h = (struct elf_link_hash_entry *) bh;
 	  h->non_elf = 0;
 	  h->def_regular = 1;
+	  h->no_sym_version = 1;
 	  h->type = STT_OBJECT;
 
 	  if (! bfd_elf_link_record_dynamic_symbol (info, h))

Re: [Patch]: Fix ld pr11138 FAILures on mips*.

[Alan Modra]

On Mon, Dec 05, 2011 at 04:49:35PM -0800, David Daney wrote:
> The root cause of this is that the mips linker synthesizes a special
> symbol "__RLD_MAP", and then sets MIPS_RLD_MAP to point to it.  When
> a version script is present, this symbol gets versioned along with
> all the rest, and when it is time to take its address, the symbol
> can no longer be found as it has had version information appended to
> its name.

Why not just change

	  && (strcmp (name, "__rld_map") == 0
	      || strcmp (name, "__RLD_MAP") == 0))

to

	  && (strncmp (name, "__rld_map", 9) == 0
	      || strncmp (name, "__RLD_MAP", 9) == 0))

in _bfd_mips_elf_finish_dynamic_symbol?  Perhaps the same for other
syms there too?

-- 
Alan Modra
Australia Development Lab, IBM

Re: [Patch]: Fix ld pr11138 FAILures on mips*.

[David Daney]

On 12/05/2011 09:40 PM, Alan Modra wrote:
> On Mon, Dec 05, 2011 at 04:49:35PM -0800, David Daney wrote:
>> The root cause of this is that the mips linker synthesizes a special
>> symbol "__RLD_MAP", and then sets MIPS_RLD_MAP to point to it.  When
>> a version script is present, this symbol gets versioned along with
>> all the rest, and when it is time to take its address, the symbol
>> can no longer be found as it has had version information appended to
>> its name.
>
> Why not just change
>
> 	&&  (strcmp (name, "__rld_map") == 0
> 	      || strcmp (name, "__RLD_MAP") == 0))
>
> to
>
> 	&&  (strncmp (name, "__rld_map", 9) == 0
> 	      || strncmp (name, "__RLD_MAP", 9) == 0))
>
> in _bfd_mips_elf_finish_dynamic_symbol?  Perhaps the same for other
> syms there too?

Because that doesn't work.  Perhpas I should have been a bit more 
detailed in my description of what is happening (at least in one case).

If the version script contains something like:
{
         global: main;
         local: *;
};

Then "__RLD_MAP" gets hidden and we never see it in 
_bfd_mips_elf_finish_dynamic_symbol().

This hiding gets done precisely in  _bfd_elf_link_assign_sym_version() 
after the version information is calculated.  So as the patch stands, we 
bail out of _bfd_elf_link_assign_sym_version() before the symbol is 
hidden (or modified in any way).

It is possible that 'no_sym_version' is not the best name for the flag, 
but I think we really some sort of flag to exclude ABI symbols from 
being mangled in _bfd_elf_link_assign_sym_version().

David Daney

Re: [Patch]: Fix ld pr11138 FAILures on mips*.

[Richard Sandiford]

Alan Modra <amodra@gmail.com> writes:
> On Mon, Dec 05, 2011 at 04:49:35PM -0800, David Daney wrote:
>> The root cause of this is that the mips linker synthesizes a special
>> symbol "__RLD_MAP", and then sets MIPS_RLD_MAP to point to it.  When
>> a version script is present, this symbol gets versioned along with
>> all the rest, and when it is time to take its address, the symbol
>> can no longer be found as it has had version information appended to
>> its name.
>
> Why not just change
>
> 	  && (strcmp (name, "__rld_map") == 0
> 	      || strcmp (name, "__RLD_MAP") == 0))
>
> to
>
> 	  && (strncmp (name, "__rld_map", 9) == 0
> 	      || strncmp (name, "__RLD_MAP", 9) == 0))
>
> in _bfd_mips_elf_finish_dynamic_symbol?  Perhaps the same for other
> syms there too?

Showing my ignorance here, but is that the usual behaviour for this kind
of thing?  I wouldn't have expected versions to apply to internally-created
symbols.

There again, is this symbol (as opposed to the DT_MIPS_RLD_MAP tag)
actually part of the ABI?  I can't find any reference to it in the
original psABI, the SGI ELF64 spec, gdb or glibc.  If it's just an
internal thing, maybe we could get rid of it altogether, or at least
make it bind locally rather than globally.

Richard

Re: [Patch]: Fix ld pr11138 FAILures on mips*.

[David Daney]

On 12/06/2011 01:16 PM, Richard Sandiford wrote:
> Alan Modra<amodra@gmail.com>  writes:
>> On Mon, Dec 05, 2011 at 04:49:35PM -0800, David Daney wrote:
>>> The root cause of this is that the mips linker synthesizes a special
>>> symbol "__RLD_MAP", and then sets MIPS_RLD_MAP to point to it.  When
>>> a version script is present, this symbol gets versioned along with
>>> all the rest, and when it is time to take its address, the symbol
>>> can no longer be found as it has had version information appended to
>>> its name.
>>
>> Why not just change
>>
>> 	&&  (strcmp (name, "__rld_map") == 0
>> 	      || strcmp (name, "__RLD_MAP") == 0))
>>
>> to
>>
>> 	&&  (strncmp (name, "__rld_map", 9) == 0
>> 	      || strncmp (name, "__RLD_MAP", 9) == 0))
>>
>> in _bfd_mips_elf_finish_dynamic_symbol?  Perhaps the same for other
>> syms there too?
>
> Showing my ignorance here,

I don't buy it, you are probably the most knowledgeable about this.

> but is that the usual behaviour for this kind
> of thing?  I wouldn't have expected versions to apply to internally-created
> symbols.

Yes, that is what I was trying to accomplish.

>
> There again, is this symbol (as opposed to the DT_MIPS_RLD_MAP tag)
> actually part of the ABI?  I can't find any reference to it in the
> original psABI, the SGI ELF64 spec, gdb or glibc.  If it's just an
> internal thing, maybe we could get rid of it altogether, or at least
> make it bind locally rather than globally.
>

That is an option too I suppose.  I would say that it is part of a de 
facto ABI if nothing else.  The question of weather anybody uses it it a 
different question.  I thought boehm-gc may have used it, but I cannot 
find it there now.

I don't know for sure why the symbol was created, but it seems like it 
may just be for the side effect of having 
_bfd_mips_elf_finish_dynamic_symbol() called.  This lets us determine 
mips_elf_hash_table(info)->rld_value at a time when the output sections 
have already been laid out.

It might be possible to #define elf_backend_output_arch_local_syms and 
then handle calculation of the rld_value value there instead.

If this seems like a good approach, I can prepare and test a patch that 
does that.

David Daney

Re: [Patch]: Fix ld pr11138 FAILures on mips*.

[Alan Modra]

On Tue, Dec 06, 2011 at 12:20:14PM -0800, David Daney wrote:
> Then "__RLD_MAP" gets hidden and we never see it in
> _bfd_mips_elf_finish_dynamic_symbol().

I see.  Well, the real question is whether this symbol needs to be
dynamic.  If it does, then you can't allow it to be hidden.  That
would best be accomplished by writing a mips elf_backend_hide_symbol
function, which is nicer than adding another field used by only one
target to struct elf_link_hash_entry.  

-- 
Alan Modra
Australia Development Lab, IBM

Re: [Patch]: Fix ld pr11138 FAILures on mips*.

[Alan Modra]

On Tue, Dec 06, 2011 at 09:16:22PM +0000, Richard Sandiford wrote:
> Showing my ignorance here, but is that the usual behaviour for this kind
> of thing?  I wouldn't have expected versions to apply to internally-created
> symbols.

Normally this sort of symbol isn't dynamic, at least nowadays.
Earlier versions of GNU ld made many symbols like
_GLOBAL_OFFSET_TABLE_ dynamic unnecessarily, so it might be just a
case of old mips code following even older practice.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [Patch]: Fix ld pr11138 FAILures on mips*.

[Richard Sandiford]

David Daney <david.daney@cavium.com> writes:
>> There again, is this symbol (as opposed to the DT_MIPS_RLD_MAP tag)
>> actually part of the ABI?  I can't find any reference to it in the
>> original psABI, the SGI ELF64 spec, gdb or glibc.  If it's just an
>> internal thing, maybe we could get rid of it altogether, or at least
>> make it bind locally rather than globally.
>>
>
> That is an option too I suppose.  I would say that it is part of a de 
> facto ABI if nothing else.  The question of weather anybody uses it it a 
> different question.  I thought boehm-gc may have used it, but I cannot 
> find it there now.

Yeah, good point.  It occured to me rather belatedly that if wasn't
part of the de facto ABI, it wouldn't have two distinct names...

So if we were just doing this for the tag, the simplest way would have
been to find the .rld_map section in _bfd_mips_elf_finish_dynamic_sections.
But I agree that we should keep the symbol Just In Case.  So...

> It might be possible to #define elf_backend_output_arch_local_syms and 
> then handle calculation of the rld_value value there instead.
>
> If this seems like a good approach, I can prepare and test a patch that 
> does that.

...how about caching the hash table entry for __rld_map/__RLD_MAP/
__rld_obj_head in mips_elf_link_hash_entry, instead of rld_value.
(One field shared by all three should be enough.)  We can then use
that in _bfd_mips_elf_finish_dynamic_sections.

The code to set sym->st_value in _bfd_mips_elf_finish_dynamic_symbol
should already be redundant: the symbol is defined as being at the
start of .rld_map by:

	  s = bfd_get_section_by_name (abfd, ".rld_map");
	  BFD_ASSERT (s != NULL);

	  name = SGI_COMPAT (abfd) ? "__rld_map" : "__RLD_MAP";
	  bh = NULL;
	  if (!(_bfd_generic_link_add_one_symbol
		(info, abfd, name, BSF_GLOBAL, s, 0, NULL, FALSE,
		 get_elf_backend_data (abfd)->collect, &bh)))
	    return FALSE;

And the code to clear the first word should be redundant too, since
_bfd_mips_elf_size_dynamic_sections uses bfd_zalloc to allocate the
section's contents.  So if we do cache the hash table entry, the whole
_bfd_mips_elf_finish_dynamic_symbol handling should become dead.

Richard

Re: [Patch]: Fix ld pr11138 FAILures on mips*.

[David Daney]

On 12/08/2011 12:34 PM, Richard Sandiford wrote:
> David Daney<david.daney@cavium.com>  writes:
>>> There again, is this symbol (as opposed to the DT_MIPS_RLD_MAP tag)
>>> actually part of the ABI?  I can't find any reference to it in the
>>> original psABI, the SGI ELF64 spec, gdb or glibc.  If it's just an
>>> internal thing, maybe we could get rid of it altogether, or at least
>>> make it bind locally rather than globally.
>>>
>>
>> That is an option too I suppose.  I would say that it is part of a de
>> facto ABI if nothing else.  The question of weather anybody uses it it a
>> different question.  I thought boehm-gc may have used it, but I cannot
>> find it there now.
>
> Yeah, good point.  It occured to me rather belatedly that if wasn't
> part of the de facto ABI, it wouldn't have two distinct names...
>
> So if we were just doing this for the tag, the simplest way would have
> been to find the .rld_map section in _bfd_mips_elf_finish_dynamic_sections.
> But I agree that we should keep the symbol Just In Case.  So...
>
>> It might be possible to #define elf_backend_output_arch_local_syms and
>> then handle calculation of the rld_value value there instead.
>>
>> If this seems like a good approach, I can prepare and test a patch that
>> does that.
>
> ...how about caching the hash table entry for __rld_map/__RLD_MAP/
> __rld_obj_head in mips_elf_link_hash_entry, instead of rld_value.
> (One field shared by all three should be enough.)  We can then use
> that in _bfd_mips_elf_finish_dynamic_sections.
>
> The code to set sym->st_value in _bfd_mips_elf_finish_dynamic_symbol
> should already be redundant: the symbol is defined as being at the
> start of .rld_map by:
>
> 	  s = bfd_get_section_by_name (abfd, ".rld_map");
> 	  BFD_ASSERT (s != NULL);
>
> 	  name = SGI_COMPAT (abfd) ? "__rld_map" : "__RLD_MAP";
> 	  bh = NULL;
> 	  if (!(_bfd_generic_link_add_one_symbol
> 		(info, abfd, name, BSF_GLOBAL, s, 0, NULL, FALSE,
> 		 get_elf_backend_data (abfd)->collect,&bh)))
> 	    return FALSE;
>
> And the code to clear the first word should be redundant too, since
> _bfd_mips_elf_size_dynamic_sections uses bfd_zalloc to allocate the
> section's contents.

That's right, also elsewhere we set the size of the section to 4, which 
is not correct for ELF64 targets.

>  So if we do cache the hash table entry, the whole
> _bfd_mips_elf_finish_dynamic_symbol handling should become dead.
>
> Richard

Thanks Richard,

Those are all good ideas, so I think I will hack up a new patch to try 
and implement this plan.  Some care also needs to be taken for the SGI 
case as there the symbol seems to come from one of the input objects 
rather than being synthesized by the linker.

David Daney