Re: [PATCH] Call _dl_open_check after relocation is finished [BZ #24259]

[Florian Weimer <fweimer at redhat dot com>]

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

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

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.

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

Thanks,
Florian