Re: [PATCH 03/15] mdadm/Grow: fix coverity issue RESOURCE_LEAK

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Mon, 15 Jul 2024 15:35:52 +0800
Xiao Ni <xni@redhat.com> wrote:

> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Grow.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 7ae967bda067..632be7db8d38 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -485,6 +485,7 @@ int Grow_addbitmap(char *devname, int fd, struct context
> *c, struct shape *s) int bitmap_fd;
>  		int d;
>  		int max_devs = st->max_devs;
> +		int err = 0;
>  
>  		/* try to load a superblock */
>  		for (d = 0; d < max_devs; d++) {
> @@ -525,13 +526,14 @@ int Grow_addbitmap(char *devname, int fd, struct
> context *c, struct shape *s) return 1;
>  		}
>  		if (ioctl(fd, SET_BITMAP_FILE, bitmap_fd) < 0) {
> -			int err = errno;
> +			err = errno;
>  			if (errno == EBUSY)
>  				pr_err("Cannot add bitmap while array is
> resyncing or reshaping etc.\n"); pr_err("Cannot set bitmap file for %s: %s\n",
>  				devname, strerror(err));
> -			return 1;
>  		}
> +		close(bitmap_fd);
> +		return err;

I don't think that we should return errno. I would say that mdadm should define
returned statues for functions, that is why I added mdadm_status_t.

Otherwise, same value may have two meanings. For example, in some cases we are
fine with particular error code from error might be misleading (it may match
allowed status).
This is not the case here, it is just an example.

I think that we should not return errno outside if not intended i.e. function is
projected to return errno in every case.

>  	}
>  
>  	return 0;
> @@ -3083,6 +3085,7 @@ static int reshape_array(char *container, int fd, char
> *devname, int done;
>  	struct mdinfo *sra = NULL;
>  	char buf[SYSFS_MAX_BUF_SIZE];
> +	bool located_backup = false;
>  
>  	/* when reshaping a RAID0, the component_size might be zero.
>  	 * So try to fix that up.
> @@ -3165,8 +3168,10 @@ static int reshape_array(char *container, int fd, char
> *devname, goto release;
>  		}
>  
> -		if (!backup_file)
> +		if (!backup_file) {
>  			backup_file = locate_backup(sra->sys_name);
> +			located_backup = true;
> +		}
>  
>  		goto started;
>  	}
> @@ -3612,15 +3617,15 @@ started:
>  			mdstat_wait(30 - (delayed-1) * 25);
>  	} while (delayed);
>  	mdstat_close();
> -	if (check_env("MDADM_GROW_VERIFY"))
> -		fd = open(devname, O_RDONLY | O_DIRECT);
> -	else
> -		fd = -1;
>  	mlockall(MCL_FUTURE);
>  
>  	if (signal_s(SIGTERM, catch_term) == SIG_ERR)
>  		goto release;
>  
> +	if (check_env("MDADM_GROW_VERIFY"))
> +		fd = open(devname, O_RDONLY | O_DIRECT);
> +	else
> +		fd = -1;

close_fd() is used unconditionally on fd few line earlier so it seems that else
path is not needed but this code is massive so please double check if I'm right.

We may call close_fd() on the closed resource and then it is not updated to -1,
assuming that close fails with EBADF.

Right way to fix it is to replace all close(fd) by close_fd(fd). It will give is
credibility that double close is not possible.


>  	if (st->ss->external) {
>  		/* metadata handler takes it from here */
>  		done = st->ss->manage_reshape(
> @@ -3632,6 +3637,8 @@ started:
>  			fd, sra, &reshape, st, blocks, fdlist, offsets,
>  			d - odisks, fdlist + odisks, offsets + odisks);
>  
> +	if (fd >= 0)
> +		close(fd);
>  	free(fdlist);
>  	free(offsets);
>  
> @@ -3701,6 +3708,8 @@ out:
>  	exit(0);
>  
>  release:
> +	if (located_backup)
> +		free(backup_file);

>  	free(fdlist);
>  	free(offsets);
>  	if (orig_level != UnSet && sra) {
> @@ -3839,6 +3848,7 @@ int reshape_container(char *container, char *devname,
>  			pr_err("Unable to initialize sysfs for %s\n",
>  			       mdstat->devnm);
>  			rv = 1;
> +			close(fd);
>  			break;
>  		}
>  
> @@ -4717,6 +4727,7 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist, unsigned long long *offsets;
>  	unsigned long long  nstripe, ostripe;
>  	int ndata, odata;
> +	int fd, backup_fd = -1;
>  
>  	odata = info->array.raid_disks - info->delta_disks - 1;
>  	if (info->array.level == 6)
> @@ -4732,9 +4743,18 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist,
>  		 * been used
>  		 */
>  		old_disks = cnt;
> +
> +	if (backup_file) {
> +		backup_fd = open(backup_file, O_RDONLY);
> +		if (backup_fd < 0) {
> +			pr_err("Can't open backup file %s : %s\n",
> +				backup_file, strerror(errno));
> +			return -EINVAL;
> +		}
> +	}
> +
>  	for (i=old_disks-(backup_file?1:0); i<cnt; i++) {
>  		struct mdinfo dinfo;
> -		int fd;
>  		int bsbsize;
>  		char *devname, namebuf[20];
>  		unsigned long long lo, hi;
> @@ -4747,12 +4767,9 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist,
>  		 * else restore data and update all superblocks
>  		 */
>  		if (i == old_disks-1) {
> -			fd = open(backup_file, O_RDONLY);
> -			if (fd<0) {
> -				pr_err("backup file %s inaccessible: %s\n",
> -					backup_file, strerror(errno));
> +			if (backup_fd < 0)
>  				continue;

You can use is_fd_valid(). Please also review other patches on that.

> -			}
> +			fd = backup_fd;
>  			devname = backup_file;
>  		} else {
>  			fd = fdlist[i];
> @@ -4907,6 +4924,8 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist, pr_err("Error restoring backup from %s\n",
>  					devname);
>  			free(offsets);
> +			if (backup_fd >= 0)
> +				close(backup_fd);
we have close_fd() for that.

>  			return 1;
>  		}
>  
> @@ -4923,6 +4942,8 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist, pr_err("Error restoring second backup from %s\n",
>  					devname);
>  			free(offsets);
> +			if (backup_fd >= 0)
> +				close(backup_fd);

You can use close_fd(). Also please analyze other patches on that.

>  			return 1;
>  		}
>  
> @@ -4984,8 +5005,14 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist, st->ss->store_super(st, fdlist[j]);
>  			st->ss->free_super(st);
>  		}
> +		if (backup_fd >= 0)
> +			close(backup_fd);
>  		return 0;
>  	}
> +
> +	if (backup_fd >= 0)
> +		close(backup_fd);

As above (use close_fd())
> +
>  	/* Didn't find any backup data, try to see if any
>  	 * was needed.
>  	 */

Thanks,
Mariusz