Re: [Xen-devel] [PATCH] tools/xenstore: domain can sometimes disappear when destroying connection

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Nov 29, 2018 at 01:10:52PM +0000, Eftime, Petre wrote:
> I didn't go extremely deep in my debugging, as the talloc library is a bit
> difficult to debug, but under the do_introduce function you have these
> two lines:

difficult to debug> I completely agree. ;-)

> 
>                  /* Now domain belongs to its connection. */
>                  talloc_steal(domain->conn, domain);
> 

Thanks, I think this is what I missed. This is useful information. I
think it should go into the commit message if we end up committing your
patch.

> After these happen, destroying the domain leads to a SIGSEGV in xenstored, as
> when conn gets freed, so does domain, which ends up in a use-after-free.
> 
> I've posted the patch with the fixed text.

Yes, saw that. Thanks for the quick response. There is no need to post
another one just yet. Please give us some time while we try to figure
out the root cause.

Wei.

> 
> Best,
> Petre
> 
> On 2018-11-29, 14:54, "Wei Liu" <wei.liu2@xxxxxxxxxx> wrote:
> 
>     On Mon, Nov 26, 2018 at 01:22:04PM +0000, Petre Eftime wrote:
>     > There is a circular link formed between domain and a connection. In 
> certain
>     > circustances, when conn is freed, domain is also freed, which leads to 
> use
>     > after free when trying to set the conn field in domain to null.
>     
>     Actually, can you provide more context on this? When will the circular
>     link happen?
>     
>     Wei.
>     
>     > 
>     > Signed-off-by: Petre Eftime <epetre@xxxxxxxxxx>
>     > ---
>     >  tools/xenstore/xenstored_domain.c | 9 ++++++++-
>     >  1 file changed, 8 insertions(+), 1 deletion(-)
>     > 
>     > diff --git a/tools/xenstore/xenstored_domain.c 
> b/tools/xenstore/xenstored_domain.c
>     > index fa6655033a..f085d40476 100644
>     > --- a/tools/xenstore/xenstored_domain.c
>     > +++ b/tools/xenstore/xenstored_domain.c
>     > @@ -222,6 +222,7 @@ static void domain_cleanup(void)
>     >  {
>     >         xc_dominfo_t dominfo;
>     >         struct domain *domain;
>     > +       struct connection *tmp_conn;
>     >         int notify = 0;
>     >  
>     >   again:
>     > @@ -238,8 +239,14 @@ static void domain_cleanup(void)
>     >                                 continue;
>     >                 }
>     >                 if (domain->conn) {
>     > -                       talloc_unlink(talloc_autofree_context(), 
> domain->conn);
>     > +                       /*
>     > +                        * In certain circumstances conn owns domain and
>     > +                        * domain will be freed when conn is unlinked.
>     > +                        */
>     > +                       tmp_conn = domain->conn;
>     >                         domain->conn = NULL;
>     > +
>     > +                       talloc_unlink(talloc_autofree_context(), 
> tmp_conn);
>     >                         notify = 0; /* destroy_domain() fires the watch 
> */
>     >                         goto again;
>     >                 }
>     > -- 
>     > 2.16.5
>     > 
>     > 
>     > 
>     > 
>     > Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. 
> Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered 
> in Romania. Registration number J22/2621/2005.
>     > 
>     
> 
> 
> 
> 
> Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar 
> Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in 
> Romania. Registration number J22/2621/2005.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel