intr-msg / hurdsig looks broken, is my analysis correct?

[Sergey Bugaev]

Hello yet again!

I'm looking at sysdeps/mach/hurd/i386/intr-msg.h in glibc -- with the
intention of eventually porting it to x86_64, but that's out of the
question for now, trying to understand it first. The three other
relevant source files for it seem to be:
1. hurd/intr-msg.c -- this is the main logic behind interruptible
messaging, intr-msg.h basically defining its arch-dependent part
(namely, INTR_MSG_TRAP). intr-msg.c itself does not seem to do any
low-level register/PC/jumps/thread_state magic, other than what
intr-msg.h does.
2. sysdeps/mach/hurd/i386/trampoline.c -- this has a piece of code
that claims to have "intimate knowledge of the special mach_msg system
call done in intr-msg.c", except that it clearly has a wrong idea
about what INTR_MSG_TRAP does, since the snippet of asm included in
the comment in trampoline.c does not match the relevant code of
INTR_MSG_TRAP.
3. hurd/hurdsig.c is what uses INTR_MSG_BACK_OUT. Here's the relevant
piece of code:

  if (state->basic.PC >= (uintptr_t) &_hurd_intr_rpc_msg_about_to
      && state->basic.PC < (uintptr_t) &_hurd_intr_rpc_msg_in_trap)
    {
      /* The thread is about to do the RPC, but hasn't yet entered
mach_msg.  Mutate the thread's state so it knows not to try
the RPC.  */
      INTR_MSG_BACK_OUT (&state->basic);
      MACHINE_THREAD_STATE_SET_PC (&state->basic,
   &_hurd_intr_rpc_msg_in_trap);
      state->basic.SYSRETURN = MACH_SEND_INTERRUPTED;
      *state_change = 1;
    }

Or in other words: this forcefully jumps the thread to right after the
syscall, pretending it has returned MACH_SEND_INTERRUPTED, but also
makes any arch-specific changes to the state (INTR_MSG_BACK_OUT).

Now, I have found commit 1d20f33ff4fb634310f27493b7b87d0b20f4a0b0 [0]
("hurd: Fix intr-msg parameter/stack kludge") that changed how
INTR_MSG_TRAP deals with the stack. Before the commit, it saved the
original esp into ecx, then set esp to, uh, something else, performed
the syscall, and reset esp back.

[0]: 
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=1d20f33ff4fb634310f27493b7b87d0b20f4a0b0

This is explicitly supported by:
- trampoline.c, which detects that esp is not the "real" esp in
between ._hurd_intr_rpc_msg_cx_sp and ._hurd_intr_rpc_msg_sp_restored,
and seems to deal with that *somehow* by loading the real sp from ecx.
- INTR_MSG_BACK_OUT, which... seems to be a little misguided, even for
the pre-1d20f33ff4fb634310f27493b7b87d0b20f4a0b0 logic. Namely, it
does:

  if (state->eip >= (natural_t) &_hurd_intr_rpc_msg_cx_sp)
    state->uesp = state->ecx;
  else
    state->ecx = state->uesp;

but since hurdsig.c then makes the thread jump to
_hurd_intr_rpc_msg_in_trap, which was doing this:

_hurd_intr_rpc_msg_do_trap: lcall $7, $0
_hurd_intr_rpc_msg_in_trap: movl %ecx, %esp
                            .cfi_def_cfa_register %esp
_hurd_intr_rpc_msg_sp_restored:

...it makes no sense, at least as I'm seeing it, to set uesp = ecx,
since esp is going to be set back to ecx in a moment by the code
anyway. Setting ecx = uesp in the other branch makes much more sense
-- it essentially ensures that if movl %esp, %ecx has not been
executed yet, the movl %ecx, %esp will just no-op and not break
anything.

But that was all pre-1d20f33ff4fb634310f27493b7b87d0b20f4a0b0. Now,
while esp is still saved into ecx (with a comment: "TODO: remove this
ecx kludge, we don't need it any more"), there is code _before movl
%esp, %ecx_ that pushes args onto the stack, and the corresponding
code after the syscall that pops the args back off the stack. movl
%ecx, %esp is no longer present at all.

To me this reads like:
- the "ecx kludge" is indeed useless now, since esp is the same as ecx
during the syscall; but this also means that setting uesp = ecx or
vice versa will not break anything
- if hurdsig.c forcefully jumps the thread to
_hurd_intr_rpc_msg_in_trap before the thread is done pushing stuff
onto the stack, the thread will attempt to pop stuff off the stack
that was never pushed there => kaboom!

Or in other words: it seems to me that right now things would break
spectacularly if a signal arrives at just the right time. The logic
tried to handle this, but it's broken
post-1d20f33ff4fb634310f27493b7b87d0b20f4a0b0.

Does my reasoning make sense? Am I missing anything? Was this a simple
oversight of 1d20f33ff4fb634310f27493b7b87d0b20f4a0b0, or is there
some deeper plan that I'm not seeing to this? Are there any tests for
this at all?

As for fixing this:
- we should drop the ecx kludge indeed, meaning, remove everything
related to ecx from all of these files
- there should be an explicit way for hurdsig.c to tell INTR_MSG_TRAP
to not do the syscall. We could use the SYSRETURN register (eax),
which is to be clobbered anyway -- say, set it to 0 initially, and to
1 if the syscall is to be skipped
- INTR_MSG_TRAP should do this check *after* doing anything with the stack
- hurdsig.c would force-jump to after-the-syscall only if we're past
that check, like this:
if (PC >= _hurd_intr_rpc_msg_setup_done)
  {
    /* The thread has completed all the pre-syscall setup, so we
        can safely jump over the syscall */
    PC = &_hurd_intr_rpc_msg_in_trap;
  }
else
  {
    /* Cannot make the thread jump over the syscall, since the setup
is not yet complete, and
        this would confuse the code we would jump to. Instead, set the
flag that the thread will
        check just before doing the syscall, and skip the syscall itself */
    SYSRETURN = 1
  }
- INTR_MSG_BACK_OUT will become empty; we can still leave it in, or
drop it entirely

The above plan should work for any arch, not just i386. What do you think?

Sergey