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

[Ján Tomko]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180125/43baa9f5/attachment-0001.sig>