[libvirt] [PATCH] PHYP: Checking for NULL values when building new guest
When creating a new gust, the function phypBuildLpar() was not checking for NULL values, making the driver to have a segmentation fault. --- src/phyp/phyp_driver.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 251111d..999870e 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3701,6 +3701,25 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; + if (!def->name) { + VIR_ERROR0(_("Field \"<name>\" on the domain XML file missing.")); + goto err; + } else if (!def->memory) { + VIR_ERROR0(_ + ("Field \"<memory>\" on the domain XML file missing.")); + goto err; + } else if (!def->maxmem) { + VIR_ERROR0(_ + ("Field \"<currentMemory>\" on the domain XML file missing.")); + goto err; + } else if (!def->vcpus) { + VIR_ERROR0(_("Field \"<vcpu>\" on the domain XML file missing.")); + goto err; + } else if (!def->disks[0]->src) { + VIR_ERROR0(_("Field \"<disk>\" on the domain XML file missing.")); + goto err; + } + virBufferAddLit(&buf, "mksyscfg"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); -- 1.7.0.4
2010/8/20 Eduardo Otubo <otubo@linux.vnet.ibm.com>:
When creating a new gust, the function phypBuildLpar() was not checking for NULL values, making the driver to have a segmentation fault. --- src/phyp/phyp_driver.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 251111d..999870e 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3701,6 +3701,25 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER;
+ if (!def->name) { + VIR_ERROR0(_("Field \"<name>\" on the domain XML file missing.")); + goto err;
def->name cannot be NULL, virDomainDefParseXML fails when there is no name given in the domain XML
+ } else if (!def->memory) { + VIR_ERROR0(_ + ("Field \"<memory>\" on the domain XML file missing.")); + goto err; + } else if (!def->maxmem) { + VIR_ERROR0(_ + ("Field \"<currentMemory>\" on the domain XML file missing.")); + goto err;
memory and maxmem might be 0 when the user sets the to 0 in the domain XML. IMHO comparing > 0 is cleaner here. Also the error message is misleading here. The elements aren't missing, but the user might have specified a value that is invalid for this driver.
+ } else if (!def->vcpus) { + VIR_ERROR0(_("Field \"<vcpu>\" on the domain XML file missing.")); + goto err;
vcpus can only be 0 when the user explicitly specified it that way, the vcpu element isn't missing in that case. If the element is really missing then vcpus defaults to 1.
+ } else if (!def->disks[0]->src) { + VIR_ERROR0(_("Field \"<disk>\" on the domain XML file missing.")); + goto err; + }
This can segfault because you dereference the first disk element without even checking if there are any disks. Once again the error message is misleading. Also you should use PHYP_ERROR instead of VIR_ERROR here, because VIR_ERROR will just output the error to the log and doesn't raise an error at the libvirt API level, but this is required when a driver functions fails. In general all other occurrences of VIR_ERROR in this driver should be replaced by PHYP_ERROR for the same reason. There are also several instances of VIR_WARN followed by goto err that should be PHYP_ERRORs too. Matthias
I fixed the way I check for invalid values and changed the way I report errors (from VIR_ERROR0 to PHYP_ERROR). I'll change the VIR_WARNs in another different patch. --- src/phyp/phyp_driver.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 251111d..a74eedf 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3701,6 +3701,29 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; + if (def->memory > 0) { + PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", + _("Field \"<memory>\" on the domain XML file is missing or has " + "invalid value.")); + goto err; + } + + if (def->maxmem > 0) { + PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", + _("Field \"<currentMemory>\" on the domain XML file is missing or" + " has invalid value.")); + goto err; + } + + if (def->ndisks > 0) { + if (!def->disks[0]->src) { + PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", + _("Field \"<src>\" under \"<disk>\" on the domain XML file is " + "missing.")); + goto err; + } + } + virBufferAddLit(&buf, "mksyscfg"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); -- 1.7.0.4
2010/8/23 Eduardo Otubo <otubo@linux.vnet.ibm.com>:
I fixed the way I check for invalid values and changed the way I report errors (from VIR_ERROR0 to PHYP_ERROR). I'll change the VIR_WARNs in another different patch.
Okay, that's fine.
--- src/phyp/phyp_driver.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 251111d..a74eedf 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3701,6 +3701,29 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER;
+ if (def->memory > 0) { + PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", + _("Field \"<memory>\" on the domain XML file is missing or has " + "invalid value.")); + goto err; + } + + if (def->maxmem > 0) { + PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", + _("Field \"<currentMemory>\" on the domain XML file is missing or" + " has invalid value.")); + goto err; + }
Sorry for confusing you about "comparing > 0", now the check is wrong here and you report an error when the input is actually valid. Please change this back to "if (!def->memory)".
+ if (def->ndisks > 0) { + if (!def->disks[0]->src) { + PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", + _("Field \"<src>\" under \"<disk>\" on the domain XML file is " + "missing.")); + goto err; + } + }
Okay, no potential segfault here anymore. But a few lines below you use def->disks[0]->src in a virBufferVSprintf call, without checking if the first disk element is there. So you either need to make the first disk element mandatory by checking for def->ndisks being 0 and reporting an error in that case, or by altering the virBufferVSprintf call to only output the virtual_scsi_adapters=%s part when the domain XML contains a disk element. Matthias
memory and maxmem might be 0 when the user sets the to 0 in the domain XML. IMHO comparing> 0 is cleaner here.
I already submitted the patch v2 with this fix and I didn't understand exactly this comparison > 0. Memory should be greater than zero, right? Hence, if memory < 0, then ERROR. Right? If not, could you explain the reasons? Thanks, -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com
2010/8/24 Eduardo Otubo <otubo@linux.vnet.ibm.com>:
memory and maxmem might be 0 when the user sets the to 0 in the domain XML. IMHO comparing> 0 is cleaner here.
I already submitted the patch v2 with this fix and I didn't understand exactly this comparison > 0. Memory should be greater than zero, right? Hence, if memory < 0, then ERROR. Right? If not, could you explain the reasons?
Thanks,
Sorry, that's my fault. def->memory and def->maxmem are unsigned so your original check using "!" was perfectly fine. Matthias
When creating a new gust, the function phypBuildLpar() was not checking for NULL values, making the driver to have a segmentation fault. --- src/phyp/phyp_driver.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 251111d..fcbb15e 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3701,6 +3701,29 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; + if (!def->memory) { + PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", + _("Field \"<memory>\" on the domain XML file is missing or has " + "invalid value.")); + goto err; + } + + if (!def->maxmem) { + PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", + _("Field \"<currentMemory>\" on the domain XML file is missing or" + " has invalid value.")); + goto err; + } + + if (def->ndisks > 0) { + if (!def->disks[0]->src) { + PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", + _("Field \"<src>\" under \"<disk>\" on the domain XML file is " + "missing.")); + goto err; + } + } + virBufferAddLit(&buf, "mksyscfg"); if (system_type == HMC) virBufferVSprintf(&buf, " -m %s", managed_system); -- 1.7.0.4
On Wed, Aug 25, 2010 at 01:27:44PM -0300, Eduardo Otubo wrote:
When creating a new gust, the function phypBuildLpar() was not checking for NULL values, making the driver to have a segmentation fault. --- src/phyp/phyp_driver.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
Seems we had forgotten that old patch, applied it, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2010/9/29 Daniel Veillard <veillard@redhat.com>:
On Wed, Aug 25, 2010 at 01:27:44PM -0300, Eduardo Otubo wrote:
When creating a new gust, the function phypBuildLpar() was not checking for NULL values, making the driver to have a segmentation fault. --- src/phyp/phyp_driver.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
Seems we had forgotten that old patch, applied it,
thanks !
Daniel
Ah, yes, but the patch you applied didn't fix the segfault a few lines below where def->disks[0]->src is used without checking for def->ndisks > 0 before, as I pointed out in an earlier review. I've send an additional patch to fix this: https://www.redhat.com/archives/libvir-list/2010-September/msg00571.html Matthias
On Thu, Sep 30, 2010 at 09:23:50PM +0200, Matthias Bolte wrote:
2010/9/29 Daniel Veillard <veillard@redhat.com>:
On Wed, Aug 25, 2010 at 01:27:44PM -0300, Eduardo Otubo wrote:
When creating a new gust, the function phypBuildLpar() was not checking for NULL values, making the driver to have a segmentation fault. --- src/phyp/phyp_driver.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
Seems we had forgotten that old patch, applied it,
thanks !
Daniel
Ah, yes, but the patch you applied didn't fix the segfault a few lines below where def->disks[0]->src is used without checking for def->ndisks > 0 before, as I pointed out in an earlier review.
I've send an additional patch to fix this:
https://www.redhat.com/archives/libvir-list/2010-September/msg00571.html
Ahhh , please push :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel Veillard -
Eduardo Otubo -
Matthias Bolte