diff mbox series

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

Message ID 20240715073604.30307-4-xni@redhat.com (mailing list archive)
State Superseded
Headers show
Series mdadm: fix coverity issues | expand

Checks

Context Check Description
mdraidci/vmtest-md-6_11-PR fail merge-conflict

Commit Message

Xiao Ni July 15, 2024, 7:35 a.m. UTC 
Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 13 deletions(-)

Comments

Mariusz Tkaczyk July 17, 2024, 11:29 a.m. UTC | #1 
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
Xiao Ni July 18, 2024, 3:27 a.m. UTC | #2 
On Wed, Jul 17, 2024 at 7:29 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> 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.

Ok, I'll keep the original way.

@@ -530,8 +530,9 @@ int Grow_addbitmap(char *devname, int fd, struct
context *c, struct shape *s)
                                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 1;
        }


>
> >       }
> >
> >       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.

How about this:
-       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);
        if (st->ss->external) {
                /* metadata handler takes it from here */
                done = st->ss->manage_reshape(
@@ -3632,6 +3634,7 @@ started:
                        fd, sra, &reshape, st, blocks, fdlist, offsets,
                        d - odisks, fdlist + odisks, offsets + odisks);

+       close_fd(&fd);
        free(fdlist);
        free(offsets);
>
> 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.

I'll replace them in the next version. But I think it should depend on
the specific case. Sometimes, we don't need to reset fd to -1, because
we don't use it anymore. So the standard close function is better.

>
>
> >       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.

I searched by command `cat * | grep "< 0"`, this is the only place.
I'll change it. But is it really convenient? if_fd_valid just does the
same thing. Because if_fd_valid is not a standard C api, developers
may not know this api.

>
> > -                     }
> > +                     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.

Ok,  I will do it.

Best Regards
Xiao
>
> >                       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
>
Mariusz Tkaczyk July 19, 2024, 9:52 a.m. UTC | #3 
On Thu, 18 Jul 2024 11:27:50 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Wed, Jul 17, 2024 at 7:29 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > 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.  
> 
> Ok, I'll keep the original way.
> 
> @@ -530,8 +530,9 @@ int Grow_addbitmap(char *devname, int fd, struct
> context *c, struct shape *s)
>                                 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 1;
>         }
> 
> 
> >  
> > >       }
> > >
> > >       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.  
> 
> How about this:
> -       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);
>         if (st->ss->external) {
>                 /* metadata handler takes it from here */
>                 done = st->ss->manage_reshape(
> @@ -3632,6 +3634,7 @@ started:
>                         fd, sra, &reshape, st, blocks, fdlist, offsets,
>                         d - odisks, fdlist + odisks, offsets + odisks);
> 
> +       close_fd(&fd);
>         free(fdlist);
>         free(offsets);
> >
> > 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.  
> 
> I'll replace them in the next version. But I think it should depend on
> the specific case. Sometimes, we don't need to reset fd to -1, because
> we don't use it anymore. So the standard close function is better.

If this is unused later then compiler will should skip the assignment (it should
be detected and optimized during compilation). Not a big deal I think
but you are right, if we know that, we don't have to use this extended close
logic.

In this case we have massive function and fd is closed and opened many times
and there is a fork in the middle of it. I cannot follow all usages.
I would prefer to make it safer. There must be a reason why there is else fd =
-1; branch. Having close_fd(&fd) instead of close(fd) everywhere has following
advantages:
- double close attempt is not possible (because fd is updated to -1 each time)
- If fd value is the number now reused (we may open different file descriptor
  after close(fd)), it won't be unexpectedly closed.
- if the mentioned close_fd(&fd) few lines earlier is not needed then is
  skipped.

I know that it might be not optimal but I accept that as a compromise between
massive, overextended code and application security.

> 
> >
> >  
> > >       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.  
> 
> I searched by command `cat * | grep "< 0"`, this is the only place.
> I'll change it. But is it really convenient? if_fd_valid just does the
> same thing. Because if_fd_valid is not a standard C api, developers
> may not know this api.
> 

It is easy to find so I don't see it as a problem.

For example in systemd we have:

        _cleanup_close_ int fd = -1;

I don't know how it works and for that reason shouldn't I use it? I have to
believe that developer who added it implemented it correctly.

Same here, name is pretty straightforward so we can use this to avoid direct
comparisons which might be bug prone because it is easy to miss <=, > in
review. Especially, that our brains are likely to skip the common patterns.

We had following problem in the past:
https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=b71de056cec70784ef2727e2febd7a6c88e580db

Thanks,
Mariusz
diff mbox series

Patch

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;
 	}
 
 	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;
 	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;
-			}
+			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);
 			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);
 			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);
+
 	/* Didn't find any backup data, try to see if any
 	 * was needed.
 	 */