[patch] Break by Ctrl-C before first syscall SIGSTOPs the traced process

[Roland McGrath]

> 	(main): Move signals and BLOCKED_SET init before the options parsing.

This is inaccurate.  It's after options parsing and before process attach/fork.

> 	[LINUX] (main): waitstop () once after each succeeded PTRACE_ATTACH.
> 	SIG_BLOCK signals of BLOCKED_SET between PTRACE_ATTACH - waitstop ().

Serializing the attach reports at startup feels wrong, and it could get
painfully slow when attaching to a horde of threads under high CPU load.
Also, I think there is a variant of this bug that can strike with -f.
internal_{fork,clone} also use PTRACE_ATTACH.  An interrupt after this but
before the SIGSTOP is seen can have the same effect.  Serializing there
could be done using wait4 with the specific PID, and would probably be less
bad in the worst case than the startup serialization's worst case.  But I
don't think it's desireable to serialize at all.

I don't really see why we block those signals (even in the existing code),
since the handler just sets a flag.  It protects us against EINTR in our
ptrace and printing calls.  But if we just set the handler with SA_RESTART
then I think that should all work fine.  But there is no reason to perturb
this part of the code, and your blocking the signals around the ptrace
calls at startup is consistent.  There were probably hysterical raisins.

The nonserializing fix is to keep track that we are still expecting a
SIGSTOP and make sure we've eaten it when we detach.  We already have a
flag set at the right time that tells us this: TCB_STARTUP.  (When
PTRACE_ATTACH was used, we always have TCB_STARTUP|TCB_ATTACHED.  Our
natural child (no -p) has TCB_STARTUP without TCB_ATTACHED, but in fact is
in the same condition because it sends itself a SIGSTOP before it execs.)

I suggest moving that startup code into a couple of functions,
so main just does:

	if (pflag_seen)
		startup_attach();
	else
		startup_child(&argv[optind]);

Then move this after the signal setup, right before trace().

If startup_attach blocks signals, it should unblock them momentarily after
each ptrace call to let an interrupt in.  After each attach and alloctcb,
check if interrupted is set.  If so, startup_attach returns early without
attaching to all the threads.  main can then go into trace() normally,
which has to be safe with interrupted set (or else there are still bugs).

trace() should check interrupted only after handling the wait status, so
its bookkeeping about whether it waited for the SIGSTOP won't go wrong.
The safe thing looks to be: check interrupted at the top of the loop,
before wait4; check interrupted right before calling trace_syscall.
Otherwise, the meaning of the wait4 result can be lost when we bail (and we
either lose track that we have seen and swallowed the expected SIGSTOP, or
else we swallow another signal that should have gone through).
The current code has that bug, AFAICS.

The TCB_STARTUP check in trace() is what swallows the startup SIGSTOP
normally.  As you noted, there might be a different signal first, and that
signal should not be swallowed, though the later SIGSTOP still should be
(if the process survives the earlier signals).  There is this related code:

			if (tcp->flags & TCB_ATTACHED) {
				/*
				 * Interestingly, the process may stop
				 * with STOPSIG equal to some other signal
				 * than SIGSTOP if we happend to attach
				 * just before the process takes a signal.
				 */
				if (!WIFSTOPPED(status)) {
					fprintf(stderr,
						"pid %u not stopped\n", pid);
					detach(tcp, WSTOPSIG(status));
					continue;
				}
			}

That comment talks about the issue, but the code is wrong (and dead)
because we never get here if !WIFSTOPPED.  I think what would make it right
is changing:

		if (tcp->flags & TCB_STARTUP) {

to:

		if ((tcp->flags & TCB_STARTUP) && WSTOPSIG(status) == SIGSTOP) {

and getting rid of that screwy TCB_ATTACHED case there.

Then other signals will fall through to the normal code below, that will
print them and pass them through.  On the next go around, we'll see the
SIGSTOP and do the TCB_STARTUP case, or else bail before wait4 if interrupted.

Finally, cleanup (i.e. detach) has check for TCB_STARTUP and know it should
not add another SIGSTOP, but must do wait4's and until it can swallow the
expected SIGSTOP (which it already does).  The !TCB_ATTACHED case in
cleanup is ok already, because sending the child SIGCONT clears all stop
signals, so its pending SIGSTOP will disappear.


Does this all make sense to you?  If so, do you think it's correct?
If so, can you try a patch doing what I've described?


Thanks,
Roland