[PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit

[PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit

[Sergey Bugaev]

This block of code was doing exactly what _hurd_self_sigstate does; so
just call that and let it do its job.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurd/signal.h | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/hurd/hurd/signal.h b/hurd/hurd/signal.h
index 662e955e..302ca25e 100644
--- a/hurd/hurd/signal.h
+++ b/hurd/hurd/signal.h
@@ -218,19 +218,7 @@ _hurd_critical_section_lock (void)
     return NULL;
 #endif
 
-  ss = THREAD_GETMEM (THREAD_SELF, _hurd_sigstate);
-  if (ss == NULL)
-    {
-      thread_t self = __mach_thread_self ();
-
-      /* The thread variable is unset; this must be the first time we've
-	 asked for it.  In this case, the critical section flag cannot
-	 possible already be set.  Look up our sigstate structure the slow
-	 way.  */
-      ss = _hurd_thread_sigstate (self);
-      THREAD_SETMEM (THREAD_SELF, _hurd_sigstate, ss);
-      __mach_port_deallocate (__mach_task_self (), self);
-    }
+  ss = _hurd_self_sigstate ();
 
   if (! __spin_try_lock (&ss->critical_section_lock))
     /* We are already in a critical section, so do nothing.  */
-- 
2.40.1

[PATCH 2/7] hurd: Move libc_hidden_def's around

[Sergey Bugaev]

Each libc_hidden_def should be placed immediately next to its function,
not in some random unrelated place.

No functional change.

Fixes: 653d74f12abea144219af00400ed1f1ac5dfa79f
"hurd: Global signal disposition"

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurdsig.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
index c84de4db..78ea59d9 100644
--- a/hurd/hurdsig.c
+++ b/hurd/hurdsig.c
@@ -174,6 +174,7 @@ _hurd_sigstate_set_global_rcv (struct hurd_sigstate *ss)
   assert (ss->thread != MACH_PORT_NULL);
   ss->actions[0].sa_handler = SIG_IGN;
 }
+libc_hidden_def (_hurd_sigstate_set_global_rcv)
 
 /* Check whether SS is a global receiver.  */
 static int
@@ -193,6 +194,8 @@ _hurd_sigstate_lock (struct hurd_sigstate *ss)
     __spin_lock (&_hurd_global_sigstate->lock);
   __spin_lock (&ss->lock);
 }
+libc_hidden_def (_hurd_sigstate_lock)
+
 void
 _hurd_sigstate_unlock (struct hurd_sigstate *ss)
 {
@@ -200,7 +203,7 @@ _hurd_sigstate_unlock (struct hurd_sigstate *ss)
   if (sigstate_is_global_rcv (ss))
     __spin_unlock (&_hurd_global_sigstate->lock);
 }
-libc_hidden_def (_hurd_sigstate_set_global_rcv)
+libc_hidden_def (_hurd_sigstate_unlock)
 
 /* Retrieve a thread's full set of pending signals, including the global
    ones if appropriate.  SS must be locked.  */
@@ -212,6 +215,7 @@ _hurd_sigstate_pending (const struct hurd_sigstate *ss)
     __sigorset (&pending, &pending, &_hurd_global_sigstate->pending);
   return pending;
 }
+libc_hidden_def (_hurd_sigstate_pending)
 
 /* Clear a pending signal and return the associated detailed
    signal information. SS must be locked, and must have signal SIGNO
@@ -230,8 +234,6 @@ sigstate_clear_pending (struct hurd_sigstate *ss, int signo)
   __sigdelset (&ss->pending, signo);
   return ss->pending_data[signo];
 }
-libc_hidden_def (_hurd_sigstate_lock)
-libc_hidden_def (_hurd_sigstate_unlock)
 
 /* Retrieve a thread's action vector.  SS must be locked.  */
 struct sigaction *
@@ -242,7 +244,6 @@ _hurd_sigstate_actions (struct hurd_sigstate *ss)
   else
     return ss->actions;
 }
-libc_hidden_def (_hurd_sigstate_pending)
 
 \f
 /* Signal delivery itself is on this page.  */
-- 
2.40.1

[PATCH 3/7] hurd: Mark various conditions as unlikely

[Sergey Bugaev]

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurd/fd.h     | 10 +++++-----
 hurd/hurd/signal.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hurd/hurd/fd.h b/hurd/hurd/fd.h
index 241797bf..824b7dbb 100644
--- a/hurd/hurd/fd.h
+++ b/hurd/hurd/fd.h
@@ -73,18 +73,18 @@ _hurd_fd_get (int fd)
 
   HURD_CRITICAL_BEGIN;
   __mutex_lock (&_hurd_dtable_lock);
