Patchwork Fix DSTs with static applications (Bug 23462).

login
register
mail settings
Submitter Carlos O'Donell
Date July 28, 2018, 1:50 a.m.
Message ID <bc84a13f-855b-86ef-7e48-63fc6e68a427@redhat.com>
Download mbox | patch
Permalink /patch/28666/
State New
Headers show

Comments

Carlos O'Donell - July 28, 2018, 1:50 a.m.
After commit d1d5471579eb0426671bf94f2d71e61dfb204c30 we no longer check
to see if the link map passed to DL_DST_REQUIRED is NULL or not.  This
is a problem becuase in _dl_init_paths we do not set the link map to
anything in the !SHARED case, and this is a bug.  With the work done to
setup link maps in both static and shared builds we can always rely on
the link map.  The fix is to always set the link map, and assert that it
is not NULL.  Setting the link map allows the use of _dl_main_map link
map to be used and results in DSTs being correctly processed.  We add
one regression test tst-dst-static.c just to look for the crash, but we
need to add a larger set of tests for DSTs, but as noted in the DST
cleanup changes this needs the test-in-container framework to go in
first.

No regressions on x86_64.

Signed-off-by: Carlos O'Donell <carlos@redhat.com>

OK for 2.29 when it opens? I don't want to touch 2.28 with this change,
and it's a long standing bug. I'd like to test this change out in Fedora
Rawhide for a while to make sure I didn't miss anything.
Carlos O'Donell - Aug. 2, 2018, 7:13 p.m.
On 07/27/2018 09:50 PM, Carlos O'Donell wrote:
> After commit d1d5471579eb0426671bf94f2d71e61dfb204c30 we no longer check
> to see if the link map passed to DL_DST_REQUIRED is NULL or not.  This
> is a problem becuase in _dl_init_paths we do not set the link map to
> anything in the !SHARED case, and this is a bug.  With the work done to
> setup link maps in both static and shared builds we can always rely on
> the link map.  The fix is to always set the link map, and assert that it
> is not NULL.  Setting the link map allows the use of _dl_main_map link
> map to be used and results in DSTs being correctly processed.  We add
> one regression test tst-dst-static.c just to look for the crash, but we
> need to add a larger set of tests for DSTs, but as noted in the DST
> cleanup changes this needs the test-in-container framework to go in
> first.
> 
> No regressions on x86_64.
> 
> Signed-off-by: Carlos O'Donell <carlos@redhat.com>
> 
> OK for 2.29 when it opens? I don't want to touch 2.28 with this change,
> and it's a long standing bug. I'd like to test this change out in Fedora
> Rawhide for a while to make sure I didn't miss anything.
> 

Ping?

https://www.sourceware.org/ml/libc-alpha/2018-07/msg01007.html

I'm going to commit this on Friday is nobody objects. It's the cleanest
and most straight forward solution for this problem.

Patch

From 4383cb3f50ccddd2af45f21b78ea2d519689d34c Mon Sep 17 00:00:00 2001
From: Carlos O'Donell <carlos@redhat.com>
Date: Fri, 27 Jul 2018 21:40:37 -0400
Subject: [PATCH] Fix DSTs with static applications (Bug 23462).

After commit d1d5471579eb0426671bf94f2d71e61dfb204c30 we no longer check
to see if the link map passed to DL_DST_REQUIRED is NULL or not.  This
is a problem becuase in _dl_init_paths we do not set the link map to
anything in the !SHARED case, and this is a bug.  With the work done to
setup link maps in both static and shared builds we can always rely on
the link map.  The fix is to always set the link map, and assert that it
is not NULL.  Setting the link map allows the use of _dl_main_map link
map to be used and results in DSTs being correctly processed.  We add
one regression test tst-dst-static.c just to look for the crash, but we
need to add a larger set of tests for DSTs, but as noted in the DST
cleanup changes this needs the test-in-container framework to go in
first.

No regressions on x86_64.

Signed-off-by: Carlos O'Donell <carlos@redhat.com>
---
 ChangeLog            |  7 ++++++
 elf/Makefile         |  7 +++++-
 elf/dl-load.c        | 68 ++++++++++++++++++++++++++--------------------------
 elf/tst-dst-static.c | 27 +++++++++++++++++++++
 4 files changed, 74 insertions(+), 35 deletions(-)
 create mode 100644 elf/tst-dst-static.c

diff --git a/ChangeLog b/ChangeLog
index e5abf96411..03fbd9dd63 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@ 
 2018-07-27  Carlos O'Donell  <carlos@redhat.com>
 
