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

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

Posted by Laine Stump 6 years, 2 months ago

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.
+                */
+               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 */
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 6 years, 2 months ago


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 */
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Ján Tomko 6 years, 2 months ago

On Thu, Jan 25, 2018 at 07:31:14AM -0500, John Ferlan wrote:
>
>
>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),

If you cannot allocate the chardev, maybe you won't be able to allocate
the OOM error message either.

>> +                * 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.
>

Agreed.

This kind of half-fix with half a screen worth of comments is IMO a
waste of time. You're just going to crash on another unhandled
allocation error.

If you want the easy way out, you have my ACK to just change of the
allocation function from VIR_ALLOC to virDomainChrDefNew.

Jan

>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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list