Re: ptrace.2: PTRACE_KILL needs a stopped process too

Re: ptrace.2: PTRACE_KILL needs a stopped process too

[Michael Kerrisk (man-pages)]

[widening CC]

Denys, (Oleg, Tejun,)

Would you be able to take a look at this report that is now already a
long time in my inbox. Is there something here that still needs to be
fixed after your recent changes to ptrace.2, Denys?

Thanks,

Michael



---------- Forwarded message ----------
From:  <pacman@kosh.dhis.org>
Date: Wed, Dec 16, 2009 at 1:45 PM
Subject: ptrace.2: PTRACE_KILL needs a stopped process too
To: mtk.manpages@gmail.com


The man page says "For requests other than PTRACE_KILL, the child process
must be stopped."

But experience[1] shows that PTRACE_KILL also requires the child process to
be stopped. gdb older than 7.0 doesn't always make sure its child process is
stopped before attempting a PTRACE_KILL (try SIGTERMing gdb while the child
process is running).

That was fixed in gdb 7.0 by the addition of a SIGSTOP and wait before the
PTRACE_KILL, so the kernel and latest gdb get along well now, which leaves
the man page as the only thing that's out of sync.

If the man page is describing actual intended kernel behavior, then it's a
fairly long-standing kernel bug. But I get the feeling this was just an
assumption by the original man page author that never got checked.

[1] http://lists.gnu.org/archive/html/bug-coreutils/2009-11/msg00267.html
   http://lists.gnu.org/archive/html/bug-coreutils/2009-12/msg00025.html
   http://lists.gnu.org/archive/html/bug-coreutils/2009-12/msg00094.html


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

Re: ptrace.2: PTRACE_KILL needs a stopped process too

[Oleg Nesterov]

On 04/23, Michael Kerrisk (man-pages) wrote:
>
> [widening CC]

add more CC's

> The man page says "For requests other than PTRACE_KILL,

Argh, PTRACE_KILL again.

You know, I simply do not know what it was supposed to do. I can only
see what the code actually does.

> the child process
> must be stopped."

Yes and no.

Yes, ptrace(PTRACE_KILL) "succeeds" even if the tracee is not stopped.

No, it has no effect if the tracee is not stopped.

All I can say is: PTRACE_KILL should never exist. If you want to kill
the tracee, you can do kill(SIGKILL).

Roughly, ptrace(PTRACE_KILL) is equal to ptrace(PTRACE_CONT, SIGKILL)
except it always returns 0.

> If the man page is describing actual intended kernel behavior, then it's a
> fairly long-standing kernel bug.

Perhaps. May be it should simply do kill(SIGKILL), but then it is not
clear why do we have PTRACE_KILL. And once again, I was never able to
understand the supposed behaviour.

Personally, I think we should fix the documentation. And imho the only
possible fix is to add this note: do not ever use PTRACE_KILL.

Oleg.

Re: ptrace.2: PTRACE_KILL needs a stopped process too

[Pedro Alves]

On 04/22/2012 09:04 PM, Oleg Nesterov wrote:

> On 04/23, Michael Kerrisk (man-pages) wrote:
>>
>> [widening CC]
> 
> add more CC's
> 
>> The man page says "For requests other than PTRACE_KILL,
> 
> Argh, PTRACE_KILL again.
> 
> You know, I simply do not know what it was supposed to do. I can only
> see what the code actually does.
> 
>> the child process
>> must be stopped."
> 
> Yes and no.
> 
> Yes, ptrace(PTRACE_KILL) "succeeds" even if the tracee is not stopped.
> 
> No, it has no effect if the tracee is not stopped.
> 
> All I can say is: PTRACE_KILL should never exist. If you want to kill
> the tracee, you can do kill(SIGKILL).
> 
> Roughly, ptrace(PTRACE_KILL) is equal to ptrace(PTRACE_CONT, SIGKILL)
> except it always returns 0.
> 
>> If the man page is describing actual intended kernel behavior, then it's a
>> fairly long-standing kernel bug.
> 
> Perhaps. May be it should simply do kill(SIGKILL), but then it is not
> clear why do we have PTRACE_KILL. And once again, I was never able to
> understand the supposed behaviour.
> 
> Personally, I think we should fix the documentation. And imho the only
> possible fix is to add this note: do not ever use PTRACE_KILL.


