[CRIU] Re: [PATCH] crtools: Make fdset operations robust against open() errors

[Pavel Emelyanov]

On 01/31/2012 06:29 PM, Cyrill Gorcunov wrote:
> There are two cases for cr_fdset_open
> 
>  - It might be called with already allocated
>    memory so we should reuse it.
> 
>  - It might be called with NULL pointing out
>    that caller expects us to allocate memory.
> 
> If an open() error happens somewhere inside cr_fdset_open
> it requires two error paths
> 
>  - Just close all files opened but don't free memory
>    if it was not allocated by us
> 
>  - Close all files opened *and* free memory allocated
>    by us.
> 
> In any case we should close all files opened so close_cr_fdset()
> helper is splitted into two parts.
> 
> Also the caller should be ready for such semantics as well and
> do not re-assign pointers obtained but simply test for NULL
> on results.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>

Ack

> ---
> 
> This one should do the trick I think.
> 
>  cr-dump.c |    8 ++---
>  crtools.c |   85 +++++++++++++++++++++++++++++++++++++-----------------------
>  2 files changed, 55 insertions(+), 38 deletions(-)
> 
> diff --git a/cr-dump.c b/cr-dump.c
> index a4b938f..8a76987 100644
> --- a/cr-dump.c
> +++ b/cr-dump.c
> @@ -1214,8 +1214,8 @@ static int dump_one_task(struct pstree_item *item, struct cr_fdset *cr_fdset)
>  		goto err;
>  	};
>  
> -	cr_fdset = cr_fdset_open(item->pid, CR_FD_DESC_TASK, cr_fdset);
> -	if (!cr_fdset)
> +	ret = -1;
> +	if (!cr_fdset_open(item->pid, CR_FD_DESC_TASK, cr_fdset))
>  		goto err;
>  
>  	ret = collect_mappings(pid, pid_dir, &vma_area_list);
> @@ -1350,9 +1350,7 @@ int cr_dump_tasks(pid_t pid, struct cr_options *opts)
>  			goto err;
>  
>  		if (item->pid == pid) {
> -			cr_fdset = cr_fdset_open(item->pid,
> -					CR_FD_DESC_USE(CR_FD_PSTREE), cr_fdset);
> -			if (!cr_fdset)
> +			if (!cr_fdset_open(item->pid, CR_FD_DESC_USE(CR_FD_PSTREE), cr_fdset))
>  				goto err;
>  			if (dump_pstree(pid, &pstree_list, cr_fdset))
>  				goto err;
> diff --git a/crtools.c b/crtools.c
> index 560c609..faabf18 100644
> --- a/crtools.c
> +++ b/crtools.c
> @@ -123,22 +123,55 @@ static struct cr_fdset *alloc_cr_fdset(void)
>  	return cr_fdset;
>  }
>  
> +void __close_cr_fdset(struct cr_fdset *cr_fdset)
> +{
> +	unsigned int i;
> +
> +	if (!cr_fdset)
> +		return;
> +
> +	for (i = 0; i < CR_FD_MAX; i++) {
> +		if (cr_fdset->fds[i] == -1)
> +			continue;
> +		pr_debug("Closed %d/%d\n", i, cr_fdset->fds[i]);
> +		close_safe(&cr_fdset->fds[i]);
> +		cr_fdset->fds[i] = -1;
> +	}
> +}
> +
> +void close_cr_fdset(struct cr_fdset **cr_fdset)
> +{
> +	if (!cr_fdset || !*cr_fdset)
> +		return;
> +
> +	__close_cr_fdset(*cr_fdset);
> +
> +	xfree(*cr_fdset);
> +	*cr_fdset = NULL;
> +}
> +
>  struct cr_fdset *cr_fdset_open(int pid, unsigned long use_mask, struct cr_fdset *cr_fdset)
>  {
> +	struct cr_fdset *fdset;
>  	unsigned int i;
>  	int ret = -1;
>  	char path[PATH_MAX];
>  
> -	if (cr_fdset == NULL) {
> -		cr_fdset = alloc_cr_fdset();
> -		if (!cr_fdset)
> +	/*
> +	 * We either reuse existing fdset or create new one.
> +	 */
> +	if (!cr_fdset) {
> +		fdset = alloc_cr_fdset();
> +		if (!fdset)
>  			goto err;
> -	}
> +	} else
> +		fdset = cr_fdset;
>  
>  	for (i = 0; i < CR_FD_MAX; i++) {
>  		if (!(use_mask & CR_FD_DESC_USE(i)))
>  			continue;
> -		if (cr_fdset->fds[i] != -1)
> +
> +		if (fdset->fds[i] != -1)
>  			continue;
>  
>  		ret = get_image_path(path, sizeof(path),
> @@ -157,15 +190,21 @@ struct cr_fdset *cr_fdset_open(int pid, unsigned long use_mask, struct cr_fdset
>  			pr_perror("Unable to open %s", path);
>  			goto err;
>  		}
> +		fdset->fds[i] = ret;
>  
>  		pr_debug("Opened %s with %d\n", path, ret);
>  		if (write_img(ret, &fdset_template[i].magic))
>  			goto err;
> -
> -		cr_fdset->fds[i] = ret;
>  	}
> +
> +	return fdset;
> +
>  err:
> -	return cr_fdset;
> +	if (fdset != cr_fdset)
> +		__close_cr_fdset(fdset);
> +	else
> +		close_cr_fdset(&fdset);
> +	return NULL;
>  }
>  
>  struct cr_fdset *prep_cr_fdset_for_restore(int pid, unsigned long use_mask)
> @@ -194,43 +233,23 @@ struct cr_fdset *prep_cr_fdset_for_restore(int pid, unsigned long use_mask)
>  			pr_perror("Unable to open %s", path);
>  			goto err;
>  		}
> +		cr_fdset->fds[i] = ret;
>  
>  		pr_debug("Opened %s with %d\n", path, ret);
>  		if (read_img(ret, &magic) < 0)
>  			goto err;
>  		if (magic != fdset_template[i].magic) {
> -			close(ret);
>  			pr_err("Magic doesn't match for %s\n", path);
>  			goto err;
>  		}
>  
> -		cr_fdset->fds[i] = ret;
>  	}
> -err:
> -	return cr_fdset;
> -}
> -
> -void close_cr_fdset(struct cr_fdset **cr_fdset)
> -{
> -	struct cr_fdset *fdset;
> -	unsigned int i;
> -
> -	if (!cr_fdset || !*cr_fdset)
> -		return;
> -
> -	fdset = *cr_fdset;
> -
> -	for (i = 0; i < CR_FD_MAX; i++) {
> -		if (fdset->fds[i] == -1)
> -			continue;
>  
> -		pr_debug("Closed %d/%d\n", i, fdset->fds[i]);
> -		close(fdset->fds[i]);
> -		fdset->fds[i] = -1;
> -	}
> +	return cr_fdset;
>  
> -	xfree(fdset);
> -	*cr_fdset = NULL;
> +err:
> +	close_cr_fdset(&cr_fdset);
> +	return NULL;
>  }
>  
>  int main(int argc, char *argv[])