[libvirt] [PATCH] vbox: fix SEGV during dumpxml of a serial port
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
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 */
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
participants (3)
-
John Ferlan -
Ján Tomko -
Laine Stump