[libvirt] [PATCH] vbox: fix SEGV during dumpxml of a serial port

[John Ferlan]

On 01/20/2018 09:52 PM, Laine Stump wrote:
> commit 77a12987a48 changed the "virDomainChrSourceDef source" inside
> virDomainChrDef to "virDomainChrSourceDefPtr source", and started
> allocating source inside virDomainChrDefNew(), but vboxDumpSerial()
> was allocating a virDomainChrDef with a simple VIR_ALLOC(), so source
> was never initialized, leading to a SEGV any time a serial port was
> present. The same problem was created in vboxDumpParallel().
> 
> This patch changes vboxDumpSerial() and vboxDumpParallel() to use
> virDomainChrDefNew() instead of VIR_ALLOC(), and makes a cursory
> attempt to "recover" from OOM afterwards (much of the vbox code was
> written to assume success, e.g. by having functions return void. Since
> I have no way to test a more sweeping change to this code, I chose to
> just short-circuit the rest of the function if virDomainChrDefNew()
> fails - this is at least one step better than the previous code, which
> would otherwise end up trying to dereference a NULL def->serials[i]
> and crash libvirtd.
> 
> This resolves: https://bugzilla.redhat.com/1536649
> ---
>  src/vbox/vbox_common.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 33aefabe5..c5fa5f08b 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -3887,8 +3887,19 @@ vboxDumpSerial(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRUin
>  
>      /* Allocate memory for the serial ports which are enabled */
>      if ((def->nserials > 0) && (VIR_ALLOC_N(def->serials, def->nserials) >= 0)) {
> -        for (i = 0; i < def->nserials; i++)
> -            ignore_value(VIR_ALLOC(def->serials[i]));
> +        for (i = 0; i < def->nserials; i++) {
> +           def->serials[i] = virDomainChrDefNew(NULL);
> +           if (!def->serials[i]) {
> +               /* there is no provision for returning an error
> +                * (although the libvirtd logs will show an OOM error),
> +                * but we need to at least prevent dereferencing
> +                * def->serials[i] and later (including continuing in
> +                * this function), as it will otherwise cause a SEGV.
> +                */

As much as I like this because it's the kind of verbiage I'd put there -
I think we really should just fix the function to cause the failure and
cause the caller to fail as well since it's the right thing to do.

These end up being the dumpxml implementation details and by ignoring
failures we're probably creating bad XML.

There have been some other recent adjustments in this space as recently
as 3.10.  In commit 'c27f79a89' the code changed void vboxDumpIDEHDDs to
be int vboxDumpDisks and went to cleanup.


John

> +               def->nserials = i;
> +               return;
> +           }
> +        }
>      }
>  
>      /* Now get the details about the serial ports here */
> @@ -3975,8 +3986,19 @@ vboxDumpParallel(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine, PRU
>  
>      /* Allocate memory for the parallel ports which are enabled */
>      if ((def->nparallels > 0) && (VIR_ALLOC_N(def->parallels, def->nparallels) >= 0)) {
> -        for (i = 0; i < def->nparallels; i++)
> -            ignore_value(VIR_ALLOC(def->parallels[i]));
> +        for (i = 0; i < def->nparallels; i++) {
> +           def->parallels[i] = virDomainChrDefNew(NULL);
> +           if (!def->parallels[i]) {
> +               /* there is no provision for returning an error
> +                * (although the libvirtd logs will show an OOM error),
> +                * but we need to at least prevent dereferencing
> +                * def->parallels[i] and later (including continuing in
> +                * this function), as it will otherwise cause a SEGV.
> +                */
> +               def->nparallels = i;
> +               return;
> +           }
> +        }
>      }
>  
>      /* Now get the details about the parallel ports here */
>