elf: Do not completely clear reused namespace in dlmopen (bug 29600)

The data in the _ns_debug member must be preserved, otherwise
_dl_debug_initialize enters an infinite loop.  To be conservative,
only clear the libc_map member for now, to fix bug 29528.

Fixes commit d0e357ff45a75553dee3b17ed7d303bfa544f6fe
("elf: Call __libc_early_init for reused namespaces (bug 29528)"),
by reverting most of it.

Tested on i686-linux-gnu and x86_64-linux-gnu.

---
 elf/dl-open.c           | 14 ++++++--------
 elf/tst-dlmopen-twice.c | 28 ++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 12 deletions(-)


base-commit: 340097d0b50eff9d3058e06c6989ae398c653d4a

* Florian Weimer via Libc-alpha:

> The data in the _ns_debug member must be preserved, otherwise
> _dl_debug_initialize enters an infinite loop.  To be conservative,
> only clear the libc_map member for now, to fix bug 29528.
>
> Fixes commit d0e357ff45a75553dee3b17ed7d303bfa544f6fe
> ("elf: Call __libc_early_init for reused namespaces (bug 29528)"),
> by reverting most of it.
>
> Tested on i686-linux-gnu and x86_64-linux-gnu.

Ping.  This patch needs review:

  <https://sourceware.org/pipermail/libc-alpha/2022-September/142199.html>

Thanks,
Florian

On 9/22/22 13:26, Florian Weimer via Libc-alpha wrote:
> The data in the _ns_debug member must be preserved, otherwise
> _dl_debug_initialize enters an infinite loop.  To be conservative,
> only clear the libc_map member for now, to fix bug 29528.
> 
> Fixes commit d0e357ff45a75553dee3b17ed7d303bfa544f6fe
> ("elf: Call __libc_early_init for reused namespaces (bug 29528)"),
> by reverting most of it.
> 
> Tested on i686-linux-gnu and x86_64-linux-gnu.

Adhemerval had the review status on this patch in patchwork, but I'm CC'ing him
here to notify him that I've done a review.

I had to do more than average due-diligence here since I was concerned about
the namespace reuse and what might need to be initialized.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/dl-open.c           | 14 ++++++--------
>  elf/tst-dlmopen-twice.c | 28 ++++++++++++++++++++++++----
>  2 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 46e8066fd8..e7db5e9642 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -844,15 +844,13 @@ _dl_open (const char *file, int mode, const void *caller_dlopen, Lmid_t nsid,
>  	  _dl_signal_error (EINVAL, file, NULL, N_("\
>  no more namespaces available for dlmopen()"));
>  	}
> +      else if (nsid == GL(dl_nns))
> +	{
> +	  __rtld_lock_initialize (GL(dl_ns)[nsid]._ns_unique_sym_table.lock);
> +	  ++GL(dl_nns);
> +	}

OK. We are in the LM_ID_NEWLM case. A new namespace is being created.

OK. We reinitialize _ns_unique_sym_table.lock (one of two things)

>  
> -      if (nsid == GL(dl_nns))
> -	++GL(dl_nns);
> -
> -      /* Initialize the new namespace.  Most members are
> -	 zero-initialized, only the lock needs special treatment.  */
> -      memset (&GL(dl_ns)[nsid], 0, sizeof (GL(dl_ns)[nsid]));

OK. We don't zero the namespace structure. We want our own r_brk to remain non-null.

> -      __rtld_lock_initialize (GL(dl_ns)[nsid]._ns_unique_sym_table.lock);
> -
> +      GL(dl_ns)[nsid].libc_map = NULL;

OK. Reset libc_map to NULL in order that dl_open_worker_begin can re-inint libc if needed (two of two things).

