1:47 p.m.
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
2:32 p.m.
2010/8/20 Eduardo Otubo <otubo(a)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
4:08 p.m.
New subject: [libvirt] [PATCHv2] PHYP: Checking for NULL values when building new guest
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
2:59 a.m.
New subject: [libvirt] [PATCHv2] PHYP: Checking for NULL values when building new guest
2010/8/23 Eduardo Otubo <otubo(a)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
12:19 p.m.
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(a)linux.vnet.ibm.com
2:50 a.m.
2010/8/24 Eduardo Otubo <otubo(a)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
11:27 a.m.
New subject: [libvirt] [PATCHv3] 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 | 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
9:55 a.m.
New subject: [libvirt] [PATCHv3] PHYP: Checking for NULL values when building new guest
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
2:23 p.m.
New subject: [libvirt] [PATCHv3] PHYP: Checking for NULL values when building new guest
2010/9/29 Daniel Veillard <veillard(a)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
3:30 a.m.
New subject: [libvirt] [PATCHv3] PHYP: Checking for NULL values when building new guest
On Thu, Sep 30, 2010 at 09:23:50PM +0200, Matthias Bolte wrote:
2010/9/29 Daniel Veillard <veillard(a)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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5166
days inactive
5208
days old
9 comments
3 participants
participants (3)
-
Daniel Veillard -
Eduardo Otubo -
Matthias Bolte