public inbox for glibc-cvs@sourceware.org
help / color / mirror / Atom feed
* [glibc] hurd: Remove __hurd_local_reply_port
@ 2023-04-14 17:32 Samuel Thibault
  0 siblings, 0 replies; only message in thread
From: Samuel Thibault @ 2023-04-14 17:32 UTC (permalink / raw)
  To: glibc-cvs

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=ba00d787f3469b02032766b074d4df9071fa7e24

commit ba00d787f3469b02032766b074d4df9071fa7e24
Author: Sergey Bugaev <bugaevc@gmail.com>
Date:   Thu Apr 13 14:58:12 2023 +0300

    hurd: Remove __hurd_local_reply_port
    
    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 <bugaevc@gmail.com>

Diff:
---
 hurd/hurd/threadvar.h         |  7 -----
 sysdeps/mach/hurd/dl-sysdep.c |  2 +-
 sysdeps/mach/hurd/mig-reply.c | 70 +++++++++++++++++++++++++++++++++++--------
 sysdeps/mach/sysdep.h         |  2 +-
 4 files changed, 60 insertions(+), 21 deletions(-)

diff --git a/hurd/hurd/threadvar.h b/hurd/hurd/threadvar.h
index f5c6a278a6..c476d98880 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/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
index a2115f6edb..2d595d0006 100644
--- a/sysdeps/mach/hurd/dl-sysdep.c
+++ b/sysdeps/mach/hurd/dl-sysdep.c
@@ -238,7 +238,7 @@ _dl_sysdep_start_cleanup (void)
   /* Deallocate the reply port and task port rights acquired by
      __mach_init.  We are done with them now, and the user will
      reacquire them for himself when he wants them.  */
-  __mig_dealloc_reply_port (MACH_PORT_NULL);
+  __mig_dealloc_reply_port (__mig_get_reply_port ());
   __mach_port_deallocate (__mach_task_self (), __mach_host_self_);
   __mach_port_deallocate (__mach_task_self (), __mach_task_self_);
 }
diff --git a/sysdeps/mach/hurd/mig-reply.c b/sysdeps/mach/hurd/mig-reply.c
index 33ff260e36..3fdee80e5a 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)
diff --git a/sysdeps/mach/sysdep.h b/sysdeps/mach/sysdep.h
index 1c869f5cb2..35ad1e3920 100644
--- a/sysdeps/mach/sysdep.h
+++ b/sysdeps/mach/sysdep.h
@@ -45,7 +45,7 @@
 
 #ifndef __ASSEMBLER__
 #define FATAL_PREPARE_INCLUDE <mach/mig_support.h>
-#define FATAL_PREPARE __mig_dealloc_reply_port (MACH_PORT_NULL)
+#define FATAL_PREPARE __mig_dealloc_reply_port (__mig_get_reply_port ())
 #endif
 
 /* sysdeps/mach/MACHINE/sysdep.h should define the following macros.  */

^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2023-04-14 17:32 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-14 17:32 [glibc] hurd: Remove __hurd_local_reply_port Samuel Thibault

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).