[RFC,26/34] 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>
---
 hurd/hurd/threadvar.h         |  7 ------
 sysdeps/mach/hurd/mig-reply.c | 43 +++++++++++++++++++++++++++--------
 2 files changed, 33 insertions(+), 17 deletions(-)

Sergey Bugaev, le dim. 19 mars 2023 18:10:09 +0300, a ecrit:
>  /* Called by MiG to deallocate the reply port.  */
>  void
> -__mig_dealloc_reply_port (mach_port_t arg)
> +__mig_dealloc_reply_port (mach_port_t port)
>  {
> -  mach_port_t port = __hurd_local_reply_port;
> -  __hurd_local_reply_port = MACH_PORT_NULL;	/* So the mod_refs RPC won't use it.  */
> +  set_reply_port (MACH_PORT_NULL);	/* So the mod_refs RPC won't use it.  */
>  
>    if (MACH_PORT_VALID (port))
>      __mach_port_mod_refs (__mach_task_self (), port,

I believe we still want to use

 mach_port_t port = get_reply_port();

because the caller might not know whether its port is still valid
or not, e.g. when a signal interrupted the RPC and thus we had to
deallocate the reply port to make sure the server doesn't get confused.
In that case the caller will still have the old reply port name, which
we don't want to reallocate since it might already have been reused for
something else.

Samuel

Samuel Thibault, le mar. 11 avril 2023 00:07:43 +0200, a ecrit:
> Sergey Bugaev, le dim. 19 mars 2023 18:10:09 +0300, a ecrit:
> >  /* Called by MiG to deallocate the reply port.  */
> >  void
> > -__mig_dealloc_reply_port (mach_port_t arg)
> > +__mig_dealloc_reply_port (mach_port_t port)
> >  {
> > -  mach_port_t port = __hurd_local_reply_port;
> > -  __hurd_local_reply_port = MACH_PORT_NULL;	/* So the mod_refs RPC won't use it.  */
> > +  set_reply_port (MACH_PORT_NULL);	/* So the mod_refs RPC won't use it.  */
> >  
> >    if (MACH_PORT_VALID (port))
> >      __mach_port_mod_refs (__mach_task_self (), port,
> 
> I believe we still want to use
> 
>  mach_port_t port = get_reply_port();
> 
> because the caller might not know whether its port is still valid
> or not, e.g. when a signal interrupted the RPC and thus we had to
> deallocate the reply port to make sure the server doesn't get confused.
> In that case the caller will still have the old reply port name, which
> we don't want to reallocate since it might already have been reused for

                   deallocate

> something else.

On Tue, Apr 11, 2023 at 1:07 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> I believe we still want to use
>
>  mach_port_t port = get_reply_port();
>
> because the caller might not know whether its port is still valid
> or not, e.g. when a signal interrupted the RPC and thus we had to
> deallocate the reply port to make sure the server doesn't get confused.
> In that case the caller will still have the old reply port name, which
> we don't want to reallocate since it might already have been reused for
> something else.

*Great* point!

(And I should have thought of that..., but hey, this is what code reviews
are for, right?)

Side note, I really really dislike this idea of some code still referencing
port names that are no longer valid / deallocated / reused by someone else.
This is really prone to use-after-frees. Typically we'd solve this by
leaving a dead-name right in place of the port, and having
mig_dealloc_reply_port () dealloc this dead name.

But in this case... we're fairly sure that the code really doesn't do
anything with the name that it has, other than immediately calling
mig_dealloc_reply_port () on it; and there'd have to be a separate code path
for deallocating the dead name since mach_port_mod_refs (recv, -1) won't do
it (mach_port_destroy would handle both, but using that is a terrible idea).

So in order not to overcomplicate this, in this particular case, it should
be fine to just deallocate the stored reply port and not what the user has,
as you're saying. But it definitely needs a comment explaining this. And
maybe an assert (port == arg || port == MACH_PORT_NULL).

Sergey

Sergey Bugaev, le mar. 11 avril 2023 11:00:27 +0300, a ecrit:
> Side note, I really really dislike this idea of some code still referencing
> port names that are no longer valid / deallocated / reused by someone else.
> This is really prone to use-after-frees. Typically we'd solve this by
> leaving a dead-name right in place of the port, and having
> mig_dealloc_reply_port () dealloc this dead name.

That could be better indeed. Rather than modifying refs under the hood,
let the code manage them.

> But in this case... we're fairly sure that the code really doesn't do
> anything with the name that it has, other than immediately calling
> mig_dealloc_reply_port () on it; and there'd have to be a separate code path
> for deallocating the dead name since mach_port_mod_refs (recv, -1) won't do
> it (mach_port_destroy would handle both, but using that is a terrible idea).
> 
> So in order not to overcomplicate this, in this particular case, it should
> be fine to just deallocate the stored reply port and not what the user has,
> as you're saying. But it definitely needs a comment explaining this.

Completely agree :)

> And maybe an assert (port == arg || port == MACH_PORT_NULL).

If that does indeed work, yes :)

Samuel