* Re: [PATCH] elf: Do not completely clear reused namespace in dlmopen (bug 29600)
2022-09-22 17:26 [PATCH] elf: Do not completely clear reused namespace in dlmopen (bug 29600) Florian Weimer
2022-10-11 7:54 ` Florian Weimer
@ 2022-10-13 20:45 ` Carlos O'Donell
1 sibling, 0 replies; 3+ messages in thread
From: Carlos O'Donell @ 2022-10-13 20:45 UTC (permalink / raw)
To: Florian Weimer, libc-alpha, Adhemerval Zanella
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
>
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 3+ messages in thread