[v2] util: disable log service in cr_system_userns if error fd was -1

Submitted by Stanislav Kinsburskiy on April 22, 2016, 4:44 p.m.

Details

Message ID 20160422164422.171123.63274.stgit@skinsbursky-vz7.qa.sw.ru
State Rejected, archived
Series "util: disable log service in cr_system_userns if error fd was -1"
Headers show

Patch hide | download patch | download mbox

diff --git a/criu/include/log.h b/criu/include/log.h
index fe53a7c..7ecc3fe 100644
--- a/criu/include/log.h
+++ b/criu/include/log.h
@@ -6,6 +6,7 @@ 
 #include "criu-log.h"
 
 extern int log_init(const char *output);
+extern void log_disable(void);
 extern void log_fini(void);
 extern int log_init_by_pid(void);
 extern void log_closedir(void);
diff --git a/criu/log.c b/criu/log.c
index 6c5d807..1992e3a 100644
--- a/criu/log.c
+++ b/criu/log.c
@@ -134,6 +134,11 @@  int log_init_by_pid(void)
 	return log_init(path);
 }
 
+void log_disable(void)
+{
+	disable_service_fd(LOG_FD_OFF);
+}
+
 void log_fini(void)
 {
 	close_service_fd(LOG_FD_OFF);
diff --git a/criu/util.c b/criu/util.c
index dae6031..216efb6 100644
--- a/criu/util.c
+++ b/criu/util.c
@@ -451,6 +451,17 @@  int criu_get_image_dir(void)
 	return get_service_fd(IMG_FD_OFF);
 }
 
