detach() logic

[Denys Vlasenko]

On 06/19/2013 02:35 PM, Dmitry V. Levin wrote:
> On Wed, Jun 19, 2013 at 01:11:56PM +0200, Denys Vlasenko wrote:
>> Dmitry, I am a bit worried about the flow in this function still.
>> Let's take a look:
>>
>>                 error = ptrace(PTRACE_DETACH, tcp->pid, 0, 0);
>>                 if (error == 0) {
>>                         /* On a clear day, you can see forever. */
>>                 }
>>                 else if (errno != ESRCH) {
>>                         /* Shouldn't happen. */
>>                         perror_msg("detach: ptrace(PTRACE_DETACH, ...)");
>>                 }
>>                 else
>>                 /* ESRCH: process is either not stopped or doesn't exist. */
>>                 if (my_tkill(tcp->pid, 0) < 0) {
>>                         if (errno != ESRCH)
>>                                 /* Shouldn't happen. */
>>                                 perror_msg("detach: checking sanity");
>>                         /* else: process doesn't exist. */
>> ^^^^^^^^^^^^^^^^^
>> Well, it may not exist already, but was it *waited for*?
>> IOW: we may still need to enter waitpid loop.
>> This may rarely trigger - say, we do "strace -p PROCESS",
>> and process exits just as we ^C the strace,
>> and we may end up here.
>> OTOH, not-waited-for child reparents to init when we exit,
>> so... do we ever detach() NOT not strace exit, where dead
>> children are a problem? I see one location:
>>   if (event == PTRACE_EVENT_EXEC) {
>>       if (detach_on_execve && !skip_one_b_execve)
>>               detach(tcp); /* do "-b execve" thingy */
>> Maybe in the name of correctness we should wait for the process
>> if we see ESRCH? Possibly with WHOHANG for paranoid reasons.
> 
> In case of "-b execve", the tracee is in syscall-stop state already, so

To nitpick, it is in PTRACE_EVENT_EXEC stop...

...or rather, we only know that it *was* in PTRACE_EVENT_EXEC.

It may no longer be true if it was suddenly nuked by SIGKILL
a microsecond later while we are calling detach() on it.

Then DETACH fails with ESRCH, tkill(0) fails with ESRCH (I guess...),
and with current code we do nothing, leaving a zombie.

Actually, that may be a good thing, we want *its parent*
to consume its exit status. But that parent can be *us* if we act
on our own child.

> PTRACE_DETACH should succeed and there should be no need to wait (and if
> PTRACE_DETACH failed, then the tracee is no more so strace is expected
> to wait for it).

My point is, we *dont* wait if both DETACH and probing tkill(0)
fail with ESRCH. This might be wrong in some situations.
In any case, it needs commenting in the source, it is very confusing.

> I see no reason why detach() called from cleanup() should have to call
> PTRACE_DETACH for TCB_ATTACHED tracees.  At strace exit, those tracees
> that have TCB_IGNORE_ONE_SIGSTOP bit set (can happen in !use_seize case)
> have to be waited for SIGSTOP and PTRACE_CONT'ed (otherwise they would be
> left stopped after strace exit), all the rest TCB_ATTACHED tracees should
> be fine as they are.

True (modulo possible kernel bugs...).
But it would be nice if detach(tcp) works correctly even if strace
plans to continue running after it detached this one tracee.

-- 
vda