On Mon, Mar 15, 2021, 8:00 PM Chris Johns <chr...@rtems.org> wrote: > 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? >
I guess so. I never worked on this code. We can check the coverage on it and make sure we have something that exercises it first. --joel > > Chris >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
