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

[Chris Johns]

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

Chris