This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Call _dl_open_check after relocation is finished [BZ #24259]
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Florian Weimer <fweimer at redhat dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Mon, 4 Mar 2019 05:59:20 -0800
- Subject: Re: [PATCH] Call _dl_open_check after relocation is finished [BZ #24259]
- References: <20190224160159.1804-1-hjl.tools@gmail.com> <87bm2qls3q.fsf@oldenburg2.str.redhat.com>
On Mon, Mar 4, 2019 at 4:52 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > diff --git a/elf/dl-open.c b/elf/dl-open.c
> > index 12a4f8b853..a62cb91975 100644
> > --- a/elf/dl-open.c
> > +++ b/elf/dl-open.c
> > @@ -292,8 +292,6 @@ dl_open_worker (void *a)
> > _dl_debug_state ();
> > LIBC_PROBE (map_complete, 3, args->nsid, r, new);
> >
> > - _dl_open_check (new);
> > -
> > /* Print scope information. */
> > if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_SCOPES))
> > _dl_show_scope (new, 0);
> > @@ -366,6 +364,11 @@ dl_open_worker (void *a)
> > _dl_relocate_object (l, l->l_scope, reloc_mode, 0);
> > }
> >
> > + /* NB: Since _dl_open_check may throw an exception, it must be called
> > + after relocation is finished. Otherwise, a shared object may be
> > + mapped without relocation. */
> > + _dl_open_check (new);
> > +
> > /* If the file is not loaded now as a dependency, add the search
> > list of the newly loaded object to the scope. */
> > bool any_tls = false;
>
> If you report the error at this, doesn't this mean the object is still
> around and in a bad state? This looks related to this bug:
Yes.
> <https://sourceware.org/bugzilla/show_bug.cgi?id=20839>
>
> Would the CET bug go away if we got rid after the object without trace
> after a failure in _dl_open_check?
Yes.
> I can look into fixing the other bug, but I don't know how hard that's
> going to be.
>
> > diff --git a/sysdeps/x86/tst-cet-legacy-5.c b/sysdeps/x86/tst-cet-legacy-5.c
> > new file mode 100644
> > index 0000000000..fbf640f664
> > --- /dev/null
> > +++ b/sysdeps/x86/tst-cet-legacy-5.c
>
> > +static void
> > +do_test_1 (const char *modname, bool fail)
> > +{
> > + int (*fp) (void);
> > + void *h;
> > +
> > + h = dlopen (modname, RTLD_LAZY);
> > + if (h == NULL)
> > + {
> > + if (fail)
> > + {
> > + const char *err = dlerror ();
> > + if (strstr (err, "shadow stack isn't enabled") == NULL)
> > + {
> > + printf ("incorrect dlopen '%s' error: %s\n", modname,
> > + dlerror ());
> > + exit (1);
> > + }
> > +
> > + return;
> > + }
>
> Is the return supposed to be taken if running on non-CET hardware? I'm
> looking for the UNSUPPORTED case.
This path is taken only on CET hardware. For non-CET hardware, 'h' shouldn't
be NULL.
> > diff --git a/sysdeps/x86/tst-cet-legacy-mod-5.c b/sysdeps/x86/tst-cet-legacy-mod-5.c
> > new file mode 100644
> > index 0000000000..38d0aaa727
> > --- /dev/null
> > +++ b/sysdeps/x86/tst-cet-legacy-mod-5.c
> > @@ -0,0 +1,91 @@
>
> > + /* This test will fail on spurious wake-ups, which are allowed; however,
> > + the current implementation shouldn't produce spurious wake-ups in the
> > + scenario we are testing here. */
> > + err = pthread_cond_wait (&cond, &mut);
> > + if (err != 0)
> > + error (EXIT_FAILURE, err, "parent: cannot wait fir signal");
>
> You can use xpthread_barrier_wait et al., which doesn't have spurious
> wake-ups. The existing wrappers should make the switch pretty easy.
>
I will update.
Thanks.
--
H.J.