Yeah.  Here's what gdb's gdbserver does nowadays (gdb does the exact same,
though only gdbserver's version has this comment):

static void
linux_kill_one_lwp (struct lwp_info *lwp)
{
  int pid = lwpid_of (lwp);

  /* PTRACE_KILL is unreliable.  After stepping into a signal handler,
     there is no signal context, and ptrace(PTRACE_KILL) (or
     ptrace(PTRACE_CONT, SIGKILL), pretty much the same) acts like
     ptrace(CONT, pid, 0,0) and just resumes the tracee.  A better
     alternative is to kill with SIGKILL.  We only need one SIGKILL
     per process, not one for each thread.  But since we still support
     linuxthreads, and we also support debugging programs using raw
     clone without CLONE_THREAD, we send one for each thread.  For
     years, we used PTRACE_KILL only, so we're being a bit paranoid
     about some old kernels where PTRACE_KILL might work better
     (dubious if there are any such, but that's why it's paranoia), so
     we try SIGKILL first, PTRACE_KILL second, and so we're fine
     everywhere.  */

  errno = 0;
  kill (pid, SIGKILL);
  if (debug_threads)
    fprintf (stderr,
	     "LKL:  kill (SIGKILL) %s, 0, 0 (%s)\n",
	     target_pid_to_str (ptid_of (lwp)),
	     errno ? strerror (errno) : "OK");

  errno = 0;
  ptrace (PTRACE_KILL, pid, 0, 0);
  if (debug_threads)
    fprintf (stderr,
	     "LKL:  PTRACE_KILL %s, 0, 0 (%s)\n",
	     target_pid_to_str (ptid_of (lwp)),
	     errno ? strerror (errno) : "OK");
}

-- 
Pedro Alves

Re: ptrace.2: PTRACE_KILL needs a stopped process too

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1468 bytes --]

On Sunday 22 April 2012 16:04:59 Oleg Nesterov wrote:
> On 04/23, Michael Kerrisk (man-pages) wrote:
> > [widening CC]
> 
> add more CC's
> 
> > The man page says "For requests other than PTRACE_KILL,
> 
> Argh, PTRACE_KILL again.
> 
> You know, I simply do not know what it was supposed to do. I can only
> see what the code actually does.
> 
> > the child process
> > must be stopped."
> 
> Yes and no.
> 
> Yes, ptrace(PTRACE_KILL) "succeeds" even if the tracee is not stopped.
> 
> No, it has no effect if the tracee is not stopped.
> 
> All I can say is: PTRACE_KILL should never exist. If you want to kill
> the tracee, you can do kill(SIGKILL).
> 
> Roughly, ptrace(PTRACE_KILL) is equal to ptrace(PTRACE_CONT, SIGKILL)
> except it always returns 0.
> 
> > If the man page is describing actual intended kernel behavior, then it's
> > a fairly long-standing kernel bug.
> 
> Perhaps. May be it should simply do kill(SIGKILL), but then it is not
> clear why do we have PTRACE_KILL. And once again, I was never able to
> understand the supposed behaviour.
> 
> Personally, I think we should fix the documentation. And imho the only
> possible fix is to add this note: do not ever use PTRACE_KILL.

probably not that big of a deal, but the reason i like using 
ptrace(PTRACE_KILL) over a raw kill() is that you are less likely to kill the 
wrong process by accident.  maybe not that big of a deal in practice though.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: ptrace.2: PTRACE_KILL needs a stopped process too

[Pedro Alves]

On 05/09/2012 04:09 PM, Mike Frysinger wrote:

> probably not that big of a deal, but the reason i like using 
> ptrace(PTRACE_KILL) over a raw kill() is that you are less likely to kill the 
> wrong process by accident.  maybe not that big of a deal in practice though.


And you can do tgkill instead.  It was specifically invented to handle the
reuse case.

-- 
Pedro Alves

Re: ptrace.2: PTRACE_KILL needs a stopped process too

[Oleg Nesterov]

On 05/09, Pedro Alves wrote:
>
> On 05/09/2012 04:09 PM, Mike Frysinger wrote:
>
> > probably not that big of a deal, but the reason i like using
> > ptrace(PTRACE_KILL) over a raw kill() is that you are less likely to kill the
> > wrong process by accident.  maybe not that big of a deal in practice though.
>
>
> And you can do tgkill instead.  It was specifically invented to handle the
> reuse case.

