[seccomp-sandbox] Fix test_prctl so that it does not leave behind a zombie process

Fix test_prctl so that it does not leave behind a stopped process

The stuck process means that running tests over ssh causes ssh to hang
when the tests are finished, because ssh never gets EOF from stdout.

The problem was apparently that PTRACE_KILL does not kill the
subprocess unless you have cleared the wait status queue for the
subprocess first.

Committed: http://code.google.com/p/seccompsandbox/source/detail?r=102

[Markus (顧孟勤), 2010-09-03 16:57:32]

http://codereview.chromium.org/3363006/diff/1/2
File tests/test_syscalls.cc (right):

http://codereview.chromium.org/3363006/diff/1/2#newcode710
tests/test_syscalls.cc:710: kill(pid, SIGKILL);
I suspect that PTRACE_KILL would actually work. But I definitely omitted a few
things that a full-fledged tracing application would normally need.

Most of the ptrace() calls must normally be followed by a call to wait(). I am
not sure if this is true for the initial PTRACE_ATTACH, but I suspect so.

Also, after a PTRACE_KILL, we probably still need the PTRACE_DETACH. I wouldn't
be surprised, if that's actually the root cause of the problem that you are
seeing. Can you try that?

http://codereview.chromium.org/3363006/diff/1/2#newcode716
tests/test_syscalls.cc:716: assert(WTERMSIG(status) == SIGKILL);
I was trying to keep the test simple and left out the wait(), as "init" should
be reaping our zombie. But maybe, that was simplifying things too much. You code
is obviously more correct -- albeit a little more verbose.

If you discover that adding the PTRACE_DETACH fixes the problem, I'll leave it
up to your judgement whether you want to leave the wait() in the code or not.

[Mark Seaborn, 2010-09-03 17:19:42]

On 3 September 2010 09:57, <markus@chromium.org> wrote:

> http://codereview.chromium.org/3363006/diff/1/2
> File tests/test_syscalls.cc (right):
>
> http://codereview.chromium.org/3363006/diff/1/2#newcode710
> tests/test_syscalls.cc:710: kill(pid, SIGKILL);
> I suspect that PTRACE_KILL would actually work. But I definitely omitted
> a few things that a full-fledged tracing application would normally
> need.
>
> Most of the ptrace() calls must normally be followed by a call to
> wait(). I am not sure if this is true for the initial PTRACE_ATTACH, but
> I suspect so.
>
> Also, after a PTRACE_KILL, we probably still need the PTRACE_DETACH. I
> wouldn't be surprised, if that's actually the root cause of the problem
> that you are seeing. Can you try that?
>

Actually that was the first thing I tried.  If I call PTRACE_DETACH after
PTRACE_KILL, it returns ESRCH (No such process).  That is a bit odd,
actually, given that the process hasn't been waited for.  But ptrace is odd
anyway.

If I put in a waitpid() call without doing kill(), waitpid() indicates that
the process was stopped, not killed.


>
> http://codereview.chromium.org/3363006/diff/1/2#newcode716
> tests/test_syscalls.cc:716: assert(WTERMSIG(status) == SIGKILL);
> I was trying to keep the test simple and left out the wait(), as "init"
> should be reaping our zombie. But maybe, that was simplifying things too
> much. You code is obviously more correct -- albeit a little more
> verbose.
>

Strictly speaking it's not a zombie -- I used the wrong term.  A run of
"make test" leaves behind processes in these states:

$ ps aux | grep run_tests
mseaborn 21310  0.2  0.0  16780  1668 ?        Tl   18:14   0:00
./run_tests_64
mseaborn 21311  0.0  0.0  16864  1028 ?        S    18:14   0:00
./run_tests_64
mseaborn 21477  0.2  0.0   3848  1640 ?        Tl   18:14   0:00
./run_tests_32
mseaborn 21478  0.0  0.0   3964   984 ?        S    18:14   0:00
./run_tests_32
mseaborn 21644  0.2  0.0  16784  1672 ?        Tl   18:14   0:00
./run_tests_64
mseaborn 21645  0.0  0.0  16864  1032 ?        S    18:14   0:00
./run_tests_64
mseaborn 21811  0.2  0.0   3852  1644 ?        Tl   18:14   0:00
./run_tests_32
mseaborn 21812  0.0  0.0   3964   988 ?        S    18:14   0:00
./run_tests_32
mseaborn 21818  0.0  0.0   3944   624 pts/1    R+   18:14   0:00 grep
run_tests

pstree shows them as:

  |-run_tests_32,21477
  |   |-run_tests_32,21478
  |   `-{run_tests_32},21479
  |-run_tests_32,21811
  |   |-run_tests_32,21812
  |   `-{run_tests_32},21813
  |-run_tests_64,21310
  |   |-run_tests_64,21311
  |   `-{run_tests_64},21312
  |-run_tests_64,21644
  |   |-run_tests_64,21645
  |   `-{run_tests_64},21646

Mark

[Mark Seaborn, 2010-09-06 10:07:55]

On 3 September 2010 19:40, Markus Gutschke <markus@chromium.org> wrote:

> I think the way how I have usually done this required that I call wait()
> with the __WALL flag. This had to happen after (almost) every call to
> ptrace(). And yes, I think it most of the time it would indicate that the
> child was stopped.
>
> I would not be entirely surprised if the kernel got "out-of-step" if we
> missed one of the earlier calls to wait(). As you said, ptrace() is a very
> strange API.
>

OK, I have changed it back to using PTRACE_KILL, but with a waitpid() call
before that to collect the stopped status.  That works, and it terminates
the process successfully.

Mark

[Markus (顧孟勤), 2010-09-06 15:37:47]

LGTM