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

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