[1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

[1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

[Ivan Efimov]

The bitbake-worker child on the SIGTERM signal handling send the SIGTERM to all
processes in it's process group. In cases when the bitbake-worker child got
SIGTERM after registering own SIGTERM handler and before the os.setsid() call
it can send SIGTERM to unwanted processes.

In the worst case during SIGTERM processing the bitbake-worker child can be in
the group of the process that started BitBake itself. As a result it can kill
processes that not related to BitBake at all.

Signed-off-by: Ivan Efimov <i.efimov@inango-systems.com>
---
 bin/bitbake-worker | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/bin/bitbake-worker b/bin/bitbake-worker
index db3c4b18..6a12a31c 100755
--- a/bin/bitbake-worker
+++ b/bin/bitbake-worker
@@ -189,9 +189,6 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, append
             global worker_pipe_lock
             pipein.close()
 
-            signal.signal(signal.SIGTERM, sigterm_handler)
-            # Let SIGHUP exit as SIGTERM
-            signal.signal(signal.SIGHUP, sigterm_handler)
             bb.utils.signal_on_parent_exit("SIGTERM")
 
             # Save out the PID so that the event can include it the
@@ -206,6 +203,11 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, append
             # This ensures signals sent to the controlling terminal like Ctrl+C
             # don't stop the child processes.
             os.setsid()
+
+            signal.signal(signal.SIGTERM, sigterm_handler)
+            # Let SIGHUP exit as SIGTERM
+            signal.signal(signal.SIGHUP, sigterm_handler)
+
             # No stdin
             newsi = os.open(os.devnull, os.O_RDWR)
             os.dup2(newsi, sys.stdin.fileno())
-- 
2.17.1

Re: [1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

[akuster]

On 11/5/19 6:07 AM, Ivan Efimov via bitbake-devel wrote:
> The bitbake-worker child on the SIGTERM signal handling send the SIGTERM to all
> processes in it's process group. In cases when the bitbake-worker child got
> SIGTERM after registering own SIGTERM handler and before the os.setsid() call
> it can send SIGTERM to unwanted processes.
>
> In the worst case during SIGTERM processing the bitbake-worker child can be in
> the group of the process that started BitBake itself. As a result it can kill
> processes that not related to BitBake at all.
>
> Signed-off-by: Ivan Efimov <i.efimov@inango-systems.com>

I see master and 1.32 have patches. To confirm, 1.44, 140.1.38, 1.36,...
need this too?

- Armin
> ---
>  bin/bitbake-worker | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/bin/bitbake-worker b/bin/bitbake-worker
> index db3c4b18..6a12a31c 100755
> --- a/bin/bitbake-worker
> +++ b/bin/bitbake-worker
> @@ -189,9 +189,6 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, append
>              global worker_pipe_lock
>              pipein.close()
>  
> -            signal.signal(signal.SIGTERM, sigterm_handler)
> -            # Let SIGHUP exit as SIGTERM
> -            signal.signal(signal.SIGHUP, sigterm_handler)
>              bb.utils.signal_on_parent_exit("SIGTERM")
>  
>              # Save out the PID so that the event can include it the
> @@ -206,6 +203,11 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, append
>              # This ensures signals sent to the controlling terminal like Ctrl+C
>              # don't stop the child processes.
>              os.setsid()
> +
> +            signal.signal(signal.SIGTERM, sigterm_handler)
> +            # Let SIGHUP exit as SIGTERM
> +            signal.signal(signal.SIGHUP, sigterm_handler)
> +
>              # No stdin
>              newsi = os.open(os.devnull, os.O_RDWR)
>              os.dup2(newsi, sys.stdin.fileno())

Re: [1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

[Ivan Efimov]

Hi Armin,

I've rechecked that this issue is relevant to next branches besides 1.32 and master:

1.26
1.28
1.30
1.34
1.36
1.38
1.40
1.42
1.44

Could you please tell me what of them are still supported by BitBake community?

Best regards,
Ivan



[ https://inango.com/ |    ] 
Ivan Efimov 
Software Architect, Arch Group 
i.efimov@inango-systems.com 
T +7 343 298 0147 C +7 904 9 888 350 
[ https://inango.com/ | inango.com ] [ https://www.linkedin.com/company/inango ]

----- Original Message -----
From: "akuster" <akuster@mvista.com>
To: "Ivan Efimov" <i.efimov@inango-systems.com>, bitbake-devel@lists.openembedded.org
Sent: Thursday, November 7, 2019 3:00:46 AM
Subject: Re: [bitbake-devel] [1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

On 11/5/19 6:07 AM, Ivan Efimov via bitbake-devel wrote:
> The bitbake-worker child on the SIGTERM signal handling send the SIGTERM to all
> processes in it's process group. In cases when the bitbake-worker child got
> SIGTERM after registering own SIGTERM handler and before the os.setsid() call
> it can send SIGTERM to unwanted processes.
>
> In the worst case during SIGTERM processing the bitbake-worker child can be in
> the group of the process that started BitBake itself. As a result it can kill
> processes that not related to BitBake at all.
>
> Signed-off-by: Ivan Efimov <i.efimov@inango-systems.com>

I see master and 1.32 have patches. To confirm, 1.44, 140.1.38, 1.36,...
need this too?

- Armin
> ---
>  bin/bitbake-worker | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/bin/bitbake-worker b/bin/bitbake-worker
> index db3c4b18..6a12a31c 100755
> --- a/bin/bitbake-worker
> +++ b/bin/bitbake-worker
> @@ -189,9 +189,6 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, append
>              global worker_pipe_lock
>              pipein.close()
>  
> -            signal.signal(signal.SIGTERM, sigterm_handler)
> -            # Let SIGHUP exit as SIGTERM
> -            signal.signal(signal.SIGHUP, sigterm_handler)
>              bb.utils.signal_on_parent_exit("SIGTERM")
>  
>              # Save out the PID so that the event can include it the
> @@ -206,6 +203,11 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, append
>              # This ensures signals sent to the controlling terminal like Ctrl+C
>              # don't stop the child processes.
>              os.setsid()
> +
> +            signal.signal(signal.SIGTERM, sigterm_handler)
> +            # Let SIGHUP exit as SIGTERM
> +            signal.signal(signal.SIGHUP, sigterm_handler)
> +
>              # No stdin
>              newsi = os.open(os.devnull, os.O_RDWR)
>              os.dup2(newsi, sys.stdin.fileno())

Re: [1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

[Richard Purdie]

On Thu, 2019-11-07 at 08:35 +0200, Ivan Efimov via bitbake-devel wrote:
> Hi Armin,
> 
> I've rechecked that this issue is relevant to next branches besides
> 1.32 and master:
> 
> 1.26
> 1.28
> 1.30
> 1.34
> 1.36
> 1.38
> 1.40
> 1.42
> 1.44
> 
> Could you please tell me what of them are still supported by BitBake
> community?

I think 1.40 onwards are still supported. I've just applied to patch to
those branches since its a clear/obvious fix and a clean backport. I
think I can do that in my capacity as bitbake maintainer! I also added
it to 1.38 since it helps with some other experiments I'm doing. I'm
reluctant to go further back as we have no testing capacity for the
older releases right now.

Cheers,

Richard

Re: [1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

[Ivan Efimov]

Thanks a lot, Richard.

Best regards,
Ivan

[ https://inango.com/ |    ] 
Ivan Efimov 
Software Architect, Arch Group 
i.efimov@inango-systems.com 
T +7 343 298 0147 C +7 904 9 888 350 
[ https://inango.com/ | inango.com ] [ https://www.linkedin.com/company/inango ]

----- Original Message -----
From: "Richard Purdie" <richard.purdie@linuxfoundation.org>
To: "Ivan Efimov" <i.efimov@inango-systems.com>, "akuster" <akuster@mvista.com>
Cc: "bitbake-devel" <bitbake-devel@lists.openembedded.org>
Sent: Friday, November 8, 2019 12:50:13 AM
Subject: Re: [bitbake-devel] [1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

On Thu, 2019-11-07 at 08:35 +0200, Ivan Efimov via bitbake-devel wrote:
> Hi Armin,
> 
> I've rechecked that this issue is relevant to next branches besides
> 1.32 and master:
> 
> 1.26
> 1.28
> 1.30
> 1.34
> 1.36
> 1.38
> 1.40
> 1.42
> 1.44
> 
> Could you please tell me what of them are still supported by BitBake
> community?

I think 1.40 onwards are still supported. I've just applied to patch to
those branches since its a clear/obvious fix and a clean backport. I
think I can do that in my capacity as bitbake maintainer! I also added
it to 1.38 since it helps with some other experiments I'm doing. I'm
reluctant to go further back as we have no testing capacity for the
older releases right now.

Cheers,

Richard

Re: [1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

[Boris Shehter]

Hi Richard 
We intensive are using yocto daisy and morty and we will be happy to test this fix on those version . Let say - we already tested. How can we integrate fix in relevant versions or only maintainer can do this  ? 


--------
Boris Shehter
(c) +972-54-6604528

----- Original Message -----
From: "Ivan Efimov" <i.efimov@inango-systems.com>
To: "Richard Purdie" <richard.purdie@linuxfoundation.org>
Cc: "akuster" <akuster@mvista.com>, "bitbake-devel" <bitbake-devel@lists.openembedded.org>
Sent: Friday, November 8, 2019 6:48:09 AM
Subject: Re: [bitbake-devel] [1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

Thanks a lot, Richard.

Best regards,
Ivan

[ https://inango.com/ |    ] 
Ivan Efimov 
Software Architect, Arch Group 
i.efimov@inango-systems.com 
T +7 343 298 0147 C +7 904 9 888 350 
[ https://inango.com/ | inango.com ] [ https://www.linkedin.com/company/inango ]

----- Original Message -----
From: "Richard Purdie" <richard.purdie@linuxfoundation.org>
To: "Ivan Efimov" <i.efimov@inango-systems.com>, "akuster" <akuster@mvista.com>
Cc: "bitbake-devel" <bitbake-devel@lists.openembedded.org>
Sent: Friday, November 8, 2019 12:50:13 AM
Subject: Re: [bitbake-devel] [1.32][PATCH] bitbake-worker child process create group before registering SIGTERM handler

On Thu, 2019-11-07 at 08:35 +0200, Ivan Efimov via bitbake-devel wrote:
> Hi Armin,
> 
> I've rechecked that this issue is relevant to next branches besides
> 1.32 and master:
> 
> 1.26
> 1.28
> 1.30
> 1.34
> 1.36
> 1.38
> 1.40
> 1.42
> 1.44
> 
> Could you please tell me what of them are still supported by BitBake
> community?

I think 1.40 onwards are still supported. I've just applied to patch to
those branches since its a clear/obvious fix and a clean backport. I
think I can do that in my capacity as bitbake maintainer! I also added
it to 1.38 since it helps with some other experiments I'm doing. I'm
reluctant to go further back as we have no testing capacity for the
older releases right now.

Cheers,

Richard