Re: [RFC PATCH glibc 25/34] hurd: Improve reply port handling when exiting signal handlers

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le dim. 19 mars 2023 18:10:08 +0300, a ecrit:
> NOTE: I don't really understand why sigunwind wants to destroy both its
> current reply port and user's reply port. Prior to commit
> fb304035c41c7ee2afede51e5e8568974549ba5e, it was *restoring* the user's
> reply port, in which case it actually made sense to destroy the current
> reply port. Post-fb304035c41c7ee2afede51e5e8568974549ba5e, wouldn't it
> be better to just keep using the current reply port, destroying the
> user's one?

We could try that, yes.

I tend to be very cautious with reply port reuse since it can confuse
servers a lot when e.g. interrupted, so it's generally safer not to try
to reuse them.

>  hurd/sigunwind.c                   | 24 +++++++++++-------------
>  sysdeps/mach/hurd/i386/sigreturn.c | 21 +++++----------------
>  2 files changed, 16 insertions(+), 29 deletions(-)
> 
> diff --git a/hurd/sigunwind.c b/hurd/sigunwind.c
> index edd40b14..399e6900 100644
> --- a/hurd/sigunwind.c
> +++ b/hurd/sigunwind.c
> @@ -18,7 +18,6 @@
>  
>  #include <hurd.h>
>  #include <thread_state.h>
> -#include <hurd/threadvar.h>
>  #include <jmpbuf-unwind.h>
>  #include <assert.h>
>  #include <stdint.h>
> @@ -39,19 +38,18 @@ _hurdsig_longjmp_from_handler (void *data, jmp_buf env, 
> int val)
>      {
>        /* Destroy the MiG reply port used by the signal handler, and restore
>        the reply port in use by the thread when interrupted.  */
> -      mach_port_t *reply_port = &__hurd_local_reply_port;
> -      if (*reply_port)
> -     {
> -       mach_port_t port = *reply_port;
> -       /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port
> -          not to get another reply port, but avoids mig_dealloc_reply_port
> -          trying to deallocate it after the receive fails (which it will,
> -          because the reply port will be bogus, regardless).  */
> -       *reply_port = MACH_PORT_DEAD;
> -       __mach_port_destroy (__mach_task_self (), port);
> -     }
> +      mach_port_t reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
> +      /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port not to
> +         get another reply port, but avoids mig_dealloc_reply_port trying to
> +         deallocate it after the receive fails (which it will, because the
> +         reply port will be bogus, regardless).  */
> +      THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD);
> +      if (MACH_PORT_VALID (reply_port))
> +        __mach_port_mod_refs (__mach_task_self (), reply_port,
> +                              MACH_PORT_RIGHT_RECEIVE, -1);
>        if (scp->sc_reply_port)
> -     __mach_port_destroy (__mach_task_self (), scp->sc_reply_port);
> +        __mach_port_mod_refs (__mach_task_self (), scp->sc_reply_port,
> +                              MACH_PORT_RIGHT_RECEIVE, -1);
>      }
>  
>    __spin_lock (&ss->lock);
> diff --git a/sysdeps/mach/hurd/i386/sigreturn.c 
> b/sysdeps/mach/hurd/i386/sigreturn.c
> index db1a01f3..29c9629f 100644
> --- a/sysdeps/mach/hurd/i386/sigreturn.c
> +++ b/sysdeps/mach/hurd/i386/sigreturn.c
> @@ -19,7 +19,6 @@ register int *sp asm ("%esp");
>  
>  #include <hurd.h>
>  #include <hurd/signal.h>
> -#include <hurd/threadvar.h>
>  #include <hurd/msg.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -59,7 +58,7 @@ __sigreturn (struct sigcontext *scp)
>  {
>    struct hurd_sigstate *ss;
>    struct hurd_userlink *link = (void *) &scp[1];
> -  mach_port_t *reply_port;
> +  mach_port_t reply_port;
>  
>    if (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK))
>      {
> @@ -101,20 +100,10 @@ __sigreturn (struct sigcontext *scp)
>  
>    /* Destroy the MiG reply port used by the signal handler, and restore the
>       reply port in use by the thread when interrupted.  */
> -  reply_port = &__hurd_local_reply_port;
> -  if (*reply_port)
> -    {
> -      mach_port_t port = *reply_port;
> -
> -      /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port not to
> -      get another reply port, but avoids mig_dealloc_reply_port trying to
> -      deallocate it after the receive fails (which it will, because the
> -      reply port will be bogus, whether we do this or not).  */
> -      *reply_port = MACH_PORT_DEAD;
> -
> -      __mach_port_destroy (__mach_task_self (), port);
> -    }
> -  *reply_port = scp->sc_reply_port;
> +  reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
> +  THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
> +  __mach_port_mod_refs (__mach_task_self (), reply_port,
> +                        MACH_PORT_RIGHT_RECEIVE, -1);
>  
>    if (scp->sc_fpused)
>      /* Restore the FPU state.  Mach conveniently stores the state
> -- 
> 2.39.2
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.