Applied with fixing the __mig_dealloc_reply_port(NULL) cases, thanks!
Sergey Bugaev, le jeu. 13 avril 2023 14:58:12 +0300, a ecrit:
> Now that the signal code no longer accesses it, the only real user of it
> was mig-reply.c, so move the logic for managing the port there.
>
> If we're in SHARED and outside of rtld, we know that __LIBC_NO_TLS ()
> always evaluates to 0, and a TLS reply port will always be used, not
> __hurd_reply_port0. Still, the compiler does not see that
> __hurd_reply_port0 is never used due to its address being taken. To deal
> with this, explicitly compile out __hurd_reply_port0 when we know we
> won't use it.
>
> Also, instead of accessing the port via THREAD_SELF->reply_port, this
> uses THREAD_GETMEM and THREAD_SETMEM directly, avoiding possible
> miscompilations.
>
> Signed-off-by: Sergey Bugaev <buga...@gmail.com>
> ---
> Changes since v1:
> * Only deallocate the port in __mig_dealloc_reply_port if it's the same
> as the stored port, because of signals.
> * Add a comment detailing the reasoning.
> * Add two assertions: that ARG and PORT are exactly the same (unless PORT
> has been invalidated), and that we succeed deallocating PORT.
>
> NOTE: Completely untested! Do your own testing! You have been warned!
>
> hurd/hurd/threadvar.h | 7 ----
> sysdeps/mach/hurd/mig-reply.c | 70 +++++++++++++++++++++++++++++------
> 2 files changed, 58 insertions(+), 19 deletions(-)
>
> diff --git a/hurd/hurd/threadvar.h b/hurd/hurd/threadvar.h
> index f5c6a278..c476d988 100644
> --- a/hurd/hurd/threadvar.h
> +++ b/hurd/hurd/threadvar.h
> @@ -29,11 +29,4 @@
> extern unsigned long int __hurd_sigthread_stack_base;
> extern unsigned long int __hurd_sigthread_stack_end;
>
> -/* Store the MiG reply port reply port until we enable TLS. */
> -extern mach_port_t __hurd_reply_port0;
> -
> -/* This returns either the TLS reply port variable, or a single-thread
> variable
> - when TLS is not initialized yet. */
> -#define __hurd_local_reply_port (*(__LIBC_NO_TLS () ? &__hurd_reply_port0 :
> &THREAD_SELF->reply_port))
> -
> #endif /* hurd/threadvar.h */
> diff --git a/sysdeps/mach/hurd/mig-reply.c b/sysdeps/mach/hurd/mig-reply.c
> index 33ff260e..3fdee80e 100644
> --- a/sysdeps/mach/hurd/mig-reply.c
> +++ b/sysdeps/mach/hurd/mig-reply.c
> @@ -17,22 +17,46 @@
>
> #include <mach.h>
> #include <mach/mig_support.h>
> -#include <hurd/threadvar.h>
> +#include <tls.h>
>
> /* These functions are called by MiG-generated code. */
>
> +#if !defined (SHARED) || IS_IN (rtld)
> mach_port_t __hurd_reply_port0;
> +#endif
> +
> +static mach_port_t
> +get_reply_port (void)
> +{
> +#if !defined (SHARED) || IS_IN (rtld)
> + if (__LIBC_NO_TLS ())
> + return __hurd_reply_port0;
> +#endif
> + return THREAD_GETMEM (THREAD_SELF, reply_port);
> +}
> +
> +static void
> +set_reply_port (mach_port_t port)
> +{
> +#if !defined (SHARED) || IS_IN (rtld)
> + if (__LIBC_NO_TLS ())
> + __hurd_reply_port0 = port;
> + else
> +#endif
> + THREAD_SETMEM (THREAD_SELF, reply_port, port);
> +}
>
> /* Called by MiG to get a reply port. */
> mach_port_t
> __mig_get_reply_port (void)
> {
> - if (__hurd_local_reply_port == MACH_PORT_NULL
> - || (&__hurd_local_reply_port != &__hurd_reply_port0
> - && __hurd_local_reply_port == __hurd_reply_port0))
> - __hurd_local_reply_port = __mach_reply_port ();
> -
> - return __hurd_local_reply_port;
> + mach_port_t port = get_reply_port ();
> + if (__glibc_unlikely (port == MACH_PORT_NULL))
> + {
> + port = __mach_reply_port ();
> + set_reply_port (port);
> + }
> + return port;
> }
> weak_alias (__mig_get_reply_port, mig_get_reply_port)
> libc_hidden_def (__mig_get_reply_port)
> @@ -41,12 +65,34 @@ libc_hidden_def (__mig_get_reply_port)
> void
> __mig_dealloc_reply_port (mach_port_t arg)
> {
> - mach_port_t port = __hurd_local_reply_port;
> - __hurd_local_reply_port = MACH_PORT_NULL; /* So the mod_refs RPC won't
> use it. */
> + error_t err;
> + mach_port_t port = get_reply_port ();
> +
> + set_reply_port (MACH_PORT_NULL); /* So the mod_refs RPC won't use it. */
> +
> + /* Normally, ARG should be the same as PORT that we store. However, if a
> + signal has interrupted the RPC, the stored PORT has been deallocated and
> + reset to MACH_PORT_NULL (or possibly MACH_PORT_DEAD). In this case the
> + MIG routine still has the old name, which it passes to us here. We must
> + not deallocate (or otherwise touch) it, since it may be already
> allocated
> + to another port right. Fortunately MIG itself doesn't do anything with
> + the reply port on errors either, other than immediately calling this
> + function.
> +
> + And so:
> + 1. Assert that things are sane, i.e. and PORT is either invalid or same
> + as ARG.
> + 2. Only deallocate the name if our stored PORT still names it. In that
> + case we're sure the right has not been deallocated / the name reused.
> + */
> +
> + if (!MACH_PORT_VALID (port))
> + return;
> + assert (port == arg);
>
> - if (MACH_PORT_VALID (port))
> - __mach_port_mod_refs (__mach_task_self (), port,
> - MACH_PORT_RIGHT_RECEIVE, -1);
> + err = __mach_port_mod_refs (__mach_task_self (), port,
> + MACH_PORT_RIGHT_RECEIVE, -1);
> + assert_perror (err);
> }
> weak_alias (__mig_dealloc_reply_port, mig_dealloc_reply_port)
> libc_hidden_def (__mig_dealloc_reply_port)
> --
> 2.39.2
>
--
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.