-  if (fd < 0 || fd >= _hurd_dtablesize)
+  if (__glibc_unlikely (fd < 0 || fd >= _hurd_dtablesize))
     descriptor = NULL;
   else
     {
       struct hurd_fd *cell = _hurd_dtable[fd];
-      if (cell == NULL)
+      if (__glibc_unlikely (cell == NULL))
 	/* No descriptor allocated at this index.  */
 	descriptor = NULL;
       else
 	{
 	  __spin_lock (&cell->port.lock);
-	  if (cell->port.port == MACH_PORT_NULL)
+	  if (__glibc_unlikely (cell->port.port == MACH_PORT_NULL))
 	    /* The descriptor at this index has no port in it.
 	       This happens if it existed before but was closed.  */
 	    descriptor = NULL;
@@ -107,7 +107,7 @@ _hurd_fd_get (int fd)
 
 #define	HURD_FD_USE(fd, expr)						      \
   ({ struct hurd_fd *descriptor = _hurd_fd_get (fd);			      \
-     descriptor == NULL ? EBADF : (expr); })
+     __glibc_unlikely (descriptor == NULL) ? EBADF : (expr); })
 
 /* Evaluate EXPR with the variable `port' bound to the port to FD, and
    `ctty' bound to the ctty port.  */
@@ -125,7 +125,7 @@ _hurd_fd_get (int fd)
      io_t port, ctty;							      \
      void *crit = _hurd_critical_section_lock ();			      \
      __spin_lock (&__d->port.lock);					      \
