On 16/3/21 11:49 am, Joel Sherrill wrote: > On Mon, Mar 15, 2021, 6:10 PM Chris Johns <chr...@rtems.org > <mailto:chr...@rtems.org>> wrote: > > On 15/3/21 2:21 pm, Joel Sherrill wrote: > > On Sun, Mar 14, 2021, 9:27 PM Chris Johns <chr...@rtems.org > <mailto:chr...@rtems.org> > > <mailto:chr...@rtems.org <mailto:chr...@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 _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
