[CRIU] [PATCH] Resolve file descriptor clashes with inherit fds

[Andrew Vagin]

On Tue, Nov 25, 2014 at 02:54:05PM -0800, Saied Kazemi wrote:
> Since with --inherit-fd option criu's caller can set any fd to be
> inheritied, there is a chance of fd clash during restore.  Resolve such
> cases by moving the inherit fd to a different descriptor.

It should work. Thanks.

One comment about coding style is inline.

> 
> Signed-off-by: Saied Kazemi <saied at google.com>
> ---
>  files-reg.c     |  1 -
>  files.c         | 25 ++++++++++++++++++++++---
>  include/files.h |  3 ++-
>  util.c          |  3 +++
>  4 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/files-reg.c b/files-reg.c
> index bfd1896..2d2328c 100644
> --- a/files-reg.c
> +++ b/files-reg.c
> @@ -963,7 +963,6 @@ int open_path(struct file_desc *d,
>  
>  	mntns_root = mntns_get_root_by_mnt_id(rfi->rfe->mnt_id);
>  
> -pr_debug(">>> open_path(): rfi->path=(%s)\n", rfi->path);
>  	tmp = inherit_fd_lookup_id(rfi->path, true);
>  	if (tmp >= 0) {
>  		pr_info("File %s will be restored from fd %d\n", rfi->path, tmp);
> diff --git a/files.c b/files.c
> index 7bace3a..a074be5 100644
> --- a/files.c
> +++ b/files.c
> @@ -1346,16 +1346,35 @@ int inherit_fd_lookup_id(char *id, bool need_fd)
>  /*
>   * Look up the inherit fd list by a file descriptor.
>   */
> -bool inherit_fd_lookup_fd(int fd)
> +struct inherit_fd *inherit_fd_lookup_fd(int fd)
>  {
>  	struct inherit_fd *inh;
>  
>  	list_for_each_entry(inh, &opts.inherit_fds, l) {
>  		if (inh->inh_fd == fd) {
>  			pr_info("File descriptor %d in inherit fd list\n", fd);
> -			return true;
> +			return inh;
>  		}
>  	}
>  
> -	return false;
> +	return NULL;
> +}
> +
> +/*
> + * If the specified fd clashes with an inherit fd,
> + * move the inherit fd.
> + */
> +int inherit_fd_resolve_clash(int fd)
> +{
> +	struct inherit_fd *inh;
> +
> +	if ((inh = inherit_fd_lookup_fd(fd)) == NULL)

Could you not assign variables in if conditional statements? IMHO: It's hard
for reading and it may be dangerous.

> +		return 0;
> +
> +	if ((inh->inh_fd = dup(fd)) == -1 || close(fd) == -1) {
> +		pr_perror("Can't resolve inherit fd clash on %d", fd);
> +		return -1;
> +	}
> +	pr_debug("Resolved clash on inherit fd %d -> %d\n", fd, inh->inh_fd);
> +	return 0;
>  }
> diff --git a/include/files.h b/include/files.h
> index e4e304f..aa0e502 100644
> --- a/include/files.h
> +++ b/include/files.h
> @@ -174,6 +174,7 @@ extern int inherit_fd_add(char *optarg);
>  extern void inherit_fd_log(void);
>  extern int inherit_fd_validate_fds(void);
>  extern int inherit_fd_lookup_id(char *id, bool need_fd);
> -extern bool inherit_fd_lookup_fd(int fd);
> +extern struct inherit_fd *inherit_fd_lookup_fd(int fd);
> +extern int inherit_fd_resolve_clash(int fd);
>  
>  #endif /* __CR_FILES_H__ */
> diff --git a/util.c b/util.c
> index 3b468f2..4d2ef17 100644
> --- a/util.c
> +++ b/util.c
> @@ -118,6 +118,9 @@ int reopen_fd_as_safe(char *file, int line, int new_fd, int old_fd, bool allow_r
>  	int tmp;
>  
>  	if (old_fd != new_fd) {
> +		/* make sure we won't clash with an inherit fd */
> +		if (inherit_fd_resolve_clash(new_fd) < 0)
> +			return -1;
>  
>  		if (!allow_reuse_fd) {
>  			if (fcntl(new_fd, F_GETFD) != -1 || errno != EBADF) {
> -- 
> 2.2.0.rc0.207.ga3a616c
>