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

[Chris Johns]

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?

Chris