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

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

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