>        _dl_debug_update (nsid)->r_state = RT_CONSISTENT;
>      }
>    /* Never allow loading a DSO in a namespace which is empty.  Such

I wanted to see the infinite loop for myself to be able to review this patch,
so I backed out the change and debugged tst-dlmopen-twice.

I confirmed that the loop is in the debug initialization of _r_debug_extended:

#0  _dl_debug_initialize (ldbase=ldbase@entry=0, ns=ns@entry=2) at dl-debug.c:77
#1  0x00007ffff7fd66c5 in dl_open_worker_begin (a=a@entry=0x7fffffffd1c0) at dl-open.c:530
#2  0x00007ffff7f26909 in __GI__dl_catch_exception (exception=<optimized out>, operate=<optimized out>, args=<optimized out>)
    at /mnt/ssd/carlos/src/glibc-review/elf/dl-error-skeleton.c:208
#3  0x00007ffff7fd5ea6 in dl_open_worker (a=a@entry=0x7fffffffd1c0) at dl-open.c:782
#4  0x00007ffff7f26909 in __GI__dl_catch_exception (exception=<optimized out>, operate=<optimized out>, args=<optimized out>)
    at /mnt/ssd/carlos/src/glibc-review/elf/dl-error-skeleton.c:208
#5  0x00007ffff7fd6288 in _dl_open (file=<optimized out>, mode=<optimized out>, caller_dlopen=0x7ffff7fc05b9, nsid=2, argc=2, 
    argv=0x7fffffffd698, env=0x7fffffffd6b0) at dl-open.c:886
#6  0x00007ffff7e62b80 in dlmopen_doit (a=a@entry=0x7fffffffd430) at dlmopen.c:61
#7  0x00007ffff7f26909 in __GI__dl_catch_exception (exception=exception@entry=0x7fffffffd390, operate=<optimized out>, 
    args=<optimized out>) at /mnt/ssd/carlos/src/glibc-review/elf/dl-error-skeleton.c:208
#8  0x00007ffff7f269af in __GI__dl_catch_error (objname=0x7fffffffd3f0, errstring=0x7fffffffd3f8, mallocedp=0x7fffffffd3ef, 
    operate=<optimized out>, args=<optimized out>) at /mnt/ssd/carlos/src/glibc-review/elf/dl-error-skeleton.c:227
#9  0x00007ffff7e62826 in _dlerror_run (operate=operate@entry=0x7ffff7e62b20 <dlmopen_doit>, args=args@entry=0x7fffffffd430)
    at dlerror.c:138
#10 0x00007ffff7e62c76 in dlmopen_implementation (dl_caller=<optimized out>, mode=<optimized out>, file=<optimized out>, 
    nsid=<optimized out>) at dlmopen.c:76
#11 ___dlmopen (nsid=<optimized out>, file=<optimized out>, mode=<optimized out>) at dlmopen.c:86

 66   else if (DL_NNS > 1)
 67     {
 68       r = &GL(dl_ns)[ns]._ns_debug;
 69       if (r->base.r_brk == 0)
 70         {
 71           /* Add the new namespace to the linked list.  After a namespace
 72              is initialized, r_brk becomes non-zero.  A namespace becomes
 73              empty (r_map == NULL) when it is unused.  But it is never
 74              removed from the linked list.  */
 75           struct r_debug_extended *p;
 76           for (pp = &_r_debug_extended.r_next;
 77                (p = *pp) != NULL;
 78                pp = &p->r_next)

Here we loop forever because of the self-reference.

(gdb) 
77		       (p = *pp) != NULL;
(gdb) 
78		       pp = &p->r_next)
(gdb) 
77		       (p = *pp) != NULL;
(gdb) 
78		       pp = &p->r_next)
(gdb) 
77		       (p = *pp) != NULL;
(gdb) 
78		       pp = &p->r_next)
....

 79             ;
 80 
 81           r->base.r_version = 2;
 82         }
 83     }

The self-reference is added because a namespace is only added once to the
_r_debug_extended structure, and we need to keep that invariant.

Since the namespace was added once already, when we go to add it again
we create an loop with r_next. AFAICT it is the clearing of r_brk which
breaks the expectation, causes the namespace to be added again, and creates
the loop.

Confirmed that with the patch we correctly initialize _r_debug_extended and
correctly track r_next and add namespaces as they first appear, never
removing them.

> diff --git a/elf/tst-dlmopen-twice.c b/elf/tst-dlmopen-twice.c
> index 449f3c8fa9..70c71fe19c 100644
> --- a/elf/tst-dlmopen-twice.c
> +++ b/elf/tst-dlmopen-twice.c
> @@ -16,18 +16,38 @@

OK. Test looks good, adds recursion to the test to create a dlmopen/dlmclose/dmopen

>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <support/xdlfcn.h>
> +#include <stdio.h>
>  #include <support/check.h>
> +#include <support/xdlfcn.h>
>  
> -static int
> -do_test (void)
> +/* Run the test multiple times, to check finding a new namespace while
> +   another namespace is already in use.  This used to trigger bug 29600.  */
> +static void
> +recurse (int depth)
>  {
> -  void *handle = xdlmopen (LM_ID_NEWLM, "tst-dlmopen-twice-mod1.so", RTLD_NOW);
> +  if (depth == 0)
> +    return;
> +
> +  printf ("info: running at depth %d\n", depth);
> +  void *handle = xdlmopen (LM_ID_NEWLM, "tst-dlmopen-twice-mod1.so",
> +                           RTLD_NOW);
>    xdlclose (handle);
>    handle = xdlmopen (LM_ID_NEWLM, "tst-dlmopen-twice-mod2.so", RTLD_NOW);
>    int (*run_check) (void) = xdlsym (handle, "run_check");
>    TEST_COMPARE (run_check (), 0);
> +  recurse (depth - 1);
>    xdlclose (handle);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  /* First run the test without nesting.  */
> +  recurse (1);
> +
> +  /* Then with nesting.  The constant needs to be less than the
> +     internal DL_NNS namespace constant.  */
> +  recurse (10);
>    return 0;
>  }
>  
> 
> base-commit: 340097d0b50eff9d3058e06c6989ae398c653d4a
>