[PATCH] perf: prevent kill(0, SIGTERM);

[PATCH] perf: prevent kill(0, SIGTERM);

[Ian Munsie]

From: Ian Munsie <imunsie@au1.ibm.com>

At exit, perf record will kill the process it was profiling by sending a
SIGTERM to child_pid (if it had been initialised), but in certain
situations child_pid may be 0 and perf would mistakenly kill more
processes than intended.

child_pid is set to the return of fork() to either 0 or the pid of the
child. Ordinarily this would not present an issue as the child calls
execvp to spawn the process to be profilled and would therefore never
run it's sig_atexit and never attempt to kill pid 0.

However, if a nonexistant binary had been passed in to perf record the
call to execvp would fail and child_pid would be left set to 0. The
child would then exit and it's atexit handler, finding that child_pid
was initialised to 0, would call kill(0, SIGTERM), resulting in every
process within it's process group being killed.

In the case that perf was being run directly from the shell this
typically would not be an issue as the shell isolates the process.
However, if perf was being called from another program it could kill
unexpected processes, which may even include X.

This patch changes the logic of the test for whether child_pid was
initialised to only considder positive pids as valid, thereby never
attempting to kill pid 0.

Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
---
 tools/perf/builtin-record.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 5e5c640..300da82 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -193,7 +193,7 @@ static void sig_handler(int sig)
 
 static void sig_atexit(void)
 {
-	if (child_pid != -1)
+	if (child_pid > 0)
 		kill(child_pid, SIGTERM);
 
 	if (signr == -1)
-- 
1.7.1

Re: [PATCH] perf: prevent kill(0, SIGTERM);

[Ian Munsie]

Hi Ingo,

Please consider merging this patch, it's a trivial fix to prevent perf
from killing too many processes in certain unusual situations - such as
killing the other intern's entire X session when called from eclipse
with a typo in the program being profiled.

Cheers,
-Ian

Excerpts from Ian Munsie's message of Wed Jun 09 18:38:00 +1000 2010:
> From: Ian Munsie <imunsie@au1.ibm.com>
> 
> At exit, perf record will kill the process it was profiling by sending a
> SIGTERM to child_pid (if it had been initialised), but in certain
> situations child_pid may be 0 and perf would mistakenly kill more
> processes than intended.
> 
> child_pid is set to the return of fork() to either 0 or the pid of the
> child. Ordinarily this would not present an issue as the child calls
> execvp to spawn the process to be profilled and would therefore never
> run it's sig_atexit and never attempt to kill pid 0.
> 
> However, if a nonexistant binary had been passed in to perf record the
> call to execvp would fail and child_pid would be left set to 0. The
> child would then exit and it's atexit handler, finding that child_pid
> was initialised to 0, would call kill(0, SIGTERM), resulting in every
> process within it's process group being killed.
> 
> In the case that perf was being run directly from the shell this
> typically would not be an issue as the shell isolates the process.
> However, if perf was being called from another program it could kill
> unexpected processes, which may even include X.
> 
> This patch changes the logic of the test for whether child_pid was
> initialised to only considder positive pids as valid, thereby never
> attempting to kill pid 0.
> 
> Signed-off-by: Ian Munsie <imunsie@au1.ibm.com>
> ---
>  tools/perf/builtin-record.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 5e5c640..300da82 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -193,7 +193,7 @@ static void sig_handler(int sig)
>  
>  static void sig_atexit(void)
>  {
> -    if (child_pid != -1)
> +    if (child_pid > 0)
>          kill(child_pid, SIGTERM);
>  
>      if (signr == -1)

Re: [PATCH] perf: prevent kill(0, SIGTERM);

[Arnaldo Carvalho de Melo]

Em Wed, Jun 09, 2010 at 06:38:00PM +1000, Ian Munsie escreveu:
> At exit, perf record will kill the process it was profiling by sending a
> SIGTERM to child_pid (if it had been initialised), but in certain
> situations child_pid may be 0 and perf would mistakenly kill more
> processes than intended.
> 
> child_pid is set to the return of fork() to either 0 or the pid of the
> child. Ordinarily this would not present an issue as the child calls
> execvp to spawn the process to be profilled and would therefore never
> run it's sig_atexit and never attempt to kill pid 0.

before:

[root@emilia mingo]# perf record bla
bla: No such file or directory
Terminated

after:

[root@emilia mingo]# perf record bla
bla: No such file or directory
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.006 MB perf.data (~259 samples) ]
[root@emilia mingo]#

Got it, queuing it in perf/urgent,

Thanks!

- Arnaldo