+int disable_service_fd(enum sfd_type type)
+{
+	int fd;
+
+	fd = get_service_fd(type);
+	if (fd < 0)
+		return 0;
+
+	clear_bit(type, sfd_map);
+	return 0;
+}
 int close_service_fd(enum sfd_type type)
 {
 	int fd;
@@ -641,9 +652,6 @@  int cr_system_userns(int in, int out, int err, char *cmd,
 		    move_img_fd(&err, STDIN_FILENO))
 			goto out_chld;
 
-		if (stop_log_fd)
-			log_fini();
-
 		if (in < 0) {
 			close(STDIN_FILENO);
 		} else {
@@ -660,6 +668,9 @@  int cr_system_userns(int in, int out, int err, char *cmd,
 		if (reopen_fd_as_nocheck(STDERR_FILENO, err))
 			goto out_chld;
 
+		if (stop_log_fd)
+			log_disable();
+
 		execvp(cmd, argv);
 
 		pr_perror("exec failed");

Comments

Andrey Vagin April 22, 2016, 11:06 p.m.
On Fri, Apr 22, 2016 at 07:44:24PM +0300, Stanislav Kinsburskiy wrote:
> In case of error fd was negative, log_fd is _moved_ as STDERR for the child.
> After move, log_fd is closed, and thus log service is broken and any error
> prints will disappear.

The bug is that we move log_fd into STDERR. If out is negative,
we will move logfd into STDOUT and your patch doesn't handle this case.

I think we should not move logfd anywhere. Pls, look at my patch.

> This patch disables log service, thus redirecting error output to STDERR
> (which is log fd in this case).
> 
> Calling of log fini is:
> 1) to early: reopen_fd_as_nocheck will fail
> 2) to much: close_safe will print error message, because log fd was already
> closed
> 
> v2:
> log_disable helper introduced
> 
> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> ---
>  criu/include/log.h |    1 +
>  criu/log.c         |    5 +++++
>  criu/util.c        |   17 ++++++++++++++---
>  3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/criu/include/log.h b/criu/include/log.h
> index fe53a7c..7ecc3fe 100644
> --- a/criu/include/log.h
> +++ b/criu/include/log.h
> @@ -6,6 +6,7 @@
>  #include "criu-log.h"
>  
>  extern int log_init(const char *output);
> +extern void log_disable(void);
>  extern void log_fini(void);
>  extern int log_init_by_pid(void);
>  extern void log_closedir(void);
> diff --git a/criu/log.c b/criu/log.c
> index 6c5d807..1992e3a 100644
> --- a/criu/log.c
> +++ b/criu/log.c
> @@ -134,6 +134,11 @@ int log_init_by_pid(void)
>  	return log_init(path);
>  }
>  
> +void log_disable(void)
> +{
> +	disable_service_fd(LOG_FD_OFF);
> +}
> +
>  void log_fini(void)
>  {
>  	close_service_fd(LOG_FD_OFF);
> diff --git a/criu/util.c b/criu/util.c
> index dae6031..216efb6 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -451,6 +451,17 @@ int criu_get_image_dir(void)
>  	return get_service_fd(IMG_FD_OFF);
>  }
>  
> +int disable_service_fd(enum sfd_type type)
> +{
> +	int fd;
> +
> +	fd = get_service_fd(type);
> +	if (fd < 0)
> +		return 0;
> +
> +	clear_bit(type, sfd_map);
> +	return 0;
> +}
>  int close_service_fd(enum sfd_type type)
>  {
>  	int fd;
> @@ -641,9 +652,6 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>  		    move_img_fd(&err, STDIN_FILENO))
>  			goto out_chld;
>  
> -		if (stop_log_fd)
> -			log_fini();
> -
>  		if (in < 0) {
>  			close(STDIN_FILENO);
>  		} else {
> @@ -660,6 +668,9 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>  		if (reopen_fd_as_nocheck(STDERR_FILENO, err))
>  			goto out_chld;
>  
> +		if (stop_log_fd)
> +			log_disable();
> +
>  		execvp(cmd, argv);
>  
>  		pr_perror("exec failed");
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Stanislav Kinsburskiy April 24, 2016, 6:41 p.m.
23.04.2016 01:06, Andrew Vagin пишет:
> On Fri, Apr 22, 2016 at 07:44:24PM +0300, Stanislav Kinsburskiy wrote:
>> In case of error fd was negative, log_fd is _moved_ as STDERR for the child.
>> After move, log_fd is closed, and thus log service is broken and any error
>> prints will disappear.
> The bug is that we move log_fd into STDERR. If out is negative,
> we will move logfd into STDOUT and your patch doesn't handle this case.
>
> I think we should not move logfd anywhere. Pls, look at my patch.

I'm ok with your patch,

>> This patch disables log service, thus redirecting error output to STDERR
>> (which is log fd in this case).
>>
>> Calling of log fini is:
>> 1) to early: reopen_fd_as_nocheck will fail
>> 2) to much: close_safe will print error message, because log fd was already
>> closed
>>
>> v2:
>> log_disable helper introduced
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>> ---
>>   criu/include/log.h |    1 +
>>   criu/log.c         |    5 +++++
>>   criu/util.c        |   17 ++++++++++++++---
>>   3 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/criu/include/log.h b/criu/include/log.h
>> index fe53a7c..7ecc3fe 100644
>> --- a/criu/include/log.h
>> +++ b/criu/include/log.h
>> @@ -6,6 +6,7 @@
>>   #include "criu-log.h"
>>   
>>   extern int log_init(const char *output);
>> +extern void log_disable(void);
>>   extern void log_fini(void);
>>   extern int log_init_by_pid(void);
>>   extern void log_closedir(void);
>> diff --git a/criu/log.c b/criu/log.c
>> index 6c5d807..1992e3a 100644
>> --- a/criu/log.c
>> +++ b/criu/log.c
>> @@ -134,6 +134,11 @@ int log_init_by_pid(void)
>>   	return log_init(path);
>>   }
>>   
>> +void log_disable(void)
>> +{
>> +	disable_service_fd(LOG_FD_OFF);
>> +}
>> +
>>   void log_fini(void)
>>   {
>>   	close_service_fd(LOG_FD_OFF);
>> diff --git a/criu/util.c b/criu/util.c
>> index dae6031..216efb6 100644
>> --- a/criu/util.c
>> +++ b/criu/util.c
>> @@ -451,6 +451,17 @@ int criu_get_image_dir(void)
>>   	return get_service_fd(IMG_FD_OFF);
>>   }
>>   
>> +int disable_service_fd(enum sfd_type type)
>> +{
>> +	int fd;
>> +
>> +	fd = get_service_fd(type);
>> +	if (fd < 0)
>> +		return 0;
>> +
>> +	clear_bit(type, sfd_map);
>> +	return 0;
>> +}
>>   int close_service_fd(enum sfd_type type)
>>   {
>>   	int fd;
>> @@ -641,9 +652,6 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>>   		    move_img_fd(&err, STDIN_FILENO))
>>   			goto out_chld;
>>   
>> -		if (stop_log_fd)
>> -			log_fini();
>> -
>>   		if (in < 0) {
>>   			close(STDIN_FILENO);
>>   		} else {
>> @@ -660,6 +668,9 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>>   		if (reopen_fd_as_nocheck(STDERR_FILENO, err))
>>   			goto out_chld;
>>   
>> +		if (stop_log_fd)
>> +			log_disable();
>> +
>>   		execvp(cmd, argv);
>>   
>>   		pr_perror("exec failed");
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu