On Mon, Jun 11 2018, Wol's lists wrote:
> Trying to fix a resource leak in Assemble.c
>
> A superblock is copied into variable tst using dup_super, which does an
> xcalloc.
>
> The section of code in question returns with an error without freeing tst.
Yes, that's a bug. I find it very hard to get excited about freeing all
memory at the right time, because it all gets freed when we call exit().
But I guess it is better to be clean.
>
> Further down, tst is cleared with a call to free_super, which does a
> load of extra work like closing files etc.
It only closes files if there are some open files.
These will be in a linked list of 'struct dl', and at this stage the
pointer to the first 'struct dl' will be NULL. So this is safe.
>
> So my question is basically, which is the correct free function to call?
Definitely tst->ss->free_super(tst).
> I think we're just throwing the duplicate away, in which case xfree is
> the correct function. But elsewhere it is using free_super - which if I
> call wrongly will mess up code accessing the original superblock that
> was copied.
If freeing the copy could hurt the original, then something bad happened
when making the copy.
>
> If you can confirm my suspicion either way, I'll submit a proper patch
> ...
I think you want
if (tst)
tst->ss->free_super(tst);
Thanks,
NeilBrown
>
> Cheers,
> Wol
> diff --git a/Assemble.c b/Assemble.c
> index e83d550..610e869 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -281,7 +281,12 @@ static int select_devices(struct mddev_dev *devlist,
> st->ss->free_super(st);
> dev_policy_free(pol);
> domain_free(domains);
> - return -1;
> +
> + // Which of these is correct ??
> + xfree(tst)
> + tst->ss->free_super(tst);
> +
> + return -1;
> }
>
> if (found_container) {
Attachment:
signature.asc
Description: PGP signature
