[LEDE-DEV,libubox,1/2] uloop: Fix race condition in SIGCHLD handling

Message ID 20170912111250.31576-2-sojkam1@fel.cvut.cz
State Accepted
Delegated to: Yousong Zhou
Headers show
Series
  • [LEDE-DEV,libubox,1/2] uloop: Fix race condition in SIGCHLD handling
Related show

Commit Message

Michal Sojka Sept. 12, 2017, 11:12 a.m.
When uloop_process_add() is called outside of uloop_run(), i.e. not
from a callback (which is the case of at least utrace and ujail),
child events can be missed. The reason is that when SIGCHILD handler
is installed in uloop_run(), after the uloop_process_add() is called,
then an initial signal could be missed.

Commit 4e3a47a ("uloop: use a waker for notifying sigchld and loop
cancel events", 2016-06-09) solved a similar problem and introduced
uloop_init() but forgot to move a call to uloop_setup_signals() there.
This is what this commit does.

Now, uloop_process_add() can be called any time after uloop_init()
without missing any event.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 uloop.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Yousong Zhou Sept. 14, 2017, 1:53 a.m. | #1
On 12 September 2017 at 19:12, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> When uloop_process_add() is called outside of uloop_run(), i.e. not
> from a callback (which is the case of at least utrace and ujail),
> child events can be missed. The reason is that when SIGCHILD handler
> is installed in uloop_run(), after the uloop_process_add() is called,
> then an initial signal could be missed.
>
> Commit 4e3a47a ("uloop: use a waker for notifying sigchld and loop
> cancel events", 2016-06-09) solved a similar problem and introduced
> uloop_init() but forgot to move a call to uloop_setup_signals() there.
> This is what this commit does.
>
> Now, uloop_process_add() can be called any time after uloop_init()
> without missing any event.
>
> Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>

Acked-by: Yousong Zhou <yszhou4tech@gmail.com>

Thanks.
                yousong

> ---
>  uloop.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/uloop.c b/uloop.c
> index d2f41bb..3813e18 100644
> --- a/uloop.c
> +++ b/uloop.c
> @@ -116,6 +116,8 @@ static int waker_init(void)
>         return 0;
>  }
>
> +static void uloop_setup_signals(bool add);
> +
>  int uloop_init(void)
>  {
>         if (uloop_init_pollfd() < 0)
> @@ -126,6 +128,8 @@ int uloop_init(void)
>                 return -1;
>         }
>
> +       uloop_setup_signals(true);
> +
>         return 0;
>  }
>
> @@ -525,12 +529,7 @@ int uloop_run_timeout(int timeout)
>         int next_time = 0;
>         struct timeval tv;
>
> -       /*
> -        * Handlers are only updated for the first call to uloop_run() (and restored
> -        * when this call is done).
> -        */
> -       if (!uloop_run_depth++)
> -               uloop_setup_signals(true);
> +       uloop_run_depth++;
>
>         uloop_status = 0;
>         uloop_cancelled = false;
> @@ -553,14 +552,15 @@ int uloop_run_timeout(int timeout)
>                 uloop_run_events(next_time);
>         }
>
> -       if (!--uloop_run_depth)
> -               uloop_setup_signals(false);
> +       --uloop_run_depth;
>
>         return uloop_status;
>  }
>
>  void uloop_done(void)
>  {
> +       uloop_setup_signals(false);
> +
>         if (poll_fd >= 0) {
>                 close(poll_fd);
>                 poll_fd = -1;
> --
> 2.14.1
>
>
> _______________________________________________
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

Patch

diff --git a/uloop.c b/uloop.c
index d2f41bb..3813e18 100644
--- a/uloop.c
+++ b/uloop.c
@@ -116,6 +116,8 @@  static int waker_init(void)
 	return 0;
 }
 
+static void uloop_setup_signals(bool add);
+
 int uloop_init(void)
 {
 	if (uloop_init_pollfd() < 0)
@@ -126,6 +128,8 @@  int uloop_init(void)
 		return -1;
 	}
 
+	uloop_setup_signals(true);
+
 	return 0;
 }
 
@@ -525,12 +529,7 @@  int uloop_run_timeout(int timeout)
 	int next_time = 0;
 	struct timeval tv;
 
-	/*
-	 * Handlers are only updated for the first call to uloop_run() (and restored
-	 * when this call is done).
-	 */
-	if (!uloop_run_depth++)
-		uloop_setup_signals(true);
+	uloop_run_depth++;
 
 	uloop_status = 0;
 	uloop_cancelled = false;
@@ -553,14 +552,15 @@  int uloop_run_timeout(int timeout)
 		uloop_run_events(next_time);
 	}
 
-	if (!--uloop_run_depth)
-		uloop_setup_signals(false);
+	--uloop_run_depth;
 
 	return uloop_status;
 }
 
 void uloop_done(void)
 {
+	uloop_setup_signals(false);
+
 	if (poll_fd >= 0) {
 		close(poll_fd);
 		poll_fd = -1;