[v2] Treat STV_HIDDEN and STV_INTERNAL symbols as STB_LOCAL
diff mbox

Message ID alpine.DEB.2.00.1605070212010.6794@tp.orcam.me.uk
State Accepted
Headers show

Commit Message

Maciej W. Rozycki May 7, 2016, 1:29 a.m. UTC
In a reference to PR ld/19908 make ld.so respect symbol export classes 
aka visibility and treat STV_HIDDEN and STV_INTERNAL symbols as local, 
preventing such symbols from preempting exported symbols.

According to the ELF gABI[1] neither STV_HIDDEN nor STV_INTERNAL symbols 
are supposed to be present in linked binaries:

"A hidden symbol contained in a relocatable object must be either
removed or converted to STB_LOCAL binding by the link-editor when the
relocatable object is included in an executable file or shared object."

"An internal symbol contained in a relocatable object must be either
removed or converted to STB_LOCAL binding by the link-editor when the
relocatable object is included in an executable file or shared object."

however some GNU binutils versions produce such symbols in some cases.  
PR ld/19908 is one and we also have this note in scripts/abilist.awk:

# binutils versions up through at least 2.23 have some bugs that
# caused STV_HIDDEN symbols to appear in .dynsym, though that is useless.

so clearly there is linked code out there which contains such symbols 
which is prone to symbol table misinterpretation, and it'll be more 
productive if we handle this gracefully, under the Robustness Principle: 
"be liberal in what you accept, and conservative in what you produce", 
especially as this is a simple (STV_HIDDEN|STV_INTERNAL) => STB_LOCAL 
mapping.

References:

[1] "System V Application Binary Interface - DRAFT - 24 April 2001",
    The Santa Cruz Operation, Inc., "Symbol Table",
    <http://www.sco.com/developers/gabi/2001-04-24/ch4.symtab.html>

2016-05-07  Maciej W. Rozycki  <macro@imgtec.com>

	* sysdeps/generic/ldsodefs.h 
	(dl_symbol_visibility_binds_local_p): New inline function.
	* elf/dl-addr.c (determine_info): Treat hidden and internal
	symbols as local.
	* elf/dl-lookup.c (do_lookup_x): Likewise.
	* elf/dl-reloc.c (RESOLVE_MAP): Likewise.
---
Hi,

 This is an updated version taking Roland's suggestions into account.  
Rather than using a macro I decided to go for an inline function instead, 
as I have a preference for them in code which can assume modern tools.  
Among the reasons are type checking and single evaluation of the 
parameters.  I hope this choice is OK, we have numerous inline functions 
throughout already.

 This has passed testing with the mips-linux-gnu target, little-endian, 
o32 ABI, with no regressions.

 OK to apply?

  Maciej

Changes from v1:

- factor out checks to `dl_symbol_visibility_binds_local_p',

- update the formatting of `ELFW' macro calls.

glibc-elf-export-class-local.diff

Comments

Maciej W. Rozycki May 7, 2016, 1:38 a.m. UTC | #1
On Sat, 7 May 2016, Maciej W. Rozycki wrote:

