[PATCH 1/2] compositor: fix a memory leak of struct process_info

[Pekka Paalanen]

On Sat, 21 Feb 2015 01:07:37 +0900
Ryo Munakata <ryomnktml at gmail.com> wrote:

> Cleanup functions of weston clients are never called
> after wl_display_run(), so that some of process_info of clients will not be freed.
> 
> Signed-off-by: Ryo Munakata <ryomnktml at gmail.com>
> ---
>  src/compositor.c | 24 +++++++++++++++++++++---
>  src/compositor.h |  1 +
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/src/compositor.c b/src/compositor.c
> index 7085053..e4b5f5a 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -303,8 +303,8 @@ child_client_exec(int sockfd, const char *path)
>  			path);
>  }
>  
> -WL_EXPORT struct wl_client *
> -weston_client_launch(struct weston_compositor *compositor,
> +static struct wl_client *
> +weston_client_launch_impl(struct weston_compositor *compositor,
>  		     struct weston_process *proc,
>  		     const char *path,
>  		     weston_process_cleanup_func_t cleanup)
> @@ -354,6 +354,16 @@ weston_client_launch(struct weston_compositor *compositor,
>  	return client;
>  }
>  
> +WL_EXPORT struct wl_client *
> +weston_client_launch(struct weston_compositor *compositor,
> +		     struct weston_process *proc,
> +		     const char *path,
> +		     weston_process_cleanup_func_t cleanup)
> +{
> +	proc->managed = false;
> +	return weston_client_launch_impl(compositor, proc, path, cleanup);
> +}
> +
>  struct process_info {
>  	struct weston_process proc;
>  	char *path;
> @@ -398,7 +408,8 @@ weston_client_start(struct weston_compositor *compositor, const char *path)
>  	if (!pinfo->path)
>  		goto out_free;
>  
> -	client = weston_client_launch(compositor, &pinfo->proc, path,
> +	pinfo->proc.managed = true;
> +	client = weston_client_launch_impl(compositor, &pinfo->proc, path,
>  				      process_handle_sigchld);
>  	if (!client)
>  		goto out_str;
> @@ -4841,6 +4852,7 @@ int main(int argc, char *argv[])
>  	struct wl_client *primary_client;
>  	struct wl_listener primary_client_destroyed;
>  	struct weston_seat *seat;
> +	struct weston_process *p, *p_next;
>  
>  	const struct weston_option core_options[] = {
>  		{ WESTON_OPTION_STRING, "backend", 'B', &backend },
> @@ -5012,6 +5024,12 @@ out:
>  	/* prevent further rendering while shutting down */
>  	ec->state = WESTON_COMPOSITOR_OFFSCREEN;
>  
> +	wl_list_for_each_safe(p, p_next, &child_process_list, link) {
> +		wl_list_remove(&p->link);
> +		if (p->managed)
> +			p->cleanup(p, 0);
> +	}
> +

I would like this to be a function of its own instead of making main()
even longer. Choosing a descriptive function name will also implicitly
document what this does.

I also thought if we would like to actively kill the remaining
processes. After all, usually cleanup is called when the process is
already dead. I'm not sure.

Passing 0 as the exit status is misleading, because I do not think it
can say "the process is still running, but want to clean up anyway".
This might lead to spurious messages on exit.

We also want to avoid spurious respawn when calling cleanup.
text-backend.c uses the cleanup call to respawn, and we don't want that
to happen on exit.

>  	wl_signal_emit(&ec->destroy_signal, ec);
>  
>  	weston_compositor_xkb_destroy(ec);
> diff --git a/src/compositor.h b/src/compositor.h
> index 47b6036..f1b630f 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -1398,6 +1398,7 @@ struct weston_process {
>  	pid_t pid;
>  	weston_process_cleanup_func_t cleanup;
>  	struct wl_list link;
> +	bool managed;

I do not think we need this boolean variable. Rationale: if struct
weston_process is in the child_process_list, it has not been cleaned
up. If something calls the cleanup function directly, it better also
remove it from the list. Hence we don't need to track how the child was
added to the list.

If something breaks this rationale, I'd rather see that fixed.

>  };
>  
>  struct wl_client *

I have some doubts on this approach.

How about we require all modules to kill their child processes as
appropriate in the compositor destroy signal handler. This would apply
only to processes started with weston_client_launch(), because
weston_client_start() doesn't expose struct process_info, but that's
all ok.

So, first send the compositor destroy signal, which shuts down all
modules. It should also deactivate any respawn logic. Then, go through
the child_process_list for any remaining entries: kill, wait, and call
cleanup. That also gives us the proper exit status for the process.

But it does mean that weston's exit would hang if a child doesn't die.

Needs more thinking, I think... need to solve how launched processes
are intended to end, and then fit the cleaning up to that.


Thanks,
pq