[PATCH 4/5] flashdisk.c: Fix Resource leak (CID #1439298)

[Joel Sherrill]

On Mon, Mar 15, 2021, 8:00 PM Chris Johns <chrisj at rtems.org> wrote:

> On 16/3/21 11:49 am, Joel Sherrill wrote:
> > On Mon, Mar 15, 2021, 6:10 PM Chris Johns <chrisj at rtems.org
> > <mailto:chrisj at rtems.org>> wrote:
> >
> >     On 15/3/21 2:21 pm, Joel Sherrill wrote:
> >     > On Sun, Mar 14, 2021, 9:27 PM Chris Johns <chrisj at rtems.org
> >     <mailto:chrisj at rtems.org>
> >     > <mailto:chrisj at rtems.org <mailto:chrisj at rtems.org>>> wrote:
> >     >     On 13/3/21 2:18 am, Ryan Long wrote:
> >     >     > CID 1439298: Resource leak in rtems_fdisk_initialize().
> >     >     >
> >     >     > Closes #4299
> >     >     > ---
> >     >     >  cpukit/libblock/src/flashdisk.c | 42
> >     >     ++++++++++++++++++++++++++++++-----------
> >     >     >  1 file changed, 31 insertions(+), 11 deletions(-)
> >     >     >
> >     >     > diff --git a/cpukit/libblock/src/flashdisk.c
> >     b/cpukit/libblock/src/flashdisk.c
> >     >     > index 91f99e0..c4bac82 100644
> >     >     > --- a/cpukit/libblock/src/flashdisk.c
> >     >     > +++ b/cpukit/libblock/src/flashdisk.c
> >     >     > @@ -2463,6 +2463,7 @@ rtems_fdisk_initialize
> >     (rtems_device_major_number major,
> >     >     >    {
> >     >     >      char     name[] = RTEMS_FLASHDISK_DEVICE_BASE_NAME "a";
> >     >     >      uint32_t device;
> >     >     > +    uint32_t device_to_free;
> >     >     >      uint32_t blocks = 0;
> >     >     >      int      ret;
> >     >     >
> >     >     > @@ -2485,18 +2486,27 @@ rtems_fdisk_initialize
> >     (rtems_device_major_number
> >     >     major,
> >     >     >       * One copy buffer of a page size.
> >     >     >       */
> >     >     >      fd->copy_buffer = malloc (c->block_size);
> >     >     > -    if (!fd->copy_buffer)
> >     >     > +    if (!fd->copy_buffer) {
> >     >     > +      free(fd);
> >     >     >        return RTEMS_NO_MEMORY;
> >     >     > +    }
> >     >     >
> >     >     >      fd->blocks = calloc (blocks, sizeof
> (rtems_fdisk_block_ctl));
> >     >     > -    if (!fd->blocks)
> >     >     > +    if (!fd->blocks) {
> >     >     > +      free(fd->copy_buffer);
> >     >     > +      free(fd);
> >     >     >        return RTEMS_NO_MEMORY;
> >     >     > +    }
> >     >     >
> >     >     >      fd->block_count = blocks;
> >     >     >
> >     >     >      fd->devices = calloc (c->device_count, sizeof
> >     (rtems_fdisk_device_ctl));
> >     >     > -    if (!fd->devices)
> >     >     > +    if (!fd->devices) {
> >     >     > +      free (fd->blocks);
> >     >     > +      free (fd->copy_buffer);
> >     >     > +      free (fd);
> >     >     >        return RTEMS_NO_MEMORY;
> >     >     > +    }
> >     >     >
> >     >     >      rtems_mutex_init (&fd->lock, "Flash Disk");
> >     >     >
> >     >     > @@ -2505,9 +2515,10 @@ rtems_fdisk_initialize
> (rtems_device_major_number
> >     >     major,
> >     >     >      if (sc != RTEMS_SUCCESSFUL)
> >     >     >      {
> >     >     >        rtems_mutex_destroy (&fd->lock);
> >     >     > -      free (fd->copy_buffer);
> >     >     > -      free (fd->blocks);
> >     >     >        free (fd->devices);
> >     >     > +      free (fd->blocks);
> >     >     > +      free (fd->copy_buffer);
> >     >
> >     >     Why the order change?
> >     >
> >     > Does the change make it exactly the opposite order of creation or
> do you
> >     see it
> >     > not being in inverse order?
> >
> >     If there is no reason to change the order the blocks are freed then
> I suggest
> >     not changing the order. It avoids adding noise to the change.
> >
> >     > This was a hard one. It was missing a LOT of cleanup.
> >     >
> >     >
> >     >     > +      free (fd);
> >     >
> >     >     What happens to the created blkdev the fd is passed into? Does
> that
> >     need to be
> >     >     destroyed before this is released?
> >
> >     I do not know. I have not looked.
> >
> >     > I didn't recognise that as an allocation. What's the destroy call
> for that?
> >
> >     If the block dev holds the pointer and you have freed it bad things
> will happen.
> >
> >     I have a funny feeling there was no block dev destroy when the code
> was written
> >     and why there are no free's for this allocation.
> >
> >
> > Is this one we should leave a version of the patch on the ticket, link
> to this
> > threadz and walk away from?
>
> The patch is wrong so I do not think it is a good idea holding it on the
> ticket.
>
> Walking away ... I have walked around this one for a while now so that is
> what
> we have been doing but it does not resolve the coverity side of things.
>
> > Or add block dev destroy?
>
> This is one option and would be a reasonable path but I am not sure how
> deep the
> pit it opens is.
>
> > What concerns me about
> > that is that it is another layer in the onion we (Ryan and I) don't fully
> > understand the interactions.
>
> Yeah, this is where it gets hard. What does it mean if the nvdisk create
> fails?
>
> > Is blocked destroy going to be just resources or will it have to
> interrupt
> > outstanding work, etc?
>
> I do not know.
>
> Can this code be arrange so everything but the block dev create is done
> and if
> that fails you can undo everything else?
>

I guess so. I never worked on this code. We can check the coverage on it
and make sure we have something that exercises it first.

--joel

>
> Chris
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210315/1396dfb4/attachment.html>