criu: fix segfault in pre-dump

Submitted by Adrian Reber on Sept. 11, 2018, 7:51 p.m.

Details

Message ID 20180911195115.25864-1-adrian@lisas.de
State New
Series "criu: fix segfault in pre-dump"
Headers show

Commit Message

Adrian Reber Sept. 11, 2018, 7:51 p.m.
From: Adrian Reber <areber@redhat.com>

By accident I found a segfault using pre-dump in combination with the
page-server. Doing the following I was able to trigger it:

 * criu page-server -D /tmp/1
 * criu pre-dump -t PID -D /tmp/3 --track-mem
 * criu page-server -D /tmp/4 --prev-images-dir ../1
 * criu pre-dump -t PID -D /tmp/3 --track-mem
 --> segfault

...
(00.010090) Warn  (criu/image.c:134): Failed to open parent directory
...
(00.012984) Error (criu/mem.c:318): Pid-reuse detection failed: no parent inventory, check warnings in get_parent_stats
...
(00.013037) Error (criu/mem.c:544): Can't dump page with parasite
...
(00.013955) Pre-dumping tasks' memory
(00.013966) 	Pre-dumping 8793
(00.014380) Transferring pages:
Segmentation fault (core dumped)

Looking in cr-dump.c at cr_pre_dump_finish(int ret) the function gets
the return code of the previous operations in 'ret' but it is
immediately overwritten and never used.

In older CRIU versions it used to be:

	if (ret < 0)
		goto err;

but that is gone now. So this reintroduces the check for the int
parameter given to cr_pre_dump_finish() by the function caller.

As the commands used to trigged the segfault do not make much sense the
result is still not usable and the same 'Warn' and 'Error' messages are
printed, but the segfault is gone.

Signed-off-by: Adrian Reber <areber@redhat.com>
---
 criu/cr-dump.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/criu/cr-dump.c b/criu/cr-dump.c
index ec5e1e45b..df31de1a9 100644
--- a/criu/cr-dump.c
+++ b/criu/cr-dump.c
@@ -1474,10 +1474,11 @@  static int setup_alarm_handler()
 	return 0;
 }
 
-static int cr_pre_dump_finish(int ret)
+static int cr_pre_dump_finish(int status)
 {
 	InventoryEntry he = INVENTORY_ENTRY__INIT;
 	struct pstree_item *item;
+	int ret;
 
 	/*
 	 * Restore registers for tasks only. The threads have not been
@@ -1495,6 +1496,9 @@  static int cr_pre_dump_finish(int ret)
 
 	timing_stop(TIME_FROZEN);
 
+	if (status < 0)
+		goto err;
+
 	pr_info("Pre-dumping tasks' memory\n");
 	for_each_pstree_item(item) {
 		struct parasite_ctl *ctl = dmpi(item)->parasite_ctl;

Comments

Andrey Vagin Sept. 11, 2018, 11:16 p.m.
On Tue, Sep 11, 2018 at 09:51:15PM +0200, Adrian Reber wrote:
> From: Adrian Reber <areber@redhat.com>
> 
> By accident I found a segfault using pre-dump in combination with the
> page-server. Doing the following I was able to trigger it:
> 
>  * criu page-server -D /tmp/1
>  * criu pre-dump -t PID -D /tmp/3 --track-mem
>  * criu page-server -D /tmp/4 --prev-images-dir ../1
>  * criu pre-dump -t PID -D /tmp/3 --track-mem
>  --> segfault
> 
> ...
> (00.010090) Warn  (criu/image.c:134): Failed to open parent directory
> ...
> (00.012984) Error (criu/mem.c:318): Pid-reuse detection failed: no parent inventory, check warnings in get_parent_stats
> ...
> (00.013037) Error (criu/mem.c:544): Can't dump page with parasite
> ...
> (00.013955) Pre-dumping tasks' memory
> (00.013966) 	Pre-dumping 8793
> (00.014380) Transferring pages:
> Segmentation fault (core dumped)
> 
> Looking in cr-dump.c at cr_pre_dump_finish(int ret) the function gets
> the return code of the previous operations in 'ret' but it is
> immediately overwritten and never used.
> 
> In older CRIU versions it used to be:
> 
> 	if (ret < 0)
> 		goto err;
> 
> but that is gone now. So this reintroduces the check for the int
> parameter given to cr_pre_dump_finish() by the function caller.
> 
> As the commands used to trigged the segfault do not make much sense the
> result is still not usable and the same 'Warn' and 'Error' messages are
> printed, but the segfault is gone.
> 
> Signed-off-by: Adrian Reber <areber@redhat.com>
> ---
>  criu/cr-dump.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/cr-dump.c b/criu/cr-dump.c
> index ec5e1e45b..df31de1a9 100644
> --- a/criu/cr-dump.c
> +++ b/criu/cr-dump.c
> @@ -1474,10 +1474,11 @@ static int setup_alarm_handler()
>  	return 0;
>  }
>  
> -static int cr_pre_dump_finish(int ret)
> +static int cr_pre_dump_finish(int status)
>  {
>  	InventoryEntry he = INVENTORY_ENTRY__INIT;
>  	struct pstree_item *item;
> +	int ret;
>  
>  	/*
>  	 * Restore registers for tasks only. The threads have not been
> @@ -1495,6 +1496,9 @@ static int cr_pre_dump_finish(int ret)
>  
>  	timing_stop(TIME_FROZEN);
>  
> +	if (status < 0)

I think we need to set ret in this case

		ret = status;
> +		goto err;
> +
>  	pr_info("Pre-dumping tasks' memory\n");
>  	for_each_pstree_item(item) {
>  		struct parasite_ctl *ctl = dmpi(item)->parasite_ctl;
> -- 
> 2.17.1
>