-     if (__d->port.port == MACH_PORT_NULL)				      \
+     if (__glibc_unlikely (__d->port.port == MACH_PORT_NULL))		      \
        {								      \
 	 __spin_unlock (&__d->port.lock);				      \
 	 _hurd_critical_section_unlock (crit);				      \
diff --git a/hurd/hurd/signal.h b/hurd/hurd/signal.h
index 302ca25e..8a547fd0 100644
--- a/hurd/hurd/signal.h
+++ b/hurd/hurd/signal.h
@@ -249,9 +249,9 @@ _hurd_critical_section_unlock (void *our_lock)
       sigset_t pending;
       _hurd_sigstate_lock (ss);
       __spin_unlock (&ss->critical_section_lock);
-      pending = _hurd_sigstate_pending(ss) & ~ss->blocked;
+      pending = _hurd_sigstate_pending (ss) & ~ss->blocked;
       _hurd_sigstate_unlock (ss);
-      if (! __sigisemptyset (&pending))
+      if (__glibc_unlikely (!__sigisemptyset (&pending)))
 	/* There are unblocked signals pending, which weren't
 	   delivered because we were in the critical section.
 	   Tell the signal thread to deliver them now.  */
-- 
2.40.1

[PATCH 4/7] hurd: Make _exit work during early boot-up

[Sergey Bugaev]

If any of the early boot-up tasks calls exit () or returns from main (),
terminate it properly instead of crashing on trying to dereference
_hurd_ports and getting forcibly terminated by the kernel.

We sadly cannot make the __USEPORT macro do the check for _hurd_ports
being unset, because it evaluates to the value of the expression
provided as the second argument, and that can be of any type; so there
is no single suitable fallback value for the macro to evaluate to in
case _hurd_ports is unset. Instead, each use site that wants to care for
this case will have to do its own checking.

Checked on x86_64-gnu.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/_exit.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sysdeps/mach/hurd/_exit.c b/sysdeps/mach/hurd/_exit.c
index 73957744..80cfe532 100644
--- a/sysdeps/mach/hurd/_exit.c
+++ b/sysdeps/mach/hurd/_exit.c
@@ -24,8 +24,9 @@
 void
 _hurd_exit (int status)
 {
-  /* Give the proc server our exit status.  */
-  __USEPORT (PROC, __proc_mark_exit (port, status, 0));
+  if (_hurd_ports != NULL)
+    /* Give the proc server our exit status.  */
+    __USEPORT (PROC, __proc_mark_exit (port, status, 0));
 
   /* Commit suicide.  */
   __task_terminate (__mach_task_self ());
-- 
2.40.1

[PATCH 5/7] hurd: Don't leak the auth port in msg* RPCs

[Sergey Bugaev]

The leak can be easily reproduced (and observed) using the portinfo
tool:

$ portinfo -v $$ | grep task
    36: send task(1577)(self) (refs: 127)
$ portinfo -v $$ | grep task
    36: send task(1577)(self) (refs: 253)
$ portinfo -v $$ | grep task
    36: send task(1577)(self) (refs: 379)
$ portinfo -v $$ | grep task
    36: send task(1577)(self) (refs: 505)
$ portinfo -v $$ | grep task
    36: send task(1577)(self) (refs: 631)

Checked on i686-gnu.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurdmsg.c | 67 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 9 deletions(-)

diff --git a/hurd/hurdmsg.c b/hurd/hurdmsg.c
index 4bedd292..896fb87c 100644
--- a/hurd/hurdmsg.c
+++ b/hurd/hurdmsg.c
@@ -35,11 +35,17 @@ kern_return_t
   _S_msg_get_init_port (mach_port_t msgport, mach_port_t auth, int which,
 			mach_port_t *result, mach_msg_type_name_t *result_type)
 {
+  error_t err;
+
   AUTHCHECK;
+
   *result_type = MACH_MSG_TYPE_MOVE_SEND;
   /* This function adds a new user reference for the *RESULT it gives back.
      Our reply message uses a move-send right that consumes this reference.  */
-  return _hurd_ports_get (which, result);
+  err = _hurd_ports_get (which, result);
+  if (!err && MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
+  return err;
 }
 
 kern_return_t
@@ -51,10 +57,13 @@ _S_msg_set_init_port (mach_port_t msgport, mach_port_t auth,
   AUTHCHECK;
 
   err = _hurd_ports_set (which, port);
-  if (err == 0)
+
+  if (!err && MACH_PORT_VALID (port))
     __mach_port_deallocate (__mach_task_self (), port);
+  if (!err && MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
 
-  return 0;
+  return err;
 }
 
 kern_return_t
@@ -88,6 +97,8 @@ _S_msg_get_init_ports (mach_port_t msgport, mach_port_t auth,
       }
 
   *ports_type = MACH_MSG_TYPE_MOVE_SEND;
+  if (MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
   return 0;
 }
 
@@ -108,6 +119,8 @@ _S_msg_set_init_ports (mach_port_t msgport, mach_port_t auth,
 	__mach_port_deallocate (__mach_task_self (), ports[i]);
     }
 
+  if (MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
   return 0;
 }
 \f
@@ -152,9 +165,16 @@ kern_return_t
 _S_msg_get_init_int (mach_port_t msgport, mach_port_t auth,
 		     int which, int *value)
 {
+  error_t err;
+
   AUTHCHECK;
 
-  return get_int (which, value);
+  err = get_int (which, value);
+  if (err)
+    return err;
+  if (MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
+  return 0;
 }
 
 kern_return_t
@@ -185,6 +205,8 @@ _S_msg_get_init_ints (mach_port_t msgport, mach_port_t auth,
 	return err;
       }
 
+  if (MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
   return 0;
 }
 
@@ -236,9 +258,16 @@ kern_return_t
 _S_msg_set_init_int (mach_port_t msgport, mach_port_t auth,
 		     int which, int value)
 {
+  error_t err;
+
   AUTHCHECK;
 
-  return set_int (which, value);
+  err = set_int (which, value);
+  if (err)
+    return err;
+  if (MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
+  return 0;
 }
 
 kern_return_t
@@ -261,6 +290,8 @@ _S_msg_set_init_ints (mach_port_t msgport, mach_port_t auth,
 	return err;
       }
 
+  if (MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
   return 0;
 }
 \f
@@ -278,6 +309,8 @@ _S_msg_get_fd (mach_port_t msgport, mach_port_t auth, int which,
     return errno;
   *result_type = MACH_MSG_TYPE_MOVE_SEND;
 
+  if (MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
   return 0;
 }
 
@@ -285,17 +318,25 @@ kern_return_t
 _S_msg_set_fd (mach_port_t msgport, mach_port_t auth,
 	       int which, mach_port_t port)
 {
+  error_t err;
+
   AUTHCHECK;
 
   /* We consume the reference if successful.  */
-  return HURD_FD_USE (which, (_hurd_port2fd (descriptor, port, 0), 0));
+  err = HURD_FD_USE (which, (_hurd_port2fd (descriptor, port, 0), 0));
+  if (err)
+    return err;
+
+  if (MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
+  return 0;
 }
 \f
 /* Snarfing and frobbing environment variables.  */
 
 kern_return_t
 _S_msg_get_env_variable (mach_port_t msgport,
-			 const_string_t variable, //
+			 const_string_t variable,
 			 char **data, mach_msg_type_number_t *datalen)
 {
   error_t err;
@@ -322,14 +363,17 @@ _S_msg_get_env_variable (mach_port_t msgport,
 
 kern_return_t
 _S_msg_set_env_variable (mach_port_t msgport, mach_port_t auth,
-			 const_string_t variable, //
-			 const_string_t value, //
+			 const_string_t variable,
+			 const_string_t value,
 			 int replace)
 {
   AUTHCHECK;
 
   if (__setenv (variable, value, replace)) /* XXX name space */
     return errno;
+
+  if (MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
   return 0;
 }
 
@@ -381,6 +425,9 @@ _S_msg_set_environment (mach_port_t msgport, mach_port_t auth,
     return errno;
   __argz_extract (data, datalen, envp);
   __environ = envp;		/* XXX cooperate with loadenv et al */
+
+  if (MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
   return 0;
 }
 \f
@@ -433,6 +480,8 @@ _S_msg_get_dtable (mach_port_t process,
 out:
   __mutex_unlock (&_hurd_dtable_lock);
   HURD_CRITICAL_END;
+  if (!err && MACH_PORT_VALID (auth))
+    __mach_port_deallocate (__mach_task_self (), auth);
   return err;
 }
 
-- 
2.40.1

[PATCH 6/7] hurd: Respect existing FD_CLOEXEC in S_msg_set_fd

[Sergey Bugaev]

If the process has set the close-on-exec flag for the file descriptor,
it expects the file descriptor to get closed on exec, even if we replace
what the file descriptor refers to.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurdmsg.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hurd/hurdmsg.c b/hurd/hurdmsg.c
index 896fb87c..8fde1f53 100644
--- a/hurd/hurdmsg.c
+++ b/hurd/hurdmsg.c
@@ -323,7 +323,13 @@ _S_msg_set_fd (mach_port_t msgport, mach_port_t auth,
   AUTHCHECK;
 
   /* We consume the reference if successful.  */
-  err = HURD_FD_USE (which, (_hurd_port2fd (descriptor, port, 0), 0));
+  err = HURD_FD_USE (which,
+		     ({
+		       int flags = (descriptor->flags & FD_CLOEXEC)
+				   ? O_CLOEXEC : 0;
+		       _hurd_port2fd (descriptor, port, flags);
+		       0;
+		     }));
   if (err)
     return err;
 
-- 
2.40.1

[PATCH 7/7] hurd: Fix FS_RETRY_MAGICAL "machtype" handling

[Sergey Bugaev]

We need to set file_name, not update retryname. This is what the other
branches do.

Before this change, any attempt to access such a file would segfault due
to file_name being unset:

$ settrans -ac /tmp/my-machtype /hurd/magic machtype
$ cat /tmp/my-machtype
Segmentation fault

Checked on i686-gnu.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/lookup-retry.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hurd/lookup-retry.c b/hurd/lookup-retry.c
index 99c98104..62f94bbc 100644
--- a/hurd/lookup-retry.c
+++ b/hurd/lookup-retry.c
@@ -277,7 +277,6 @@ __hurd_file_name_lookup_retry (error_t (*use_init_port)
 		  error_t err;
 		  struct host_basic_info hostinfo;
 		  mach_msg_type_number_t hostinfocnt = HOST_BASIC_INFO_COUNT;
-		  char *p;
 		  /* XXX want client's host */
 		  if (err = __host_info (__mach_host_self (), HOST_BASIC_INFO,
 					 (integer_t *) &hostinfo,
@@ -288,13 +287,11 @@ __hurd_file_name_lookup_retry (error_t (*use_init_port)
 		      err = EGRATUITOUS;
 		      goto out;
 		    }
-		  p = _itoa (hostinfo.cpu_subtype, &retryname[8], 10, 0);
-		  *--p = '/';
-		  p = _itoa (hostinfo.cpu_type, &retryname[8], 10, 0);
-		  if (p < retryname)
+		  file_name = _itoa (hostinfo.cpu_subtype, &retryname[8], 10, 0);
+		  *--file_name = '/';
+		  file_name = _itoa (hostinfo.cpu_type, file_name, 10, 0);
+		  if (file_name < retryname)
 		    abort ();	/* XXX write this right if this ever happens */
-		  if (p > retryname)
-		    memmove (retryname, p, strlen(p) + 1);
 		  startdir = *result;
 		}
 	      else
-- 
2.40.1

Re: [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le sam. 29 avril 2023 16:13:48 +0300, a ecrit:
> This block of code was doing exactly what _hurd_self_sigstate does; so
> just call that and let it do its job.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/hurd/signal.h | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/hurd/hurd/signal.h b/hurd/hurd/signal.h
> index 662e955e..302ca25e 100644
> --- a/hurd/hurd/signal.h
> +++ b/hurd/hurd/signal.h
> @@ -218,19 +218,7 @@ _hurd_critical_section_lock (void)
>      return NULL;
>  #endif
>  
> -  ss = THREAD_GETMEM (THREAD_SELF, _hurd_sigstate);
> -  if (ss == NULL)
> -    {
> -      thread_t self = __mach_thread_self ();
> -
> -      /* The thread variable is unset; this must be the first time we've
> -	 asked for it.  In this case, the critical section flag cannot
> -	 possible already be set.  Look up our sigstate structure the slow
> -	 way.  */
> -      ss = _hurd_thread_sigstate (self);
> -      THREAD_SETMEM (THREAD_SELF, _hurd_sigstate, ss);
> -      __mach_port_deallocate (__mach_task_self (), self);
> -    }
> +  ss = _hurd_self_sigstate ();
>  
>    if (! __spin_try_lock (&ss->critical_section_lock))
>      /* We are already in a critical section, so do nothing.  */
> -- 
> 2.40.1
> 

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

Re: [PATCH 2/7] hurd: Move libc_hidden_def's around

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev via Libc-alpha, le sam. 29 avril 2023 16:13:49 +0300, a ecrit:
> Each libc_hidden_def should be placed immediately next to its function,
> not in some random unrelated place.
> 
> No functional change.
> 
> Fixes: 653d74f12abea144219af00400ed1f1ac5dfa79f
> "hurd: Global signal disposition"
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/hurdsig.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
> index c84de4db..78ea59d9 100644
> --- a/hurd/hurdsig.c
> +++ b/hurd/hurdsig.c
> @@ -174,6 +174,7 @@ _hurd_sigstate_set_global_rcv (struct hurd_sigstate *ss)
>    assert (ss->thread != MACH_PORT_NULL);
>    ss->actions[0].sa_handler = SIG_IGN;
>  }
> +libc_hidden_def (_hurd_sigstate_set_global_rcv)
>  
>  /* Check whether SS is a global receiver.  */
>  static int
> @@ -193,6 +194,8 @@ _hurd_sigstate_lock (struct hurd_sigstate *ss)
>      __spin_lock (&_hurd_global_sigstate->lock);
>    __spin_lock (&ss->lock);
>  }
> +libc_hidden_def (_hurd_sigstate_lock)
> +
>  void
>  _hurd_sigstate_unlock (struct hurd_sigstate *ss)
>  {
> @@ -200,7 +203,7 @@ _hurd_sigstate_unlock (struct hurd_sigstate *ss)
>    if (sigstate_is_global_rcv (ss))
>      __spin_unlock (&_hurd_global_sigstate->lock);
>  }
> -libc_hidden_def (_hurd_sigstate_set_global_rcv)
> +libc_hidden_def (_hurd_sigstate_unlock)
>  
>  /* Retrieve a thread's full set of pending signals, including the global
>     ones if appropriate.  SS must be locked.  */
> @@ -212,6 +215,7 @@ _hurd_sigstate_pending (const struct hurd_sigstate *ss)
>      __sigorset (&pending, &pending, &_hurd_global_sigstate->pending);
>    return pending;
>  }
> +libc_hidden_def (_hurd_sigstate_pending)
>  
>  /* Clear a pending signal and return the associated detailed
>     signal information. SS must be locked, and must have signal SIGNO
> @@ -230,8 +234,6 @@ sigstate_clear_pending (struct hurd_sigstate *ss, int signo)
>    __sigdelset (&ss->pending, signo);
>    return ss->pending_data[signo];
>  }
> -libc_hidden_def (_hurd_sigstate_lock)
> -libc_hidden_def (_hurd_sigstate_unlock)
>  
>  /* Retrieve a thread's action vector.  SS must be locked.  */
>  struct sigaction *
> @@ -242,7 +244,6 @@ _hurd_sigstate_actions (struct hurd_sigstate *ss)
>    else
>      return ss->actions;
>  }
> -libc_hidden_def (_hurd_sigstate_pending)
>  
>  \f
>  /* Signal delivery itself is on this page.  */
> -- 
> 2.40.1
> 

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

Re: [PATCH 3/7] hurd: Mark various conditions as unlikely

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le sam. 29 avril 2023 16:13:50 +0300, a ecrit:
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/hurd/fd.h     | 10 +++++-----
>  hurd/hurd/signal.h |  4 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hurd/hurd/fd.h b/hurd/hurd/fd.h
> index 241797bf..824b7dbb 100644
> --- a/hurd/hurd/fd.h
> +++ b/hurd/hurd/fd.h
> @@ -73,18 +73,18 @@ _hurd_fd_get (int fd)
>  
>    HURD_CRITICAL_BEGIN;
>    __mutex_lock (&_hurd_dtable_lock);
> -  if (fd < 0 || fd >= _hurd_dtablesize)
> +  if (__glibc_unlikely (fd < 0 || fd >= _hurd_dtablesize))
>      descriptor = NULL;
>    else
>      {
>        struct hurd_fd *cell = _hurd_dtable[fd];
> -      if (cell == NULL)
> +      if (__glibc_unlikely (cell == NULL))
>  	/* No descriptor allocated at this index.  */
>  	descriptor = NULL;
>        else
>  	{
>  	  __spin_lock (&cell->port.lock);
> -	  if (cell->port.port == MACH_PORT_NULL)
> +	  if (__glibc_unlikely (cell->port.port == MACH_PORT_NULL))
>  	    /* The descriptor at this index has no port in it.
>  	       This happens if it existed before but was closed.  */
>  	    descriptor = NULL;
> @@ -107,7 +107,7 @@ _hurd_fd_get (int fd)
>  
>  #define	HURD_FD_USE(fd, expr)						      \
>    ({ struct hurd_fd *descriptor = _hurd_fd_get (fd);			      \
> -     descriptor == NULL ? EBADF : (expr); })
> +     __glibc_unlikely (descriptor == NULL) ? EBADF : (expr); })
>  
>  /* Evaluate EXPR with the variable `port' bound to the port to FD, and
>     `ctty' bound to the ctty port.  */
> @@ -125,7 +125,7 @@ _hurd_fd_get (int fd)
>       io_t port, ctty;							      \
>       void *crit = _hurd_critical_section_lock ();			      \
>       __spin_lock (&__d->port.lock);					      \
> -     if (__d->port.port == MACH_PORT_NULL)				      \
> +     if (__glibc_unlikely (__d->port.port == MACH_PORT_NULL))		      \
>         {								      \
>  	 __spin_unlock (&__d->port.lock);				      \
>  	 _hurd_critical_section_unlock (crit);				      \
> diff --git a/hurd/hurd/signal.h b/hurd/hurd/signal.h
> index 302ca25e..8a547fd0 100644
> --- a/hurd/hurd/signal.h
> +++ b/hurd/hurd/signal.h
> @@ -249,9 +249,9 @@ _hurd_critical_section_unlock (void *our_lock)
>        sigset_t pending;
>        _hurd_sigstate_lock (ss);
>        __spin_unlock (&ss->critical_section_lock);
> -      pending = _hurd_sigstate_pending(ss) & ~ss->blocked;
> +      pending = _hurd_sigstate_pending (ss) & ~ss->blocked;
>        _hurd_sigstate_unlock (ss);
> -      if (! __sigisemptyset (&pending))
> +      if (__glibc_unlikely (!__sigisemptyset (&pending)))
>  	/* There are unblocked signals pending, which weren't
>  	   delivered because we were in the critical section.
>  	   Tell the signal thread to deliver them now.  */
> -- 
> 2.40.1
> 

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

Re: [PATCH 4/7] hurd: Make _exit work during early boot-up

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le sam. 29 avril 2023 16:13:51 +0300, a ecrit:
> If any of the early boot-up tasks calls exit () or returns from main (),
> terminate it properly instead of crashing on trying to dereference
> _hurd_ports and getting forcibly terminated by the kernel.
> 
> We sadly cannot make the __USEPORT macro do the check for _hurd_ports
> being unset, because it evaluates to the value of the expression
> provided as the second argument, and that can be of any type; so there
> is no single suitable fallback value for the macro to evaluate to in
> case _hurd_ports is unset. Instead, each use site that wants to care for
> this case will have to do its own checking.
> 
> Checked on x86_64-gnu.

\o/

> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/_exit.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/_exit.c b/sysdeps/mach/hurd/_exit.c
> index 73957744..80cfe532 100644
> --- a/sysdeps/mach/hurd/_exit.c
> +++ b/sysdeps/mach/hurd/_exit.c
> @@ -24,8 +24,9 @@
>  void
>  _hurd_exit (int status)
>  {
> -  /* Give the proc server our exit status.  */
> -  __USEPORT (PROC, __proc_mark_exit (port, status, 0));
> +  if (_hurd_ports != NULL)
> +    /* Give the proc server our exit status.  */
> +    __USEPORT (PROC, __proc_mark_exit (port, status, 0));
>  
>    /* Commit suicide.  */
>    __task_terminate (__mach_task_self ());
> -- 
> 2.40.1
> 

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

Re: [PATCH 5/7] hurd: Don't leak the auth port in msg* RPCs

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le sam. 29 avril 2023 16:13:52 +0300, a ecrit:
> The leak can be easily reproduced (and observed) using the portinfo
> tool:
> 
> $ portinfo -v $$ | grep task
>     36: send task(1577)(self) (refs: 127)
> $ portinfo -v $$ | grep task
>     36: send task(1577)(self) (refs: 253)
> $ portinfo -v $$ | grep task
>     36: send task(1577)(self) (refs: 379)
> $ portinfo -v $$ | grep task
>     36: send task(1577)(self) (refs: 505)
> $ portinfo -v $$ | grep task
>     36: send task(1577)(self) (refs: 631)
> 
> Checked on i686-gnu.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/hurdmsg.c | 67 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 58 insertions(+), 9 deletions(-)
> 
> diff --git a/hurd/hurdmsg.c b/hurd/hurdmsg.c
> index 4bedd292..896fb87c 100644
> --- a/hurd/hurdmsg.c
> +++ b/hurd/hurdmsg.c
> @@ -35,11 +35,17 @@ kern_return_t
>    _S_msg_get_init_port (mach_port_t msgport, mach_port_t auth, int which,
>  			mach_port_t *result, mach_msg_type_name_t *result_type)
>  {
> +  error_t err;
> +
>    AUTHCHECK;
> +
>    *result_type = MACH_MSG_TYPE_MOVE_SEND;
>    /* This function adds a new user reference for the *RESULT it gives back.
>       Our reply message uses a move-send right that consumes this reference.  */
> -  return _hurd_ports_get (which, result);
> +  err = _hurd_ports_get (which, result);
> +  if (!err && MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
> +  return err;
>  }
>  
>  kern_return_t
> @@ -51,10 +57,13 @@ _S_msg_set_init_port (mach_port_t msgport, mach_port_t auth,
>    AUTHCHECK;
>  
>    err = _hurd_ports_set (which, port);
> -  if (err == 0)
> +
> +  if (!err && MACH_PORT_VALID (port))
>      __mach_port_deallocate (__mach_task_self (), port);
> +  if (!err && MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
>  
> -  return 0;
> +  return err;
>  }
>  
>  kern_return_t
> @@ -88,6 +97,8 @@ _S_msg_get_init_ports (mach_port_t msgport, mach_port_t auth,
>        }
>  
>    *ports_type = MACH_MSG_TYPE_MOVE_SEND;
> +  if (MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
>    return 0;
>  }
>  
> @@ -108,6 +119,8 @@ _S_msg_set_init_ports (mach_port_t msgport, mach_port_t auth,
>  	__mach_port_deallocate (__mach_task_self (), ports[i]);
>      }
>  
> +  if (MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
>    return 0;
>  }
>  \f
> @@ -152,9 +165,16 @@ kern_return_t
>  _S_msg_get_init_int (mach_port_t msgport, mach_port_t auth,
>  		     int which, int *value)
>  {
> +  error_t err;
> +
>    AUTHCHECK;
>  
> -  return get_int (which, value);
> +  err = get_int (which, value);
> +  if (err)
> +    return err;
> +  if (MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
> +  return 0;
>  }
>  
>  kern_return_t
> @@ -185,6 +205,8 @@ _S_msg_get_init_ints (mach_port_t msgport, mach_port_t auth,
>  	return err;
>        }
>  
> +  if (MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
>    return 0;
>  }
>  
> @@ -236,9 +258,16 @@ kern_return_t
>  _S_msg_set_init_int (mach_port_t msgport, mach_port_t auth,
>  		     int which, int value)
>  {
> +  error_t err;
> +
>    AUTHCHECK;
>  
> -  return set_int (which, value);
> +  err = set_int (which, value);
> +  if (err)
> +    return err;
> +  if (MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
> +  return 0;
>  }
>  
>  kern_return_t
> @@ -261,6 +290,8 @@ _S_msg_set_init_ints (mach_port_t msgport, mach_port_t auth,
>  	return err;
>        }
>  
> +  if (MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
>    return 0;
>  }
>  \f
> @@ -278,6 +309,8 @@ _S_msg_get_fd (mach_port_t msgport, mach_port_t auth, int which,
>      return errno;
>    *result_type = MACH_MSG_TYPE_MOVE_SEND;
>  
> +  if (MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
>    return 0;
>  }
>  
> @@ -285,17 +318,25 @@ kern_return_t
>  _S_msg_set_fd (mach_port_t msgport, mach_port_t auth,
>  	       int which, mach_port_t port)
>  {
> +  error_t err;
> +
>    AUTHCHECK;
>  
>    /* We consume the reference if successful.  */
> -  return HURD_FD_USE (which, (_hurd_port2fd (descriptor, port, 0), 0));
> +  err = HURD_FD_USE (which, (_hurd_port2fd (descriptor, port, 0), 0));
> +  if (err)
> +    return err;
> +
> +  if (MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
> +  return 0;
>  }
>  \f
>  /* Snarfing and frobbing environment variables.  */
>  
>  kern_return_t
>  _S_msg_get_env_variable (mach_port_t msgport,
> -			 const_string_t variable, //
> +			 const_string_t variable,
>  			 char **data, mach_msg_type_number_t *datalen)
>  {
>    error_t err;
> @@ -322,14 +363,17 @@ _S_msg_get_env_variable (mach_port_t msgport,
>  
>  kern_return_t
>  _S_msg_set_env_variable (mach_port_t msgport, mach_port_t auth,
> -			 const_string_t variable, //
> -			 const_string_t value, //
> +			 const_string_t variable,
> +			 const_string_t value,
>  			 int replace)
>  {
>    AUTHCHECK;
>  
>    if (__setenv (variable, value, replace)) /* XXX name space */
>      return errno;
> +
> +  if (MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
>    return 0;
>  }
>  
> @@ -381,6 +425,9 @@ _S_msg_set_environment (mach_port_t msgport, mach_port_t auth,
>      return errno;
>    __argz_extract (data, datalen, envp);
>    __environ = envp;		/* XXX cooperate with loadenv et al */
> +
> +  if (MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
>    return 0;
>  }
>  \f
> @@ -433,6 +480,8 @@ _S_msg_get_dtable (mach_port_t process,
>  out:
>    __mutex_unlock (&_hurd_dtable_lock);
>    HURD_CRITICAL_END;
> +  if (!err && MACH_PORT_VALID (auth))
> +    __mach_port_deallocate (__mach_task_self (), auth);
>    return err;
>  }
>  
> -- 
> 2.40.1
> 
> 

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

Re: [PATCH 6/7] hurd: Respect existing FD_CLOEXEC in S_msg_set_fd

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le sam. 29 avril 2023 16:13:53 +0300, a ecrit:
> If the process has set the close-on-exec flag for the file descriptor,
> it expects the file descriptor to get closed on exec, even if we replace
> what the file descriptor refers to.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/hurdmsg.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hurd/hurdmsg.c b/hurd/hurdmsg.c
> index 896fb87c..8fde1f53 100644
> --- a/hurd/hurdmsg.c
> +++ b/hurd/hurdmsg.c
> @@ -323,7 +323,13 @@ _S_msg_set_fd (mach_port_t msgport, mach_port_t auth,
>    AUTHCHECK;
>  
>    /* We consume the reference if successful.  */
> -  err = HURD_FD_USE (which, (_hurd_port2fd (descriptor, port, 0), 0));
> +  err = HURD_FD_USE (which,
> +		     ({
> +		       int flags = (descriptor->flags & FD_CLOEXEC)
> +				   ? O_CLOEXEC : 0;
> +		       _hurd_port2fd (descriptor, port, flags);
> +		       0;
> +		     }));
>    if (err)
>      return err;
>  
> -- 
> 2.40.1
> 

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

Re: [PATCH 7/7] hurd: Fix FS_RETRY_MAGICAL "machtype" handling

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le sam. 29 avril 2023 16:13:54 +0300, a ecrit:
> We need to set file_name, not update retryname. This is what the other
> branches do.
> 
> Before this change, any attempt to access such a file would segfault due
> to file_name being unset:
> 
> $ settrans -ac /tmp/my-machtype /hurd/magic machtype
> $ cat /tmp/my-machtype
> Segmentation fault
> 
> Checked on i686-gnu.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/lookup-retry.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hurd/lookup-retry.c b/hurd/lookup-retry.c
> index 99c98104..62f94bbc 100644
> --- a/hurd/lookup-retry.c
> +++ b/hurd/lookup-retry.c
> @@ -277,7 +277,6 @@ __hurd_file_name_lookup_retry (error_t (*use_init_port)
>  		  error_t err;
>  		  struct host_basic_info hostinfo;
>  		  mach_msg_type_number_t hostinfocnt = HOST_BASIC_INFO_COUNT;
> -		  char *p;
>  		  /* XXX want client's host */
>  		  if (err = __host_info (__mach_host_self (), HOST_BASIC_INFO,
>  					 (integer_t *) &hostinfo,
> @@ -288,13 +287,11 @@ __hurd_file_name_lookup_retry (error_t (*use_init_port)
>  		      err = EGRATUITOUS;
>  		      goto out;
>  		    }
> -		  p = _itoa (hostinfo.cpu_subtype, &retryname[8], 10, 0);
> -		  *--p = '/';
> -		  p = _itoa (hostinfo.cpu_type, &retryname[8], 10, 0);
> -		  if (p < retryname)
> +		  file_name = _itoa (hostinfo.cpu_subtype, &retryname[8], 10, 0);
> +		  *--file_name = '/';
> +		  file_name = _itoa (hostinfo.cpu_type, file_name, 10, 0);
> +		  if (file_name < retryname)
>  		    abort ();	/* XXX write this right if this ever happens */
> -		  if (p > retryname)
> -		    memmove (retryname, p, strlen(p) + 1);
>  		  startdir = *result;
>  		}
>  	      else
> -- 
> 2.40.1
> 

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