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

[Joel Sherrill]

On Mon, Mar 15, 2021, 6:10 PM Chris Johns <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>> 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? Or add block dev destroy? What concerns me
about that is that it is another layer in the onion we (Ryan and I) don't
fully understand the interactions.

Is blocked destroy going to be just resources or will it have to interrupt
outstanding work, etc?

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