* [PATCH 2/7] hurd: Move libc_hidden_def's around
2023-04-29 13:13 [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit Sergey Bugaev
@ 2023-04-29 13:13 ` Sergey Bugaev
2023-04-29 14:52 ` Samuel Thibault
2023-04-29 13:13 ` [PATCH 3/7] hurd: Mark various conditions as unlikely Sergey Bugaev
` (5 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Sergey Bugaev @ 2023-04-29 13:13 UTC (permalink / raw)
To: libc-alpha; +Cc: bug-hurd, Samuel Thibault
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/7] hurd: Move libc_hidden_def's around
2023-04-29 13:13 ` [PATCH 2/7] hurd: Move libc_hidden_def's around Sergey Bugaev
@ 2023-04-29 14:52 ` Samuel Thibault
0 siblings, 0 replies; 14+ messages in thread
From: Samuel Thibault @ 2023-04-29 14:52 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/7] hurd: Mark various conditions as unlikely
2023-04-29 13:13 [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit Sergey Bugaev
2023-04-29 13:13 ` [PATCH 2/7] hurd: Move libc_hidden_def's around Sergey Bugaev
@ 2023-04-29 13:13 ` Sergey Bugaev
2023-04-29 14:53 ` Samuel Thibault
2023-04-29 13:13 ` [PATCH 4/7] hurd: Make _exit work during early boot-up Sergey Bugaev
` (4 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Sergey Bugaev @ 2023-04-29 13:13 UTC (permalink / raw)
To: libc-alpha; +Cc: bug-hurd, Samuel Thibault
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/7] hurd: Mark various conditions as unlikely
2023-04-29 13:13 ` [PATCH 3/7] hurd: Mark various conditions as unlikely Sergey Bugaev
@ 2023-04-29 14:53 ` Samuel Thibault
0 siblings, 0 replies; 14+ messages in thread
From: Samuel Thibault @ 2023-04-29 14:53 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/7] hurd: Make _exit work during early boot-up
2023-04-29 13:13 [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit Sergey Bugaev
2023-04-29 13:13 ` [PATCH 2/7] hurd: Move libc_hidden_def's around Sergey Bugaev
2023-04-29 13:13 ` [PATCH 3/7] hurd: Mark various conditions as unlikely Sergey Bugaev
@ 2023-04-29 13:13 ` Sergey Bugaev
2023-04-29 14:54 ` Samuel Thibault
2023-04-29 13:13 ` [PATCH 5/7] hurd: Don't leak the auth port in msg* RPCs Sergey Bugaev
` (3 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Sergey Bugaev @ 2023-04-29 13:13 UTC (permalink / raw)
To: libc-alpha; +Cc: bug-hurd, Samuel Thibault
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/7] hurd: Make _exit work during early boot-up
2023-04-29 13:13 ` [PATCH 4/7] hurd: Make _exit work during early boot-up Sergey Bugaev
@ 2023-04-29 14:54 ` Samuel Thibault
0 siblings, 0 replies; 14+ messages in thread
From: Samuel Thibault @ 2023-04-29 14:54 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/7] hurd: Don't leak the auth port in msg* RPCs
2023-04-29 13:13 [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit Sergey Bugaev
` (2 preceding siblings ...)
2023-04-29 13:13 ` [PATCH 4/7] hurd: Make _exit work during early boot-up Sergey Bugaev
@ 2023-04-29 13:13 ` Sergey Bugaev
2023-04-29 14:56 ` Samuel Thibault
2023-04-29 13:13 ` [PATCH 6/7] hurd: Respect existing FD_CLOEXEC in S_msg_set_fd Sergey Bugaev
` (2 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Sergey Bugaev @ 2023-04-29 13:13 UTC (permalink / raw)
To: libc-alpha; +Cc: bug-hurd, Samuel Thibault
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 5/7] hurd: Don't leak the auth port in msg* RPCs
2023-04-29 13:13 ` [PATCH 5/7] hurd: Don't leak the auth port in msg* RPCs Sergey Bugaev
@ 2023-04-29 14:56 ` Samuel Thibault
0 siblings, 0 replies; 14+ messages in thread
From: Samuel Thibault @ 2023-04-29 14:56 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 6/7] hurd: Respect existing FD_CLOEXEC in S_msg_set_fd
2023-04-29 13:13 [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit Sergey Bugaev
` (3 preceding siblings ...)
2023-04-29 13:13 ` [PATCH 5/7] hurd: Don't leak the auth port in msg* RPCs Sergey Bugaev
@ 2023-04-29 13:13 ` Sergey Bugaev
2023-04-29 14:57 ` Samuel Thibault
2023-04-29 13:13 ` [PATCH 7/7] hurd: Fix FS_RETRY_MAGICAL "machtype" handling Sergey Bugaev
2023-04-29 14:50 ` [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit Samuel Thibault
6 siblings, 1 reply; 14+ messages in thread
From: Sergey Bugaev @ 2023-04-29 13:13 UTC (permalink / raw)
To: libc-alpha; +Cc: bug-hurd, Samuel Thibault
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 6/7] hurd: Respect existing FD_CLOEXEC in S_msg_set_fd
2023-04-29 13:13 ` [PATCH 6/7] hurd: Respect existing FD_CLOEXEC in S_msg_set_fd Sergey Bugaev
@ 2023-04-29 14:57 ` Samuel Thibault
0 siblings, 0 replies; 14+ messages in thread
From: Samuel Thibault @ 2023-04-29 14:57 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 7/7] hurd: Fix FS_RETRY_MAGICAL "machtype" handling
2023-04-29 13:13 [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit Sergey Bugaev
` (4 preceding siblings ...)
2023-04-29 13:13 ` [PATCH 6/7] hurd: Respect existing FD_CLOEXEC in S_msg_set_fd Sergey Bugaev
@ 2023-04-29 13:13 ` Sergey Bugaev
2023-04-29 15:01 ` Samuel Thibault
2023-04-29 14:50 ` [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit Samuel Thibault
6 siblings, 1 reply; 14+ messages in thread
From: Sergey Bugaev @ 2023-04-29 13:13 UTC (permalink / raw)
To: libc-alpha; +Cc: bug-hurd, Samuel Thibault
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 7/7] hurd: Fix FS_RETRY_MAGICAL "machtype" handling
2023-04-29 13:13 ` [PATCH 7/7] hurd: Fix FS_RETRY_MAGICAL "machtype" handling Sergey Bugaev
@ 2023-04-29 15:01 ` Samuel Thibault
0 siblings, 0 replies; 14+ messages in thread
From: Samuel Thibault @ 2023-04-29 15:01 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit
2023-04-29 13:13 [PATCH 1/7] hurd: Simplify _hurd_critical_section_lock a bit Sergey Bugaev
` (5 preceding siblings ...)
2023-04-29 13:13 ` [PATCH 7/7] hurd: Fix FS_RETRY_MAGICAL "machtype" handling Sergey Bugaev
@ 2023-04-29 14:50 ` Samuel Thibault
6 siblings, 0 replies; 14+ messages in thread
From: Samuel Thibault @ 2023-04-29 14:50 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 14+ messages in thread