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

[Qiang Huang]

On 2014/1/23 8:27, Serge Hallyn wrote:
> 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,

Sorry, I'll drop this.

> 
>> 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.

I already reasoned in the added comment, we need this because if not,
lxc_container won't be freed when daemon exits, and PID file won't
be unlinked either.

What about this:

>From 80f3862f9c4dbc8a05e79e50c50e79e30ffebc25 Mon Sep 17 00:00:00 2001
From: Qiang Huang <h.huangqiang at huawei.com>
Date: Thu, 23 Jan 2014 14:25:31 +0800
Subject: [PATCH] daemon: add lxc_container_put to free container when daemon exits

PID file in lxc_container is unlinked when lxc_container_free,
if we leak the container, the PID file also won't be removed
after container down.

Signed-off-by: Qiang Huang <h.huangqiang at huawei.com>
---
 src/lxc/lxccontainer.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 28de455..d76e386 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -669,9 +669,14 @@ reboot:
 		goto reboot;
 	}

-	if (daemonize)
+	if (daemonize) {
+		/* When daemon forked, he inherited father's
+		 * lxc_container, so here need a put to free
+		 * lxc_container.
+		 */
+		lxc_container_put(c);
 		exit (ret == 0 ? true : false);
-	else
+	} else
 		return (ret == 0 ? true : false);
 }

-- 
1.8.3

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