[lxc-devel] [PATCH 1/3] lxc-start: fix the container leak when daemonize

[Serge Hallyn]

Quoting Qiang Huang (h.huangqiang at huawei.com):
> When start container with daemon model, we'll have a new daemon
> process in lxcapi_start, whose c->numthreads is 2, inherited
> from his father, and his father's return and lxc_container_put
> won't affect son's numthreads. So when daemon stops, he only
> did on lxc_container_put, the lxc_container will leak.
> 
> Actually, lxc_container_get only works for mulit-threads cases,
> here we did fork, and don't need to hold container count to
> protect anything, so just remove it.
> 
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>

No,

> Signed-off-by: Qiang Huang <h.huangqiang at huawei.com>
> ---
>  src/lxc/lxccontainer.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index 368cb46..d020918 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -589,15 +589,11 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
>  	* while container is running...
>  	*/
>  	if (daemonize) {
> -		if (!lxc_container_get(c))
> -			return false;
>  		lxc_monitord_spawn(c->config_path);
>  
>  		pid_t pid = fork();
> -		if (pid < 0) {
> -			lxc_container_put(c);
> +		if (pid < 0)
>  			return false;
> -		}
>  		if (pid != 0)
>  			return wait_on_daemonized_start(c, pid);
>  
> @@ -639,6 +635,10 @@ reboot:
>  	}
>  
>  	if (daemonize) {
> +		/* When daemon was forked, it inherited parent's
> +		 * lxc_container, so here need a put to free
> +		 * lxc_container.
> +		 */
>  		lxc_container_put(c);

Why did you change this in my patch?  Since we have removed the
lxc_container_get() at the top of this patch, the matching put
should also be removed.

I'm going to go ahead an apply my original patch 
(https://lists.linuxcontainers.org/pipermail/lxc-devel/2014-January/007343.html)
If you think there is an error in that logic, it'll be easier
to reason about as a separate patch on top of mine.

>  		exit (ret == 0 ? true : false);
>  	} else {
> -- 
> 1.8.3
> 
>