<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 15, 2021, 8:00 PM Chris Johns <<a href="mailto:chrisj@rtems.org">chrisj@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 16/3/21 11:49 am, Joel Sherrill wrote:<br>
> On Mon, Mar 15, 2021, 6:10 PM Chris Johns <<a href="mailto:chrisj@rtems.org" target="_blank" rel="noreferrer">chrisj@rtems.org</a><br>
> <mailto:<a href="mailto:chrisj@rtems.org" target="_blank" rel="noreferrer">chrisj@rtems.org</a>>> wrote:<br>
> <br>
> On 15/3/21 2:21 pm, Joel Sherrill wrote:<br>
> > On Sun, Mar 14, 2021, 9:27 PM Chris Johns <<a href="mailto:chrisj@rtems.org" target="_blank" rel="noreferrer">chrisj@rtems.org</a><br>
> <mailto:<a href="mailto:chrisj@rtems.org" target="_blank" rel="noreferrer">chrisj@rtems.org</a>><br>
> > <mailto:<a href="mailto:chrisj@rtems.org" target="_blank" rel="noreferrer">chrisj@rtems.org</a> <mailto:<a href="mailto:chrisj@rtems.org" target="_blank" rel="noreferrer">chrisj@rtems.org</a>>>> wrote:<br>
> > On 13/3/21 2:18 am, Ryan Long wrote:<br>
> > > CID 1439298: Resource leak in rtems_fdisk_initialize().<br>
> > ><br>
> > > Closes #4299<br>
> > > ---<br>
> > > cpukit/libblock/src/flashdisk.c | 42<br>
> > ++++++++++++++++++++++++++++++-----------<br>
> > > 1 file changed, 31 insertions(+), 11 deletions(-)<br>
> > ><br>
> > > diff --git a/cpukit/libblock/src/flashdisk.c<br>
> b/cpukit/libblock/src/flashdisk.c<br>
> > > index 91f99e0..c4bac82 100644<br>
> > > --- a/cpukit/libblock/src/flashdisk.c<br>
> > > +++ b/cpukit/libblock/src/flashdisk.c<br>
> > > @@ -2463,6 +2463,7 @@ rtems_fdisk_initialize<br>
> (rtems_device_major_number major,<br>
> > > {<br>
> > > char name[] = RTEMS_FLASHDISK_DEVICE_BASE_NAME "a";<br>
> > > uint32_t device;<br>
> > > + uint32_t device_to_free;<br>
> > > uint32_t blocks = 0;<br>
> > > int ret;<br>
> > > <br>
> > > @@ -2485,18 +2486,27 @@ rtems_fdisk_initialize<br>
> (rtems_device_major_number<br>
> > major,<br>
> > > * One copy buffer of a page size.<br>
> > > */<br>
> > > fd->copy_buffer = malloc (c->block_size);<br>
> > > - if (!fd->copy_buffer)<br>
> > > + if (!fd->copy_buffer) {<br>
> > > + free(fd);<br>
> > > return RTEMS_NO_MEMORY;<br>
> > > + }<br>
> > > <br>
> > > fd->blocks = calloc (blocks, sizeof (rtems_fdisk_block_ctl));<br>
> > > - if (!fd->blocks)<br>
> > > + if (!fd->blocks) {<br>
> > > + free(fd->copy_buffer);<br>
> > > + free(fd);<br>
> > > return RTEMS_NO_MEMORY;<br>
> > > + }<br>
> > > <br>
> > > fd->block_count = blocks;<br>
> > > <br>
> > > fd->devices = calloc (c->device_count, sizeof<br>
> (rtems_fdisk_device_ctl));<br>
> > > - if (!fd->devices)<br>
> > > + if (!fd->devices) {<br>
> > > + free (fd->blocks);<br>
> > > + free (fd->copy_buffer);<br>
> > > + free (fd);<br>
> > > return RTEMS_NO_MEMORY;<br>
> > > + }<br>
> > > <br>
> > > rtems_mutex_init (&fd->lock, "Flash Disk");<br>
> > > <br>
> > > @@ -2505,9 +2515,10 @@ rtems_fdisk_initialize (rtems_device_major_number<br>
> > major,<br>
> > > if (sc != RTEMS_SUCCESSFUL)<br>
> > > {<br>
> > > rtems_mutex_destroy (&fd->lock);<br>
> > > - free (fd->copy_buffer);<br>
> > > - free (fd->blocks);<br>
> > > free (fd->devices);<br>
> > > + free (fd->blocks);<br>
> > > + free (fd->copy_buffer);<br>
> ><br>
> > Why the order change?<br>
> ><br>
> > Does the change make it exactly the opposite order of creation or do you<br>
> see it<br>
> > not being in inverse order?<br>
> <br>
> If there is no reason to change the order the blocks are freed then I suggest<br>
> not changing the order. It avoids adding noise to the change.<br>
> <br>
> > This was a hard one. It was missing a LOT of cleanup.<br>
> ><br>
> ><br>
> > > + free (fd);<br>
> ><br>
> > What happens to the created blkdev the fd is passed into? Does that<br>
> need to be<br>
> > destroyed before this is released?<br>
> <br>
> I do not know. I have not looked.<br>
> <br>
> > I didn't recognise that as an allocation. What's the destroy call for that?<br>
> <br>
> If the block dev holds the pointer and you have freed it bad things will happen.<br>
> <br>
> I have a funny feeling there was no block dev destroy when the code was written<br>
> and why there are no free's for this allocation.<br>
> <br>
> <br>
> Is this one we should leave a version of the patch on the ticket, link to this<br>
> threadz and walk away from? <br>
<br>
The patch is wrong so I do not think it is a good idea holding it on the ticket.<br>
<br>
Walking away ... I have walked around this one for a while now so that is what<br>
we have been doing but it does not resolve the coverity side of things.<br>
<br>
> Or add block dev destroy? <br>
<br>
This is one option and would be a reasonable path but I am not sure how deep the<br>
pit it opens is.<br>
<br>
> What concerns me about<br>
> that is that it is another layer in the onion we (Ryan and I) don't fully<br>
> understand the interactions.<br>
<br>
Yeah, this is where it gets hard. What does it mean if the nvdisk create fails?<br>
<br>
> Is blocked destroy going to be just resources or will it have to interrupt<br>
> outstanding work, etc?<br>
<br>
I do not know.<br>
<br>
Can this code be arrange so everything but the block dev create is done and if<br>
that fails you can undo everything else?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">--joel</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Chris<br>
</blockquote></div></div></div>