Patchwork elf: Always set l in _dl_init_paths (Bug 23462).

login
register
mail settings
Submitter Carlos O'Donell
Date Oct. 9, 2019, 12:01 a.m.
Message ID <ca723e73-c8eb-ca4c-1ca0-5fbf2b88edcd@redhat.com>
Download mbox | patch
Permalink /patch/34885/
State New
Headers show

Comments

Carlos O'Donell - Oct. 9, 2019, 12:01 a.m.
I was doing a Fedora triage with Florian and we caught that this
issue had slipped through the cracks. Here is the patch with a
very simple test case.

8< --- 8< --- 8<
After d1d5471579eb0426671bf94f2d71e61dfb204c30 we always setup the
link map l to make the static and shared cases the same.  The bug
is that in elf/dl-load.c (_dl_init_paths) we conditionally set l
only in the #ifdef SHARED case, but unconditionally use it later.
The simple solution is to remove the #ifdef SHARED conditional,
because it's no longer needed, and unconditionally setup l for
both the static and shared cases. A regression test is added to
run a static binary with LD_LIBRARY_PATH='$ORIGIN' which crashes
before the fix and runs after the fix.

Regression tested on x86_64.
---
 ChangeLog            |  9 ++++++
 elf/Makefile         |  5 +++-
 elf/dl-load.c        | 67 +++++++++++++++++++++-----------------------
 elf/tst-dst-static.c | 30 ++++++++++++++++++++
 4 files changed, 75 insertions(+), 36 deletions(-)
 create mode 100644 elf/tst-dst-static.c
Carlos O'Donell - Oct. 15, 2019, 4:23 p.m.
On 10/8/19 8:01 PM, Carlos O'Donell wrote:
> I was doing a Fedora triage with Florian and we caught that this
> issue had slipped through the cracks. Here is the patch with a
> very simple test case.

Ping.

