[PATCH v2] libxl: prepare environment for domcreate_stream_done

[PATCH v2] libxl: prepare environment for domcreate_stream_done

[Olaf Hering]

The function domcreate_bootloader_done may branch early to
domcreate_stream_done, in case some error occoured. Here srs->dcs will be
NULL, which leads to a crash.

It is unclear what the purpose of that backpointer is. Perhaps it can be
removed, and domcreate_stream_done could use CONTAINER_OF.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
v2:
 unconditional assignment

 tools/libxl/libxl_create.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a4e74a5cd2..c8b5ea984b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1093,6 +1093,9 @@ static void domcreate_bootloader_done(libxl__egc *egc,
         return;
     }
 
+    /* Prepare environment for domcreate_stream_done */
+    dcs->srs.dcs = dcs;
+
     /* Restore */
     callbacks->restore_results = libxl__srm_callout_callback_restore_results;
 
@@ -1116,7 +1119,6 @@ static void domcreate_bootloader_done(libxl__egc *egc,
         goto out;
 
     dcs->srs.ao = ao;
-    dcs->srs.dcs = dcs;
     dcs->srs.fd = restore_fd;
     dcs->srs.legacy = (dcs->restore_params.stream_version == 1);
     dcs->srs.back_channel = false;

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

Re: [PATCH v2] libxl: prepare environment for domcreate_stream_done

[Wei Liu]

On Fri, Mar 08, 2019 at 01:24:15PM +0100, Olaf Hering wrote:
> The function domcreate_bootloader_done may branch early to
> domcreate_stream_done, in case some error occoured. Here srs->dcs will be
> NULL, which leads to a crash.
> 
> It is unclear what the purpose of that backpointer is. Perhaps it can be
> removed, and domcreate_stream_done could use CONTAINER_OF.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v2] libxl: prepare environment for domcreate_stream_done

[Ian Jackson]

Olaf Hering writes ("[PATCH v2] libxl: prepare environment for domcreate_stream_done"):
> The function domcreate_bootloader_done may branch early to
> domcreate_stream_done, in case some error occoured. Here srs->dcs will be
> NULL, which leads to a crash.

Thanks.  I think this is OK as far as it goes.  But:

> +    /* Prepare environment for domcreate_stream_done */
> +    dcs->srs.dcs = dcs;

The need for this comment is telling us something about the weird code
structure here.  We initialise all of dcs->srs much later, so even
with this change we call domcreate_stream_done with a
mostly-uninitialised srs.

In fact this is OK because domcreate_stream_done only reads srs->dcs
and then does everything with the obtained dcs.  But there is nothing
there to indicate that srs might be mostly uninitialised.  Maybe we
could add a comment there, something like:

  /* NB perhaps only srs->dcs is valid; eg in the case of an
   * early branch to domcreate_bootloader_done's `out' block */

?

Ian.

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

Re: [PATCH v2] libxl: prepare environment for domcreate_stream_done

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 557 bytes --]

Am Fri, 8 Mar 2019 14:08:10 +0000
schrieb Ian Jackson <ian.jackson@citrix.com>:

> In fact this is OK because domcreate_stream_done only reads srs->dcs
> and then does everything with the obtained dcs.  But there is nothing
> there to indicate that srs might be mostly uninitialised.  Maybe we
> could add a comment there, something like:
> 
>   /* NB perhaps only srs->dcs is valid; eg in the case of an
>    * early branch to domcreate_bootloader_done's `out' block */

I'm find with that. Can this comment be adjusted at commit time?

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v2] libxl: prepare environment for domcreate_stream_done

[Wei Liu]

On Fri, Mar 08, 2019 at 03:43:18PM +0100, Olaf Hering wrote:
> Am Fri, 8 Mar 2019 14:08:10 +0000
> schrieb Ian Jackson <ian.jackson@citrix.com>:
> 
> > In fact this is OK because domcreate_stream_done only reads srs->dcs
> > and then does everything with the obtained dcs.  But there is nothing
> > there to indicate that srs might be mostly uninitialised.  Maybe we
> > could add a comment there, something like:
> > 
> >   /* NB perhaps only srs->dcs is valid; eg in the case of an
> >    * early branch to domcreate_bootloader_done's `out' block */
> 
> I'm find with that. Can this comment be adjusted at commit time?

Sure.

Wei.

> 
> Olaf



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

Re: [PATCH for-4.12 v2] libxl: prepare environment for domcreate_stream_done

[Ian Jackson]

Olaf Hering writes ("Re: [PATCH v2] libxl: prepare environment for domcreate_stream_done"):
> Am Fri, 8 Mar 2019 14:08:10 +0000
> schrieb Ian Jackson <ian.jackson@citrix.com>:
> 
> > In fact this is OK because domcreate_stream_done only reads srs->dcs
> > and then does everything with the obtained dcs.  But there is nothing
> > there to indicate that srs might be mostly uninitialised.  Maybe we
> > could add a comment there, something like:
> > 
> >   /* NB perhaps only srs->dcs is valid; eg in the case of an
> >    * early branch to domcreate_bootloader_done's `out' block */
> 
> I'm find with that. Can this comment be adjusted at commit time?

Sure, although if it's me that commits it I'd do it as a followup
patch.

We need a release ack I think for 4.12.  Subject adjusted, CCing
Juergen.  Juergen, will bounce the original patch to you too.

Ian.

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

Re: [PATCH for-4.12 v2] libxl: prepare environment for domcreate_stream_done

[Juergen Gross]

On 08/03/2019 13:24, Olaf Hering wrote:
> The function domcreate_bootloader_done may branch early to
> domcreate_stream_done, in case some error occoured. Here srs->dcs will be
> NULL, which leads to a crash.
> 
> It is unclear what the purpose of that backpointer is. Perhaps it can be
> removed, and domcreate_stream_done could use CONTAINER_OF.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

As we are very close to a 4.12 release and this is no recent regression
I'd like to defer this patch to 4.13.


Juergen

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