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

  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
  

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>
>
  

* 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
  

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.
  

* 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
  

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
>
  

On 10/16/19 9:31 AM, Carlos O'Donell wrote:
> 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.

This does not turn out to be true. And it makes sense when I think
about it more thoroughly.

The static executable, lacking a .dynamic section, does not fill in
any of the link maps's l_info data, in fact it's all just a list of
NULLs because there was no data to load.

Any subsequently loaded DSO will have such things, but they are already
handled by the DSO loading code.

You would have to assemble a static binary with a .dynamic section
and the static linker doesn't want to do that.

Therefore it's not a new feature.

We would have to add a LD_RUNPATH or LD_RPATH to feed this data into
the binary via an environment variable.

>>> 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.

Given the above, do you think the original patch is good as-is?

Did I miss something else?
  

* Carlos O'Donell:

> This does not turn out to be true. And it makes sense when I think
> about it more thoroughly.
>
> The static executable, lacking a .dynamic section, does not fill in
> any of the link maps's l_info data, in fact it's all just a list of
> NULLs because there was no data to load.

Statically linked executables can have dynamic segments.  However, there
seems to be a bug in BFD ld because it ignores -rpath for them.

> You would have to assemble a static binary with a .dynamic section
> and the static linker doesn't want to do that.

You can use -static-pie on some targets, then there is a dynamic
segment, and BFD ld adds DT_RPATH to it.

Thanks,
Florian
  

On 10/25/19 4:36 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> This does not turn out to be true. And it makes sense when I think
>> about it more thoroughly.
>>
>> The static executable, lacking a .dynamic section, does not fill in
>> any of the link maps's l_info data, in fact it's all just a list of
>> NULLs because there was no data to load.
> 
> Statically linked executables can have dynamic segments.  However, there
> seems to be a bug in BFD ld because it ignores -rpath for them.

OK, so we assume you can assemble a minimum .dynamic in a static 
executable with a DT_RUNPATH or DT_RPATH that will impact a dlopen
made from a static executable?

Did you file a binutils bug for this?

>> You would have to assemble a static binary with a .dynamic section
>> and the static linker doesn't want to do that.
> 
> You can use -static-pie on some targets, then there is a dynamic
> segment, and BFD ld adds DT_RPATH to it.

Oh! That's a great idea, let me add a test case conditioned on that
then, since we have all those pieces in place.