> 8< --- 8< --- 8<
> After d1d5471579eb0426671bf94f2d71e61dfb204c30 we always setup the
> link map l to make the static and shared cases the same.  The bug
> is that in elf/dl-load.c (_dl_init_paths) we conditionally set l
> only in the #ifdef SHARED case, but unconditionally use it later.
> The simple solution is to remove the #ifdef SHARED conditional,
> because it's no longer needed, and unconditionally setup l for
> both the static and shared cases. A regression test is added to
> run a static binary with LD_LIBRARY_PATH='$ORIGIN' which crashes
> before the fix and runs after the fix.
> 
> Regression tested on x86_64.
> ---
>  ChangeLog            |  9 ++++++
>  elf/Makefile         |  5 +++-
>  elf/dl-load.c        | 67 +++++++++++++++++++++-----------------------
>  elf/tst-dst-static.c | 30 ++++++++++++++++++++
>  4 files changed, 75 insertions(+), 36 deletions(-)
>  create mode 100644 elf/tst-dst-static.c
> 
> diff --git a/ChangeLog b/ChangeLog
> index 2ce48223f4..cf198257ba 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,12 @@
> +2019-10-08  Carlos O'Donell  <carlos@redhat.com>
> +
> +	[BZ #23462]
> +	* elf/Makefile (tests-static-normal): tst-dst-static.
> +	(tst-dst-static-ENV): Define.
> +	* elf/dl-load.c (_dl_init_paths): Remove #ifdef SHARED, and refactor
> +	to always assume l is non-NULL.
> +	* elf/tst-dst-static.c: New file.
> +
>  2019-10-07  Carlos O'Donell  <carlos@redhat.com>
>  
>  	* nptl/cancellation.c: Document that all functions here must be
> diff --git a/elf/Makefile b/elf/Makefile
> index dea51ca182..bb55baaa1f 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -148,7 +148,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
> @@ -1557,3 +1558,5 @@ $(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
>  $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
>  
>  CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
> +
> +tst-dst-static-ENV = LD_LIBRARY_PATH='$$ORIGIN'
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 438793e53d..903f8af13a 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -748,50 +748,47 @@ _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.  */
> +  /* This points to the map of the main object.  It is always non-NULL
> +     since we have purposely made the dynamic and static cases look the
> +     same.  */
>    l = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
> -  if (l != NULL)
> +  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 */
>  
>    if (llp != NULL && *llp != '\0')
>      {
> diff --git a/elf/tst-dst-static.c b/elf/tst-dst-static.c
> new file mode 100644
> index 0000000000..7092f24a8e
> --- /dev/null
> +++ b/elf/tst-dst-static.c
> @@ -0,0 +1,30 @@
> +/* Test DST expansion for static binaries doesn't carsh. Bug 23462.
> +   Copyright (C) 2019 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* The purpose of this test is to exercise the code in elf/dl-loac.c
> +   (_dl_init_paths) or thereabout and ensure that static binaries
> +   don't crash when expanding DSTs. */
> +static int
> +do_test (void)
> +{
> +  /* If the dynamic loader code linked into the static binary cannot
> +     handle expanding the DSTs e.g. null-deref on an incomplete link
> +     map, then it will crash before reaching main.  */
> +  return 0;
> +}
> +#include <support/test-driver.c>
>
Florian Weimer - Oct. 16, 2019, 9:54 a.m.
* Carlos O'Donell:

> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 438793e53d..903f8af13a 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -748,50 +748,47 @@ _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.  */
> +  /* This points to the map of the main object.  It is always non-NULL
> +     since we have purposely made the dynamic and static cases look the
> +     same.  */
>    l = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
> -  if (l != NULL)
> +  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;
> +    }

Does this change enable DT_RUNPATH/DT_RPATH for statically linked
binaries?  Should that receive a NEWS entry and a test?

Thanks,
Florian
Carlos O'Donell - Oct. 16, 2019, 11:15 a.m.
On 10/16/19 5:54 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 438793e53d..903f8af13a 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -748,50 +748,47 @@ _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.  */
>> +  /* This points to the map of the main object.  It is always non-NULL
>> +     since we have purposely made the dynamic and static cases look the
>> +     same.  */
>>    l = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
>> -  if (l != NULL)
>> +  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;
>> +    }
> 
> Does this change enable DT_RUNPATH/DT_RPATH for statically linked
> binaries?  Should that receive a NEWS entry and a test?

It does enable DT_RUNPATH/DT_RPATH for statically linked binaries,
and we should probably add a test for that?

This is not a consequence I actively thought too deeply about because
it fell under the "make static binaries behave like dynamic binaries"
which was within our design goals. However, now that you mention it,
it will be a semantic change if anyone has ever built a static
binary with DT_RUNPATH/DT_RPATH, the behaviour will now be changed.

Let me add a NEWS entry for this.

If you think the semantic change here is wrong, please call that out.
Florian Weimer - Oct. 16, 2019, 11:23 a.m.
* Carlos O'Donell:

>> Does this change enable DT_RUNPATH/DT_RPATH for statically linked
>> binaries?  Should that receive a NEWS entry and a test?
>
> It does enable DT_RUNPATH/DT_RPATH for statically linked binaries,
> and we should probably add a test for that?

Right.

> If you think the semantic change here is wrong, please call that out.

No, I think it would be a useful feature.  I sort-of ran into this here:

  resolv/tst-idna_name_classify: Isolate from system libraries
  <https://sourceware.org/ml/libc-alpha/2019-10/msg00338.html>

And I briefly wondered if we should do this automatically for
--enable-hardcoded-path-in-tests.

Thanks,
Florian
Carlos O'Donell - Oct. 16, 2019, 1:31 p.m.
On 10/16/19 7:23 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>> Does this change enable DT_RUNPATH/DT_RPATH for statically linked
>>> binaries?  Should that receive a NEWS entry and a test?
>>
>> It does enable DT_RUNPATH/DT_RPATH for statically linked binaries,
>> and we should probably add a test for that?
> 
> Right.

Perfect, let me do that. I'll add another test.

>> If you think the semantic change here is wrong, please call that out.
> 
> No, I think it would be a useful feature.  I sort-of ran into this here:
> 
>   resolv/tst-idna_name_classify: Isolate from system libraries
>   <https://sourceware.org/ml/libc-alpha/2019-10/msg00338.html>
> 
> And I briefly wondered if we should do this automatically for
> --enable-hardcoded-path-in-tests.
> 
> Thanks,
> Florian
>

Patch

diff --git a/ChangeLog b/ChangeLog
index 2ce48223f4..cf198257ba 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@ 
+2019-10-08  Carlos O'Donell  <carlos@redhat.com>
+
+	[BZ #23462]
+	* elf/Makefile (tests-static-normal): tst-dst-static.
+	(tst-dst-static-ENV): Define.
+	* elf/dl-load.c (_dl_init_paths): Remove #ifdef SHARED, and refactor
+	to always assume l is non-NULL.
+	* elf/tst-dst-static.c: New file.
+
 2019-10-07  Carlos O'Donell  <carlos@redhat.com>
 
 	* nptl/cancellation.c: Document that all functions here must be
diff --git a/elf/Makefile b/elf/Makefile
index dea51ca182..bb55baaa1f 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -148,7 +148,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
@@ -1557,3 +1558,5 @@  $(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
 $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
 
 CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
+
+tst-dst-static-ENV = LD_LIBRARY_PATH='$$ORIGIN'
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 438793e53d..903f8af13a 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -748,50 +748,47 @@  _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.  */
+  /* This points to the map of the main object.  It is always non-NULL
+     since we have purposely made the dynamic and static cases look the
+     same.  */
   l = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
-  if (l != NULL)
+  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 */
 
   if (llp != NULL && *llp != '\0')
     {
diff --git a/elf/tst-dst-static.c b/elf/tst-dst-static.c
new file mode 100644
index 0000000000..7092f24a8e
--- /dev/null
+++ b/elf/tst-dst-static.c
@@ -0,0 +1,30 @@ 
+/* Test DST expansion for static binaries doesn't carsh. Bug 23462.
+   Copyright (C) 2019 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
+   <https://www.gnu.org/licenses/>.  */
+
+/* The purpose of this test is to exercise the code in elf/dl-loac.c
+   (_dl_init_paths) or thereabout and ensure that static binaries
+   don't crash when expanding DSTs. */
+static int
+do_test (void)
+{
+  /* If the dynamic loader code linked into the static binary cannot
+     handle expanding the DSTs e.g. null-deref on an incomplete link
+     map, then it will crash before reaching main.  */
+  return 0;
+}
+#include <support/test-driver.c>