+	[BZ# 23462]
+	* elf/Makefile (tests-static-normal): tst-dst-static.
+	(tst-dst-static-ENV): Define.
+	* elf/tst-dst-static.c: New file.
+	* elf/dl-load.c (_dl_init_paths): Always set l. Assume l is never
+	NULL, assert that.
+
 	* po/uk.po: Update translations.
 	* po/cs.po: Likewise.
 	* po/pl.po: Likewise.
diff --git a/elf/Makefile b/elf/Makefile
index cd0771307f..dac5599756 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -147,7 +147,8 @@  endif
 tests-static-normal := tst-leaks1-static tst-array1-static tst-array5-static \
 	       tst-dl-iter-static \
 	       tst-tlsalign-static tst-tlsalign-extern-static \
-	       tst-linkall-static tst-env-setuid tst-env-setuid-tunables
+	       tst-linkall-static tst-env-setuid tst-env-setuid-tunables \
+	       tst-dst-static
 tests-static-internal := tst-tls1-static tst-tls2-static \
 	       tst-ptrguard1-static tst-stackguard1-static \
 	       tst-tls1-static-non-pie tst-libc_dlvsym-static
@@ -1484,3 +1485,7 @@  tst-libc_dlvsym-static-ENV = \
 $(objpfx)tst-libc_dlvsym-static.out: $(objpfx)tst-libc_dlvsym-dso.so
 
 $(objpfx)tst-big-note: $(objpfx)tst-big-note-lib.so
+
+# We run tst-dst-static with an LD_LIBRARY_PATH of '$LIB', which before
+# the fix for bug 23462 would cause an immediately segfault.
+tst-dst-static-ENV = LD_LIBRARY_PATH='$$LIB'
diff --git a/elf/dl-load.c b/elf/dl-load.c
index c51e4b3718..5d5953da95 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -748,48 +748,48 @@  _dl_init_paths (const char *llp)
   max_dirnamelen = SYSTEM_DIRS_MAX_LEN;
   *aelem = NULL;
 
-#ifdef SHARED
-  /* This points to the map of the main object.  */
+  /* Map of the main object (even in static executables).  */
   l = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
-  if (l != NULL)
+  assert (l != NULL);
+
+#ifdef SHARED
+  /* Static executable startup should never execute this code.  */
+  assert (l->l_type != lt_loaded);
+
+  if (l->l_info[DT_RUNPATH])
+    {
+      /* Allocate room for the search path and fill in information
+	 from RUNPATH.  */
+      decompose_rpath (&l->l_runpath_dirs,
+		       (const void *) (D_PTR (l, l_info[DT_STRTAB])
+				       + l->l_info[DT_RUNPATH]->d_un.d_val),
+		       l, "RUNPATH");
+      /* During rtld init the memory is allocated by the stub malloc,
+	 prevent any attempt to free it by the normal malloc.  */
+      l->l_runpath_dirs.malloced = 0;
+
+      /* The RPATH is ignored.  */
+      l->l_rpath_dirs.dirs = (void *) -1;
+    }
+  else
     {
-      assert (l->l_type != lt_loaded);
+      l->l_runpath_dirs.dirs = (void *) -1;
 
-      if (l->l_info[DT_RUNPATH])
+      if (l->l_info[DT_RPATH])
 	{
 	  /* Allocate room for the search path and fill in information
-	     from RUNPATH.  */
-	  decompose_rpath (&l->l_runpath_dirs,
+	     from RPATH.  */
+	  decompose_rpath (&l->l_rpath_dirs,
 			   (const void *) (D_PTR (l, l_info[DT_STRTAB])
-					   + l->l_info[DT_RUNPATH]->d_un.d_val),
-			   l, "RUNPATH");
-	  /* During rtld init the memory is allocated by the stub malloc,
-	     prevent any attempt to free it by the normal malloc.  */
-	  l->l_runpath_dirs.malloced = 0;
-
-	  /* The RPATH is ignored.  */
-	  l->l_rpath_dirs.dirs = (void *) -1;
+					   + l->l_info[DT_RPATH]->d_un.d_val),
+			   l, "RPATH");
+	  /* During rtld init the memory is allocated by the stub
+	     malloc, prevent any attempt to free it by the normal
+	     malloc.  */
+	  l->l_rpath_dirs.malloced = 0;
 	}
       else
-	{
-	  l->l_runpath_dirs.dirs = (void *) -1;
-
-	  if (l->l_info[DT_RPATH])
-	    {
-	      /* Allocate room for the search path and fill in information
-		 from RPATH.  */
-	      decompose_rpath (&l->l_rpath_dirs,
-			       (const void *) (D_PTR (l, l_info[DT_STRTAB])
-					       + l->l_info[DT_RPATH]->d_un.d_val),
-			       l, "RPATH");
-	      /* During rtld init the memory is allocated by the stub
-		 malloc, prevent any attempt to free it by the normal
-		 malloc.  */
-	      l->l_rpath_dirs.malloced = 0;
-	    }
-	  else
-	    l->l_rpath_dirs.dirs = (void *) -1;
-	}
+	l->l_rpath_dirs.dirs = (void *) -1;
     }
 #endif	/* SHARED */
 
diff --git a/elf/tst-dst-static.c b/elf/tst-dst-static.c
new file mode 100644
index 0000000000..5ad28ebbdc
--- /dev/null
+++ b/elf/tst-dst-static.c
@@ -0,0 +1,27 @@ 
+/* Verify that statically compiled executables with DSTs don't crash.
+   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/>.  */
+
+static int
+do_test (void)
+{
+  /* Before the fix for bug 23462 a static application run with
+     LD_LIBRARY_PATH='$LIB' would crash at startup.  */
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.14.4