[v2,2/2] elf: Correct absolute (SHN_ABS) symbol run-time calculation [BZ #19818]
diff mbox series

Message ID alpine.DEB.2.00.1802201815060.3553@tp.orcam.me.uk
State Accepted
Headers show
Series
  • Correct absolute (SHN_ABS) symbol run-time calculation [BZ #19818]
Related show

Commit Message

Maciej W. Rozycki Feb. 20, 2018, 6:38 p.m. UTC
Do not relocate absolute symbols by the base address.  Such symbols have 
SHN_ABS as the section index and their value is not supposed to be 
affected by relocation as per the ELF gABI[1]:

"SHN_ABS
    The symbol has an absolute value that will not change because of
    relocation."

The reason for our non-conformance here seems to be an old SysV linker 
bug causing symbols like _DYNAMIC to be incorrectly emitted as absolute 
symbols[2].  However in a previous discussion it was pointed that this 
is seriously flawed by preventing the lone purpose of the existence of 
absolute symbols from being used[3]:

"On the contrary, the only interpretation that makes sense to me is that 
it will not change because of relocation at link time or at load time.  
Absolute symbols, from the days of the earliest linking loaders, have 
been used to represent addresses that are outside the address space of 
the module (e.g., memory-mapped addresses or kernel gateway pages).  
They've even been used to represent true symbolic constants (e.g., 
system entry point numbers, sizes, version numbers).  There's no other 
way to represent a true absolute symbol, while the meaning you seek is 
easily represented by giving the symbol a non-negative st_shndx value." 

and we ought to stop supporting our current broken interpretation.

Update processing for dladdr(3) and dladdr1(3) so that SHN_ABS symbols 
are ignored, because under the corrected interpretation they do not 
represent addresses within a mapped file and therefore are not supposed 
to be considered.

References:

[1] "System V Application Binary Interface - DRAFT - 19 October 2010",
    The SCO Group, Section "Symbol Table",
    <http://www.sco.com/developers/gabi/2012-12-31/ch4.symtab.html>

[2] Alan Modra, "Absolute symbols" 
    <https://sourceware.org/ml/binutils/2012-05/msg00019.html>

[3] Cary Coutant, "Re: Absolute symbols"
    <https://sourceware.org/ml/binutils/2012-05/msg00020.html>

	[BZ #19818]
	* sysdeps/generic/ldsodefs.h (SYMBOL_ADDRESS): Handle SHN_ABS 
	symbols.
	* elf/dl-addr.c (determine_info): Ignore SHN_ABS symbols.
	* elf/tst-absolute-sym.c: New file.
	* elf/tst-absolute-sym-lib.c: New file.
	* elf/tst-absolute-sym-lib.lds: New file.
	* elf/Makefile (tests): Add `tst-absolute-sym'.
	(modules-names): Add `tst-absolute-sym-lib'.
	(LDLIBS-tst-absolute-sym-lib.so): New variable.
	($(objpfx)tst-absolute-sym-lib.so): New dependency.
	($(objpfx)tst-absolute-sym): New dependency.
---
Changes from v1:

- Ignore SHN_ABS symbols in `determine_info',

- Regenerate for the 1/2 update.

 OK to apply?

  Maciej
---
 elf/Makefile                 |    8 ++++++--
 elf/dl-addr.c                |    2 ++
 elf/tst-absolute-sym-lib.c   |   25 +++++++++++++++++++++++++
 elf/tst-absolute-sym-lib.lds |   19 +++++++++++++++++++
 elf/tst-absolute-sym.c       |   38 ++++++++++++++++++++++++++++++++++++++
 sysdeps/generic/ldsodefs.h   |    3 ++-
 6 files changed, 92 insertions(+), 3 deletions(-)

glibc-elf-shn-abs.diff

Comments

Adhemerval Zanella March 7, 2018, 8:25 p.m. UTC | #1
On 20/02/2018 15:38, Maciej W. Rozycki wrote:
> Do not relocate absolute symbols by the base address.  Such symbols have 
> SHN_ABS as the section index and their value is not supposed to be 
> affected by relocation as per the ELF gABI[1]:
> 
> "SHN_ABS
>     The symbol has an absolute value that will not change because of
>     relocation."
> 
> The reason for our non-conformance here seems to be an old SysV linker 
> bug causing symbols like _DYNAMIC to be incorrectly emitted as absolute 
> symbols[2].  However in a previous discussion it was pointed that this 
> is seriously flawed by preventing the lone purpose of the existence of 
> absolute symbols from being used[3]:
> 
> "On the contrary, the only interpretation that makes sense to me is that 
> it will not change because of relocation at link time or at load time.  
> Absolute symbols, from the days of the earliest linking loaders, have 
> been used to represent addresses that are outside the address space of 
> the module (e.g., memory-mapped addresses or kernel gateway pages).  
> They've even been used to represent true symbolic constants (e.g., 
> system entry point numbers, sizes, version numbers).  There's no other 
> way to represent a true absolute symbol, while the meaning you seek is 
> easily represented by giving the symbol a non-negative st_shndx value." 
> 
> and we ought to stop supporting our current broken interpretation.
> 
> Update processing for dladdr(3) and dladdr1(3) so that SHN_ABS symbols 
> are ignored, because under the corrected interpretation they do not 
> represent addresses within a mapped file and therefore are not supposed 
> to be considered.
> 
> References:
> 
> [1] "System V Application Binary Interface - DRAFT - 19 October 2010",
>     The SCO Group, Section "Symbol Table",
>     <http://www.sco.com/developers/gabi/2012-12-31/ch4.symtab.html>
> 
> [2] Alan Modra, "Absolute symbols" 
>     <https://sourceware.org/ml/binutils/2012-05/msg00019.html>
> 
> [3] Cary Coutant, "Re: Absolute symbols"
>     <https://sourceware.org/ml/binutils/2012-05/msg00020.html>
> 
> 	[BZ #19818]
> 	* sysdeps/generic/ldsodefs.h (SYMBOL_ADDRESS): Handle SHN_ABS 
> 	symbols.
> 	* elf/dl-addr.c (determine_info): Ignore SHN_ABS symbols.
> 	* elf/tst-absolute-sym.c: New file.
> 	* elf/tst-absolute-sym-lib.c: New file.
> 	* elf/tst-absolute-sym-lib.lds: New file.
> 	* elf/Makefile (tests): Add `tst-absolute-sym'.
> 	(modules-names): Add `tst-absolute-sym-lib'.
> 	(LDLIBS-tst-absolute-sym-lib.so): New variable.
> 	($(objpfx)tst-absolute-sym-lib.so): New dependency.
> 	($(objpfx)tst-absolute-sym): New dependency.

The patch and rationale looks reasonable, Andrea and Florian do you still have
objections or doubts about the patch?
Florian Weimer March 8, 2018, 3:40 p.m. UTC | #2
On 03/07/2018 09:25 PM, Adhemerval Zanella wrote:

> The patch and rationale looks reasonable, Andrea and Florian do you still have
> objections or doubts about the patch?

I have no further comments/objections.

Thanks,
Florian
Maciej W. Rozycki April 4, 2018, 10:19 p.m. UTC | #3
On Thu, 8 Mar 2018, Florian Weimer wrote:

> > The patch and rationale looks reasonable, Andrea and Florian do you still
> > have
> > objections or doubts about the patch?
> 
> I have no further comments/objections.

 Committed now and bug closed.  Thanks to everyone involved.

  Maciej

Patch
diff mbox series

Index: glibc/elf/Makefile
===================================================================
--- glibc.orig/elf/Makefile	2018-02-20 11:35:52.000000000 +0000
+++ glibc/elf/Makefile	2018-02-20 16:34:25.160539142 +0000
@@ -187,7 +187,7 @@  tests += restest1 preloadtest loadfail m
 	 tst-tlsalign tst-tlsalign-extern tst-nodelete-opened \
 	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
 	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
-	 tst-debug1 tst-main1
+	 tst-debug1 tst-main1 tst-absolute-sym
 #	 reldep9
 tests-internal += loadtest unload unload2 circleload1 \
 	 neededtest neededtest2 neededtest3 neededtest4 \
@@ -272,7 +272,7 @@  modules-names = testobj1 testobj2 testob
 		tst-audit12mod1 tst-audit12mod2 tst-audit12mod3 tst-auditmod12 \
 		tst-latepthreadmod $(tst-tls-many-dynamic-modules) \
 		tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
-		tst-main1mod tst-libc_dlvsym-dso
+		tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib
 ifeq (yes,$(have-mtls-dialect-gnu2))
 tests += tst-gnu2-tls1
 modules-names += tst-gnu2-tls1mod
@@ -1437,6 +1437,10 @@  tst-main1-no-pie = yes
 LDLIBS-tst-main1 = $(libsupport)
 tst-main1mod.so-no-z-defs = yes
 
+LDLIBS-tst-absolute-sym-lib.so = tst-absolute-sym-lib.lds
+$(objpfx)tst-absolute-sym-lib.so: $(LDLIBS-tst-absolute-sym-lib.so)
+$(objpfx)tst-absolute-sym: $(objpfx)tst-absolute-sym-lib.so
+
 # Both the main program and the DSO for tst-libc_dlvsym need to link
 # against libdl.
 $(objpfx)tst-libc_dlvsym: $(libdl)
Index: glibc/elf/dl-addr.c
===================================================================
--- glibc.orig/elf/dl-addr.c	2018-02-20 11:35:52.000000000 +0000
+++ glibc/elf/dl-addr.c	2018-02-20 16:34:25.163584547 +0000
@@ -59,6 +59,7 @@  determine_info (const ElfW(Addr) addr, s
 		     we can omit that test here.  */
 		  if ((symtab[symndx].st_shndx != SHN_UNDEF
 		       || symtab[symndx].st_value != 0)
+		      && symtab[symndx].st_shndx != SHN_ABS
 		      && ELFW(ST_TYPE) (symtab[symndx].st_info) != STT_TLS
 		      && DL_ADDR_SYM_MATCH (match, &symtab[symndx],
 					    matchsym, addr)
@@ -91,6 +92,7 @@  determine_info (const ElfW(Addr) addr, s
 	    && ELFW(ST_TYPE) (symtab->st_info) != STT_TLS
 	    && (symtab->st_shndx != SHN_UNDEF
 		|| symtab->st_value != 0)
+	    && symtab->st_shndx != SHN_ABS
 	    && DL_ADDR_SYM_MATCH (match, symtab, matchsym, addr)
 	    && symtab->st_name < strtabsize)
 	  matchsym = (ElfW(Sym) *) symtab;
Index: glibc/elf/tst-absolute-sym-lib.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc/elf/tst-absolute-sym-lib.c	2018-02-20 16:34:25.185804348 +0000
@@ -0,0 +1,25 @@ 
+/* BZ #19818 absolute symbol calculation shared module.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+extern char absolute;
+
+void *
+get_absolute (void)
+{
+  return &absolute;
+}
Index: glibc/elf/tst-absolute-sym-lib.lds
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc/elf/tst-absolute-sym-lib.lds	2018-02-20 16:34:25.204877904 +0000
@@ -0,0 +1,19 @@ 
+/* BZ #19818 absolute symbol calculation linker script.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+"absolute" = 0x55aa;
Index: glibc/elf/tst-absolute-sym.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ glibc/elf/tst-absolute-sym.c	2018-02-20 16:34:25.232178264 +0000
@@ -0,0 +1,38 @@ 
+/* BZ #19818 absolute symbol calculation main executable.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <support/check.h>
+#include <support/support.h>
+#include <support/test-driver.h>
+
+void *get_absolute (void);
+
+static int
+do_test (void)
+{
+  void *ref = (void *) 0x55aa;
+  void *ptr;
+
+  ptr = get_absolute ();
+  if (ptr != ref)
+    FAIL_EXIT1 ("Got %p, expected %p\n", ptr, ref);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
Index: glibc/sysdeps/generic/ldsodefs.h
===================================================================
--- glibc.orig/sysdeps/generic/ldsodefs.h	2018-02-20 16:32:02.000000000 +0000
+++ glibc/sysdeps/generic/ldsodefs.h	2018-02-20 16:34:25.271185000 +0000
@@ -72,7 +72,8 @@  typedef struct link_map *lookup_t;
    if non-NULL.  Don't check for NULL map if MAP_SET is TRUE.  */
 #define SYMBOL_ADDRESS(map, ref, map_set)				\
   ((ref) == NULL ? 0							\
-   : LOOKUP_VALUE_ADDRESS (map, map_set) + (ref)->st_value)
+   : (__glibc_unlikely ((ref)->st_shndx == SHN_ABS) ? 0			\
+      : LOOKUP_VALUE_ADDRESS (map, map_set)) + (ref)->st_value)
 
 /* On some architectures a pointer to a function is not just a pointer
    to the actual code of the function but rather an architecture