tgkill() can kill the wrong process/thread too, although it lessens the risk.

But I don't really understand the problem. The traced thread can't go away
until the tracer does wait/detach, and thus its pid can't be reused?

May be, "by accident" above means something else, not pid reuse...

Oleg.

Re: ptrace.2: PTRACE_KILL needs a stopped process too

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1011 bytes --]

On Wednesday 09 May 2012 16:12:19 Oleg Nesterov wrote:
> On 05/09, Pedro Alves wrote:
> > On 05/09/2012 04:09 PM, Mike Frysinger wrote:
> > > probably not that big of a deal, but the reason i like using
> > > ptrace(PTRACE_KILL) over a raw kill() is that you are less likely to
> > > kill the wrong process by accident.  maybe not that big of a deal in
> > > practice though.
> > 
> > And you can do tgkill instead.  It was specifically invented to handle
> > the reuse case.
> 
> tgkill() can kill the wrong process/thread too, although it lessens the
> risk.
> 
> But I don't really understand the problem. The traced thread can't go away
> until the tracer does wait/detach, and thus its pid can't be reused?

or the process has received a SIGKILL for some reason

> May be, "by accident" above means something else, not pid reuse...

i like to assume that my code isn't going to be bug free, so the more 
mechanisms i have in place to protect innocent bystanders the better :)
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: ptrace.2: PTRACE_KILL needs a stopped process too

[Oleg Nesterov]

On 05/09, Mike Frysinger wrote:
>
> On Wednesday 09 May 2012 16:12:19 Oleg Nesterov wrote:
> > On 05/09, Pedro Alves wrote:
> > > On 05/09/2012 04:09 PM, Mike Frysinger wrote:
> > > > probably not that big of a deal, but the reason i like using
> > > > ptrace(PTRACE_KILL) over a raw kill() is that you are less likely to
> > > > kill the wrong process by accident.  maybe not that big of a deal in
> > > > practice though.
> > >
> > > And you can do tgkill instead.  It was specifically invented to handle
> > > the reuse case.
> >
> > tgkill() can kill the wrong process/thread too, although it lessens the
> > risk.
> >
> > But I don't really understand the problem. The traced thread can't go away
> > until the tracer does wait/detach, and thus its pid can't be reused?
>
> or the process has received a SIGKILL for some reason

And? In this case it will be killed, yes. But this zombie can't go away
until the tracer does do_wait().

OK, the multi-threaded exec adds more fun.

> > May be, "by accident" above means something else, not pid reuse...
>
> i like to assume that my code isn't going to be bug free, so the more
> mechanisms i have in place to protect innocent bystanders the better :)
> -mike

>From this pov PTRACE_KILL is safer, I agree ;)

Oleg.

Re: ptrace.2: PTRACE_KILL needs a stopped process too

[Pedro Alves]

On 05/09/2012 10:14 PM, Oleg Nesterov wrote:

>> i like to assume that my code isn't going to be bug free, so the more
>> mechanisms i have in place to protect innocent bystanders the better :)
>> -mike
> 
> From this pov PTRACE_KILL is safer, I agree ;)


Yeah, until you trip on that case where it resumes the process
instead of killing it...  At that point, it's useless.

Since I know Mike likes hacking on GDB so much ;-), FYI, this is
exercised by the gdb.base/kill-after-signal.exp test.

-- 
Pedro Alves

Re: ptrace.2: PTRACE_KILL needs a stopped process too

[Pedro Alves]

On 05/09/2012 09:12 PM, Oleg Nesterov wrote:

> tgkill() can kill the wrong process/thread too, although it lessens the risk.

> 
> But I don't really understand the problem. The traced thread can't go away

> until the tracer does wait/detach, and thus its pid can't be reused?


There's the non-leader thread execs and tracer didn't enable
PTRACE_O_TRACEEXEC case at least, while you try to kill the thread that just
execed, I think.  Though that's quite pedantic.  The tracer could prevent this
in other ways.

> May be, "by accident" above means something else, not pid reuse...


Pffft, who writes buggy code anyway?  ;-)

-- 
Pedro Alves