On Wed, Jun 04, 2025 at 11:27:47PM +0800, Valentin Lab wrote: > On 2025-06-01 13:35:07 +0800, Nadav Tasher wrote: > > Since we're setting the signal handler to SIG_IGN, both SA_RESTART and > > SA_NOCLDWAIT are meaningless - they only matter when setting the handler > > to SIG_DFL or to a custom signal handler. > > > > This could be simplified to signal(SIGCHLD, SIG_IGN). > > > > My personal opinion is that SIGCHLD should be used until you actually > > implement error handling, as it is more efficient. > > When you do implement error handling, there would be no other option > > then to use waitpid, and that would be acceptable. > > > > If you do decide to use SIGCHLD, I'd appreciate it if you added a > > comment next to the existing waitpid, just to make sure nobody tries > > to pass wstatus to it. > > Sure ! Here's what you've suggested. Thx for your time. > > > From 5b31a17e0feb23b8fa89df521973f422604e84d0 Mon Sep 17 00:00:00 2001 > From: Valentin Lab <[email protected]> > Date: Mon, 2 Jun 2025 16:56:26 +0800 > Subject: [PATCH] crond: reap orphaned grandchildren to prevent zombie > buildup > > If a cron job launches a background task, e.g. `sh -c "sleep 5 &"`, > the shell exits immediately and the `sleep` process is re-parented to > PID 1. When BusyBox `crond` itself happens to be PID 1 (common in a > minimal container), those orphans become direct children of `crond`. > Because `crond` only calls waitpid() for the PIDs it explicitly tracks, > these processes remain forever in Z state and the container slowly > fills with zombies. > > Setting the SIGCHLD disposition to SIG_IGN will ensure the kernel > reaps automatically all child processes. > > Size impact: +0 bytes on x86-64 (gcc 13.3.0 -Os, static) > > Signed-off-by: Valentin Lab <[email protected]> > --- > miscutils/crond.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/miscutils/crond.c b/miscutils/crond.c > index b3762d327..933816456 100644 > --- a/miscutils/crond.c > +++ b/miscutils/crond.c > @@ -989,6 +989,15 @@ static int check_completions(void) > if (line->cl_pid <= 0) > continue; > > + /* SIGCHLD disposition is set to SIG_IGN in > + * `crond_main()`, so the kernel reaps children > + * automatically and discards their exit status. If we > + * ever need the exit status, drop SIG_IGN and add a > + * generic reaper (e.g. `while (waitpid(-1, NULL, > WNOHANG) > + * > 0);`) at the end of this function to prevent > zombies > + * when crond happens to be PID 1 (all orphans are > + * re-parented to it). > + */ > r = waitpid(line->cl_pid, NULL, WNOHANG); > if (r < 0 || r == line->cl_pid) { > process_finished_job(file->cf_username, line); > @@ -1025,6 +1034,9 @@ int crond_main(int argc UNUSED_PARAM, char **argv) > unsigned sleep_time; > unsigned opts; > > + /* Reap all children process automatically. */ > + signal(SIGCHLD, SIG_IGN); > + > INIT_G(); > > opts = getopt32(argv, "^" > -- > 2.43.0
> From 5b31a17e0feb23b8fa89df521973f422604e84d0 Mon Sep 17 00:00:00 2001 > From: Valentin Lab <[email protected]> > Date: Mon, 2 Jun 2025 16:56:26 +0800 > Subject: [PATCH] crond: reap orphaned grandchildren to prevent zombie buildup > > If a cron job launches a background task, e.g. `sh -c "sleep 5 &"`, > the shell exits immediately and the `sleep` process is re-parented to > PID 1. When BusyBox `crond` itself happens to be PID 1 (common in a > minimal container), those orphans become direct children of `crond`. > Because `crond` only calls waitpid() for the PIDs it explicitly tracks, > these processes remain forever in Z state and the container slowly > fills with zombies. > > Setting the SIGCHLD disposition to SIG_IGN will ensure the kernel > reaps automatically all child processes. > > Size impact: +0 bytes on x86-64 (gcc 13.3.0 -Os, static) > > Signed-off-by: Valentin Lab <[email protected]> > --- > miscutils/crond.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/miscutils/crond.c b/miscutils/crond.c > index b3762d327..933816456 100644 > --- a/miscutils/crond.c > +++ b/miscutils/crond.c > @@ -989,6 +989,15 @@ static int check_completions(void) > if (line->cl_pid <= 0) > continue; > > + /* SIGCHLD disposition is set to SIG_IGN in > + * `crond_main()`, so the kernel reaps children > + * automatically and discards their exit status. If we > + * ever need the exit status, drop SIG_IGN and add a > + * generic reaper (e.g. `while (waitpid(-1, NULL, > WNOHANG) > + * > 0);`) at the end of this function to prevent > zombies > + * when crond happens to be PID 1 (all orphans are > + * re-parented to it). > + */ > r = waitpid(line->cl_pid, NULL, WNOHANG); > if (r < 0 || r == line->cl_pid) { > process_finished_job(file->cf_username, line); > @@ -1025,6 +1034,9 @@ int crond_main(int argc UNUSED_PARAM, char **argv) > unsigned sleep_time; > unsigned opts; > > + /* Reap all children process automatically. */ > + signal(SIGCHLD, SIG_IGN); > + > INIT_G(); > > opts = getopt32(argv, "^" > -- > 2.43.0 > Looks good to me. This change is non-intrusive and makes sense, regardless of the use-case for crond (running as PID 1 or as a service). Nadav _______________________________________________ busybox mailing list [email protected] https://lists.busybox.net/mailman/listinfo/busybox
