Re: [PATCH v3 06/10] run-command: add clean_on_exit_handler

From: Johannes Sixt <j6t@kdbg.org>
Date: Jul 30, 2016, 09:50 UTC
Message-ID: <ef6c6152-a720-6bd5-22bb-6ebf375ca919@kdbg.org>
In-Reply-To: <20160729233801.82844-7-larsxschneider@gmail.com>

Am 30.07.2016 um 01:37 schrieb larsxschneider@gmail.com:

Show 27 quoted lines

> Some commands might need to perform cleanup tasks on exit. Let's give
> them an interface for doing this.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  run-command.c | 12 ++++++++----
>  run-command.h |  1 +
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 33bc63a..197b534 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)
>
>  struct child_to_clean {
>  	pid_t pid;
> +	void (*clean_on_exit_handler)(pid_t);
>  	struct child_to_clean *next;
>  };
>  static struct child_to_clean *children_to_clean;
> @@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal)
>  {
>  	while (children_to_clean) {
>  		struct child_to_clean *p = children_to_clean;
> +		if (p->clean_on_exit_handler)
> +			p->clean_on_exit_handler(p->pid);

This summons demons. cleanup_children() is invoked from a signal handler. In this case, it can call only async-signal-safe functions. It does not look like the handler that you are going to install later will take note of this caveat!

>  		children_to_clean = p->next;
>  		kill(p->pid, sig);
>  		if (!in_signal)

The condition that we see here in the context protects free(p) (which is not async-signal-safe). Perhaps the invocation of the new callback should be skipped in the same manner when this is called from a signal handler? 507d7804 (pager: don't use unsafe functions in signal handlers) may be worth a look.

-- Hannes