> Index: glibc/elf/dl-reloc.c
> ===================================================================
> --- glibc.orig/elf/dl-reloc.c	2016-05-05 19:36:17.418334585 +0100
> +++ glibc/elf/dl-reloc.c	2016-05-06 04:41:12.649078174 +0100
> @@ -233,7 +233,8 @@ _dl_relocate_object (struct link_map *l,
>  
>      /* This macro is used as a callback from the ELF_DYNAMIC_RELOCATE code.  */
>  #define RESOLVE_MAP(ref, version, r_type) \
> -    (ELFW(ST_BIND) ((*ref)->st_info) != STB_LOCAL			      \
> +    ((ELFW (ST_BIND) ((*ref)->st_info) != STB_LOCAL			      \

 I missed the unwanted `ELFW (ST_BIND)' part in this change and will keep 
the original formatting intact in the final commit.  I don't think there's 
any sense in reposting the whole submission just for this bit.

  Maciej
Maciej W. Rozycki May 16, 2016, 8:15 a.m. UTC | #2
Ping: <https://www.sourceware.org/ml/libc-alpha/2016-05/msg00142.html> is 
pending patch review.

  Maciej
Mike Frysinger May 22, 2016, 4:14 a.m. UTC | #3
On 07 May 2016 02:29, Maciej W. Rozycki wrote:
> In a reference to PR ld/19908 make ld.so respect symbol export classes 
> aka visibility and treat STV_HIDDEN and STV_INTERNAL symbols as local, 
> preventing such symbols from preempting exported symbols.
> 
> According to the ELF gABI[1] neither STV_HIDDEN nor STV_INTERNAL symbols 
> are supposed to be present in linked binaries:
> 
> "A hidden symbol contained in a relocatable object must be either
> removed or converted to STB_LOCAL binding by the link-editor when the
> relocatable object is included in an executable file or shared object."
> 
> "An internal symbol contained in a relocatable object must be either
> removed or converted to STB_LOCAL binding by the link-editor when the
> relocatable object is included in an executable file or shared object."
> 
> however some GNU binutils versions produce such symbols in some cases.  
> PR ld/19908 is one and we also have this note in scripts/abilist.awk:
> 
> # binutils versions up through at least 2.23 have some bugs that
> # caused STV_HIDDEN symbols to appear in .dynsym, though that is useless.
> 
> so clearly there is linked code out there which contains such symbols 
> which is prone to symbol table misinterpretation, and it'll be more 
> productive if we handle this gracefully, under the Robustness Principle: 
> "be liberal in what you accept, and conservative in what you produce", 
> especially as this is a simple (STV_HIDDEN|STV_INTERNAL) => STB_LOCAL 
> mapping.

your basic premise sounds fine as does your reading of the spec.
i glanced at the places you're adding checks and they look OK to
me, but i'm not super familiar with all the ldso/libdl paths.
-mike
Maciej W. Rozycki May 26, 2016, 11:10 p.m. UTC | #4
Hi Mike,

> your basic premise sounds fine as does your reading of the spec.
> i glanced at the places you're adding checks and they look OK to
> me, but i'm not super familiar with all the ldso/libdl paths.

 Thank you for your review.

 Does anyone have anything to add or have we reached a consensus?

  Maciej
Maciej W. Rozycki June 9, 2016, 2:37 p.m. UTC | #5
On Fri, 27 May 2016, Maciej W. Rozycki wrote:

> > your basic premise sounds fine as does your reading of the spec.
> > i glanced at the places you're adding checks and they look OK to
> > me, but i'm not super familiar with all the ldso/libdl paths.
> 
>  Thank you for your review.
> 
>  Does anyone have anything to add or have we reached a consensus?

Roland,

 You were kind enough to review the first version of the patch -- may I 
ask you to chime in and say if you are happy with the way I addressed your 
concerns?

 For your convenience here is a link to an archived copy of this change:
<https://www.sourceware.org/ml/libc-alpha/2016-05/msg00142.html>.

  Maciej
Maciej W. Rozycki July 1, 2016, 10:56 p.m. UTC | #6
On Fri, 27 May 2016, Maciej W. Rozycki wrote:

> > your basic premise sounds fine as does your reading of the spec.
> > i glanced at the places you're adding checks and they look OK to
> > me, but i'm not super familiar with all the ldso/libdl paths.
> 
>  Thank you for your review.
> 
>  Does anyone have anything to add or have we reached a consensus?

 I saw no further comments and certainly no objections, so I have 
committed this change now so that it makes it to 2.24.

 Thanks, Mike, again for your review.

  Maciej

Patch
diff mbox

Index: glibc/elf/dl-addr.c
===================================================================
--- glibc.orig/elf/dl-addr.c	2016-05-05 19:36:17.395247035 +0100
+++ glibc/elf/dl-addr.c	2016-05-06 04:37:39.262222495 +0100
@@ -88,6 +88,7 @@  determine_info (const ElfW(Addr) addr, s
       for (; (void *) symtab < (void *) symtabend; ++symtab)
 	if ((ELFW(ST_BIND) (symtab->st_info) == STB_GLOBAL
 	     || ELFW(ST_BIND) (symtab->st_info) == STB_WEAK)
+	    && __glibc_likely (!dl_symbol_visibility_binds_local_p (symtab))
 	    && ELFW(ST_TYPE) (symtab->st_info) != STT_TLS
 	    && (symtab->st_shndx != SHN_UNDEF
 		|| symtab->st_value != 0)
Index: glibc/elf/dl-lookup.c
===================================================================
--- glibc.orig/elf/dl-lookup.c	2016-05-05 20:13:10.146644695 +0100
+++ glibc/elf/dl-lookup.c	2016-05-06 04:40:30.236831042 +0100
@@ -516,6 +516,10 @@  do_lookup_x (const char *undef_name, uin
 #endif
 	    }
 
+	  /* Hidden and internal symbols are local, ignore them.  */
+	  if (__glibc_unlikely (dl_symbol_visibility_binds_local_p (sym)))
+	    goto skip;
+
 	  switch (ELFW(ST_BIND) (sym->st_info))
 	    {
 	    case STB_WEAK:
Index: glibc/elf/dl-reloc.c
===================================================================
--- glibc.orig/elf/dl-reloc.c	2016-05-05 19:36:17.418334585 +0100
+++ glibc/elf/dl-reloc.c	2016-05-06 04:41:12.649078174 +0100
@@ -233,7 +233,8 @@  _dl_relocate_object (struct link_map *l,
 
     /* This macro is used as a callback from the ELF_DYNAMIC_RELOCATE code.  */
 #define RESOLVE_MAP(ref, version, r_type) \
-    (ELFW(ST_BIND) ((*ref)->st_info) != STB_LOCAL			      \
+    ((ELFW (ST_BIND) ((*ref)->st_info) != STB_LOCAL			      \
+      && __glibc_likely (!dl_symbol_visibility_binds_local_p (*ref)))	      \
      ? ((__builtin_expect ((*ref) == l->l_lookup_cache.sym, 0)		      \
 	 && elf_machine_type_class (r_type) == l->l_lookup_cache.type_class)  \
 	? (bump_num_cache_relocations (),				      \
Index: glibc/sysdeps/generic/ldsodefs.h
===================================================================
--- glibc.orig/sysdeps/generic/ldsodefs.h	2016-05-05 20:13:20.000000000 +0100
+++ glibc/sysdeps/generic/ldsodefs.h	2016-05-06 07:23:22.874587470 +0100
@@ -88,6 +88,19 @@  typedef struct link_map *lookup_t;
        || (ADDR) < (L)->l_addr + (SYM)->st_value + (SYM)->st_size)	\
    && ((MATCHSYM) == NULL || (MATCHSYM)->st_value < (SYM)->st_value))
 
+/* According to the ELF gABI no STV_HIDDEN or STV_INTERNAL symbols are
+   expected to be present in dynamic symbol tables as they should have
+   been either removed or converted to STB_LOCAL binding by the static
+   linker.  However some GNU binutils versions produce such symbols in
+   some cases.  To prevent such symbols present in a buggy binary from
+   preempting global symbols we filter them out with this predicate.  */
+static __always_inline bool
+dl_symbol_visibility_binds_local_p (const ElfW(Sym) *sym)
+{
+  return (ELFW(ST_VISIBILITY) (sym->st_other) == STV_HIDDEN
+	  || ELFW(ST_VISIBILITY) (sym->st_other) == STV_INTERNAL);
+}
+
 /* Unmap a loaded object, called by _dl_close (). */
 #ifndef DL_UNMAP_IS_SPECIAL
 # define DL_UNMAP(map)	_dl_unmap_segments (map)