[PATCH] elf: Always set l in _dl_init_paths (bug 23462)

[PATCH] elf: Always set l in _dl_init_paths (bug 23462)

[Florian Weimer]

From: Carlos O'Donell <carlos@redhat.com>

After d1d5471579eb0426671bf94f2d71e61dfb204c30 ("Remove dead
DL_DST_REQ_STATIC code.") 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.

Co-Authored-By: Florian Weimer <fweimer@redhat.com>

---
v2: Adjusted test not to use the test framework.  Redid the change from
    scratch.

    I think we should revisit testing for static DT_RUNPATH support
    later, and not as part of this patch.

 elf/Makefile         |  5 ++++-
 elf/dl-load.c        | 63 ++++++++++++++++++++++++----------------------------
 elf/tst-dst-static.c | 32 ++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 35 deletions(-)

diff --git a/elf/Makefile b/elf/Makefile
index b06bf6ca20..4c9e63dac9 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -164,7 +164,8 @@ 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-single_threaded-static tst-single_threaded-pthread-static
+	       tst-single_threaded-static tst-single_threaded-pthread-static \
+	       tst-dst-static
 
 tests-static-internal := tst-tls1-static tst-tls2-static \
 	       tst-ptrguard1-static tst-stackguard1-static \
@@ -1904,3 +1905,5 @@ $(objpfx)list-tunables.out: tst-rtld-list-tunables.sh $(objpfx)ld.so
 	cmp tst-rtld-list-tunables.exp \
 	    $(objpfx)/tst-rtld-list-tunables.out > $@; \
 	$(evaluate-test)
+
+tst-dst-static-ENV = LD_LIBRARY_PATH='$$ORIGIN'
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 9e2089cfaa..376a2e64d6 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -758,50 +758,45 @@ _dl_init_paths (const char *llp, const char *source,
   max_dirnamelen = SYSTEM_DIRS_MAX_LEN;
   *aelem = NULL;
 
-#ifdef SHARED
   /* This points to the map of the main object.  */
   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..56eb371c96
--- /dev/null
+++ b/elf/tst-dst-static.c
@@ -0,0 +1,32 @@
+/* Test DST expansion for static binaries doesn't carsh.  Bug 23462.
+   Copyright (C) 2021 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.
+
+   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, so the test harness
+   is unnecessary.  */
+
+int
+main (void)
+{
+  return 0;
+}

Re: [PATCH] elf: Always set l in _dl_init_paths (bug 23462)

[Carlos O'Donell]

On 3/5/21 1:25 PM, Florian Weimer wrote:
> From: Carlos O'Donell <carlos@redhat.com>
> 
> After d1d5471579eb0426671bf94f2d71e61dfb204c30 ("Remove dead
> DL_DST_REQ_STATIC code.") 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.
> 
> Co-Authored-By: Florian Weimer <fweimer@redhat.com>

If you have reviewed this patch and find it sufficient this is enough
consensus IMO to commit the patch.

The technical direction was set by earlier patches where we are working
towards making static and dynamic linking look and behave in similar
ways.

Thanks for taking this patch up, working through it, and adjusting the
testing.

Unless anyone objects I'd commit this patch since it fixes a real bug
with ldconfig running when LD_LIBRRAY_PATH is set in some way globally.
 
> ---
> v2: Adjusted test not to use the test framework.  Redid the change from
>     scratch.
> 
>     I think we should revisit testing for static DT_RUNPATH support
>     later, and not as part of this patch.
> 
>  elf/Makefile         |  5 ++++-
>  elf/dl-load.c        | 63 ++++++++++++++++++++++++----------------------------
>  elf/tst-dst-static.c | 32 ++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+), 35 deletions(-)
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index b06bf6ca20..4c9e63dac9 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -164,7 +164,8 @@ 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-single_threaded-static tst-single_threaded-pthread-static
> +	       tst-single_threaded-static tst-single_threaded-pthread-static \
> +	       tst-dst-static
>  
>  tests-static-internal := tst-tls1-static tst-tls2-static \
>  	       tst-ptrguard1-static tst-stackguard1-static \
> @@ -1904,3 +1905,5 @@ $(objpfx)list-tunables.out: tst-rtld-list-tunables.sh $(objpfx)ld.so
>  	cmp tst-rtld-list-tunables.exp \
>  	    $(objpfx)/tst-rtld-list-tunables.out > $@; \
>  	$(evaluate-test)
> +
> +tst-dst-static-ENV = LD_LIBRARY_PATH='$$ORIGIN'
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 9e2089cfaa..376a2e64d6 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -758,50 +758,45 @@ _dl_init_paths (const char *llp, const char *source,
>    max_dirnamelen = SYSTEM_DIRS_MAX_LEN;
>    *aelem = NULL;
>  
> -#ifdef SHARED
>    /* This points to the map of the main object.  */
>    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..56eb371c96
> --- /dev/null
> +++ b/elf/tst-dst-static.c
> @@ -0,0 +1,32 @@
> +/* Test DST expansion for static binaries doesn't carsh.  Bug 23462.
> +   Copyright (C) 2021 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.
> +
> +   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, so the test harness
> +   is unnecessary.  */
> +
> +int
> +main (void)
> +{
> +  return 0;
> +}
> 


-- 
Cheers,
Carlos.

Re: [PATCH] elf: Always set l in _dl_init_paths (bug 23462)

[Navin P]

On Tue, Mar 9, 2021 at 4:42 AM Carlos O'Donell via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On 3/5/21 1:25 PM, Florian Weimer wrote:
> > From: Carlos O'Donell <carlos@redhat.com>
> >
> > After d1d5471579eb0426671bf94f2d71e61dfb204c30 ("Remove dead
> > DL_DST_REQ_STATIC code.") 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.
> >
> > Co-Authored-By: Florian Weimer <fweimer@redhat.com>
>
> If you have reviewed this patch and find it sufficient this is enough
> consensus IMO to commit the patch.
>
> The technical direction was set by earlier patches where we are working
> towards making static and dynamic linking look and behave in similar
> ways.
>
> Thanks for taking this patch up, working through it, and adjusting the
> testing.
>
> Unless anyone objects I'd commit this patch since it fixes a real bug
> with ldconfig running when LD_LIBRRAY_PATH is set in some way globally.
>
> > ---
> > v2: Adjusted test not to use the test framework.  Redid the change from
> >     scratch.
> >
> >     I think we should revisit testing for static DT_RUNPATH support
> >     later, and not as part of this patch.
> >

  Does this fix https://sourceware.org/bugzilla/show_bug.cgi?id=27504 also ?

Re: [PATCH] elf: Always set l in _dl_init_paths (bug 23462)

[Florian Weimer]

* Navin P.:

>   Does this fix https://sourceware.org/bugzilla/show_bug.cgi?id=27504 also ?

No, as discussed on the list, that's not really a bug because libasan
calls dlopen, so its (nonexistent) $ORIGIN value has to be used.

Thanks,
Florian

Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462).

[Carlos O'Donell]

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.

-- 
Cheers,
Carlos.

Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462).

[Florian Weimer]

* 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

Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462).

[Carlos O'Donell]

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?

-- 
Cheers,
Carlos.

Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462).

[Carlos O'Donell]

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
> 


-- 
Cheers,
Carlos.

Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462).

[Florian Weimer]

* 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

Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462).

[Carlos O'Donell]

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.

-- 
Cheers,
Carlos.

Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462).

[Florian Weimer]

* 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

Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462).

[Carlos O'Donell]

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


-- 
Cheers,
Carlos.

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

[Carlos O'Donell]

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

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