Re: [Xen-devel] [PATCH for-next 5/8] x86/domain: factor out pv_domain_destroy

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Apr 10, 2017 at 04:16:12PM +0100, Andrew Cooper wrote:
> On 10/04/17 16:12, Wei Liu wrote:
> > On Mon, Apr 10, 2017 at 04:04:22PM +0100, Andrew Cooper wrote:
> >> On 10/04/17 14:27, Wei Liu wrote:
> >>> No functional change.
> >>>
> >>> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> >> Throughout this series, please make sure you add in proper NULL'ing of
> >> freed data.
> >>
> >> While this patch is no functional change at the moment, you have
> >> introduced a latent double-free bug for if (/when) pv_domain_destroy()
> >> gets used on a failed create path.
> >>
> > No it won't. I made pv_domain_initialise idempotent.
> 
> Ah - I hadn't spotted that.
> 
> My long-term goal is to reorder the init/destroy logic such that init
> has no cleanup in it, and any failure comes out and calls destroy at the
> top level.

Just like what we do in libxl? :-)

> 
> This will avoid us opencoding the teardown logic twice;  we have had
> bugs in the past where the two paths are different (the most common of
> which is missing cleanup in the destroy path, leading to a memory leak).
> 

Given there is going to be a lot of code churn, I think the best bet at
the moment is to have the teardown logic twice, with each setting
respective fields to their "NULL" state. Although that's a bit
excessive, but it would prevent accidental memory leak or double free
errors. And then after we are happy with the overall structures of
hypervisor code, we can then audit the code to remove the clean up code
in _init.

What do you think?

Wei.

> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel