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

2021-03-15 Thread Joel Sherrill 
On Mon, Mar 15, 2021, 8:00 PM Chris Johns  wrote:

> On 16/3/21 11:49 am, Joel Sherrill wrote:
> > On Mon, Mar 15, 2021, 6:10 PM Chris Johns  > > wrote:
> >
> > On 15/3/21 2:21 pm, Joel Sherrill wrote:
> > > On Sun, Mar 14, 2021, 9:27 PM Chris Johns  > 
> > > >> 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 (>lock, "Flash Disk");
> > > >
> > > > @@ -2505,9 +2515,10 @@ rtems_fdisk_initialize
> (rtems_device_major_number
> > > major,
> > > >  if (sc != RTEMS_SUCCESSFUL)
> > > >  {
> > > >rtems_mutex_destroy (>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

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

2021-03-15 Thread Chris Johns 
On 16/3/21 11:49 am, Joel Sherrill wrote:
> On Mon, Mar 15, 2021, 6:10 PM Chris Johns  > wrote:
> 
> On 15/3/21 2:21 pm, Joel Sherrill wrote:
> > On Sun, Mar 14, 2021, 9:27 PM Chris Johns  
> > >> 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 (>lock, "Flash Disk");
> >     > 
> >     > @@ -2505,9 +2515,10 @@ rtems_fdisk_initialize 
> (rtems_device_major_number
> >     major,
> >     >      if (sc != RTEMS_SUCCESSFUL)
> >     >      {
> >     >        rtems_mutex_destroy (>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
___
devel mailing list
devel@rtems.org

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

2021-03-15 Thread Joel Sherrill 
On Mon, Mar 15, 2021, 6:10 PM Chris Johns  wrote:

> On 15/3/21 2:21 pm, Joel Sherrill wrote:
> > On Sun, Mar 14, 2021, 9:27 PM Chris Johns  > > 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 (>lock, "Flash Disk");
> > >
> > > @@ -2505,9 +2515,10 @@ rtems_fdisk_initialize
> (rtems_device_major_number
> > major,
> > >  if (sc != RTEMS_SUCCESSFUL)
> > >  {
> > >rtems_mutex_destroy (>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
>
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

2021-03-15 Thread Chris Johns 
On 15/3/21 2:21 pm, Joel Sherrill wrote:
> On Sun, Mar 14, 2021, 9:27 PM Chris Johns  > 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 (>lock, "Flash Disk");
> > 
> > @@ -2505,9 +2515,10 @@ rtems_fdisk_initialize (rtems_device_major_number
> major,
> >      if (sc != RTEMS_SUCCESSFUL)
> >      {
> >        rtems_mutex_destroy (>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
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

2021-03-14 Thread Joel Sherrill 
On Sun, Mar 14, 2021, 9:27 PM Chris Johns  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 (>lock, "Flash Disk");
> >
> > @@ -2505,9 +2515,10 @@ rtems_fdisk_initialize (rtems_device_major_number
> major,
> >  if (sc != RTEMS_SUCCESSFUL)
> >  {
> >rtems_mutex_destroy (>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?

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 didn't recognise that as an allocation. What's the destroy call for that?

>
> Same for the other cases below?
>
> Chris
>
> >rtems_fdisk_error ("disk create phy failed");
> >return sc;
> >  }
> > @@ -2524,11 +2535,14 @@ rtems_fdisk_initialize
> (rtems_device_major_number major,
> >   sizeof
> (rtems_fdisk_segment_ctl));
> >if (!fd->devices[device].segments)
> >{
> > +for (device_to_free = device - 1; device_to_free >= 0;
> device_to_free--)
> > +   free(fd->devices[device_to_free].segments);
> >  unlink (name);
> >  rtems_mutex_destroy (>lock);
> > -free (fd->copy_buffer);
> > -free (fd->blocks);
> >  free (fd->devices);
> > +free (fd->blocks);
> > +free (fd->copy_buffer);
> > +free (fd);
> >  return RTEMS_NO_MEMORY;
> >}
> >
> > @@ -2559,11 +2573,14 @@ rtems_fdisk_initialize
> (rtems_device_major_number major,
> >  ret = rtems_fdisk_recover_block_mappings (fd);
> >  if (ret)
> >  {
> > +  for (device_to_free = device - 1; device_to_free >= 0;
> device_to_free--)
> > +free(fd->devices[device_to_free].segments);
> >unlink (name);
> >rtems_mutex_destroy (>lock);
> > -  free (fd->copy_buffer);
> > -  free (fd->blocks);
> >free (fd->devices);
> > +  free (fd->blocks);
> > +  free (fd->copy_buffer);
> > +  free (fd);
> >rtems_fdisk_error ("recovery of disk failed: %s (%d)",
> >   strerror (ret), ret);
> >return ret;
> > @@ -2572,11 +2589,14 @@ rtems_fdisk_initialize
> (rtems_device_major_number major,
> >  ret = rtems_fdisk_compact (fd);
> >  if (ret)
> >  {
> > +  for (device_to_free = device - 1; device_to_free >= 0;
> device_to_free--)
> > +free(fd->devices[device_to_free].segments);
> >unlink (name);
> >rtems_mutex_destroy (>lock);
> > -  free (fd->copy_buffer);
> > -  free (fd->blocks);
> >free (fd->devices);
> > +  free (fd->blocks);
> > +  free (fd->copy_buffer);
> > +  free (fd);
> >rtems_fdisk_error ("compacting of disk failed: %s (%d)",
> >   strerror (ret), ret);
> >return ret;
> >
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

2021-03-14 Thread Chris Johns 



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 (>lock, "Flash Disk");
>  
> @@ -2505,9 +2515,10 @@ rtems_fdisk_initialize (rtems_device_major_number 
> major,
>  if (sc != RTEMS_SUCCESSFUL)
>  {
>rtems_mutex_destroy (>lock);
> -  free (fd->copy_buffer);
> -  free (fd->blocks);
>free (fd->devices);
> +  free (fd->blocks);
> +  free (fd->copy_buffer);

Why the order change?

> +  free (fd);

What happens to the created blkdev the fd is passed into? Does that need to be
destroyed before this is released?

Same for the other cases below?

Chris

>rtems_fdisk_error ("disk create phy failed");
>return sc;
>  }
> @@ -2524,11 +2535,14 @@ rtems_fdisk_initialize (rtems_device_major_number 
> major,
>   sizeof 
> (rtems_fdisk_segment_ctl));
>if (!fd->devices[device].segments)
>{
> +for (device_to_free = device - 1; device_to_free >= 0; 
> device_to_free--)
> +   free(fd->devices[device_to_free].segments);
>  unlink (name);
>  rtems_mutex_destroy (>lock);
> -free (fd->copy_buffer);
> -free (fd->blocks);
>  free (fd->devices);
> +free (fd->blocks);
> +free (fd->copy_buffer);
> +free (fd);
>  return RTEMS_NO_MEMORY;
>}
>  
> @@ -2559,11 +2573,14 @@ rtems_fdisk_initialize (rtems_device_major_number 
> major,
>  ret = rtems_fdisk_recover_block_mappings (fd);
>  if (ret)
>  {
> +  for (device_to_free = device - 1; device_to_free >= 0; 
> device_to_free--)
> +free(fd->devices[device_to_free].segments);
>unlink (name);
>rtems_mutex_destroy (>lock);
> -  free (fd->copy_buffer);
> -  free (fd->blocks);
>free (fd->devices);
> +  free (fd->blocks);
> +  free (fd->copy_buffer);
> +  free (fd);
>rtems_fdisk_error ("recovery of disk failed: %s (%d)",
>   strerror (ret), ret);
>return ret;
> @@ -2572,11 +2589,14 @@ rtems_fdisk_initialize (rtems_device_major_number 
> major,
>  ret = rtems_fdisk_compact (fd);
>  if (ret)
>  {
> +  for (device_to_free = device - 1; device_to_free >= 0; 
> device_to_free--)
> +free(fd->devices[device_to_free].segments);
>unlink (name);
>rtems_mutex_destroy (>lock);
> -  free (fd->copy_buffer);
> -  free (fd->blocks);
>free (fd->devices);
> +  free (fd->blocks);
> +  free (fd->copy_buffer);
> +  free (fd);
>rtems_fdisk_error ("compacting of disk failed: %s (%d)",
>   strerror (ret), ret);
>return ret;
> 
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

2021-03-12 Thread Ryan Long 
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 (>lock, "Flash Disk");
 
@@ -2505,9 +2515,10 @@ rtems_fdisk_initialize (rtems_device_major_number major,
 if (sc != RTEMS_SUCCESSFUL)
 {
   rtems_mutex_destroy (>lock);
-  free (fd->copy_buffer);
-  free (fd->blocks);
   free (fd->devices);
+  free (fd->blocks);
+  free (fd->copy_buffer);
+  free (fd);
   rtems_fdisk_error ("disk create phy failed");
   return sc;
 }
@@ -2524,11 +2535,14 @@ rtems_fdisk_initialize (rtems_device_major_number major,
  sizeof (rtems_fdisk_segment_ctl));
   if (!fd->devices[device].segments)
   {
+for (device_to_free = device - 1; device_to_free >= 0; 
device_to_free--)
+ free(fd->devices[device_to_free].segments);
 unlink (name);
 rtems_mutex_destroy (>lock);
-free (fd->copy_buffer);
-free (fd->blocks);
 free (fd->devices);
+free (fd->blocks);
+free (fd->copy_buffer);
+free (fd);
 return RTEMS_NO_MEMORY;
   }
 
@@ -2559,11 +2573,14 @@ rtems_fdisk_initialize (rtems_device_major_number major,
 ret = rtems_fdisk_recover_block_mappings (fd);
 if (ret)
 {
+  for (device_to_free = device - 1; device_to_free >= 0; device_to_free--)
+free(fd->devices[device_to_free].segments);
   unlink (name);
   rtems_mutex_destroy (>lock);
-  free (fd->copy_buffer);
-  free (fd->blocks);
   free (fd->devices);
+  free (fd->blocks);
+  free (fd->copy_buffer);
+  free (fd);
   rtems_fdisk_error ("recovery of disk failed: %s (%d)",
  strerror (ret), ret);
   return ret;
@@ -2572,11 +2589,14 @@ rtems_fdisk_initialize (rtems_device_major_number major,
 ret = rtems_fdisk_compact (fd);
 if (ret)
 {
+  for (device_to_free = device - 1; device_to_free >= 0; device_to_free--)
+free(fd->devices[device_to_free].segments);
   unlink (name);
   rtems_mutex_destroy (>lock);
-  free (fd->copy_buffer);
-  free (fd->blocks);
   free (fd->devices);
+  free (fd->blocks);
+  free (fd->copy_buffer);
+  free (fd);
   rtems_fdisk_error ("compacting of disk failed: %s (%d)",
  strerror (ret), ret);
   return ret;
-- 
1.8.3.1

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel