[libvirt] [PATCH] domain: Fix crash if trying to live update disk <serial>
If you pass <disk><serial> XML to UpdateDevice, and the original device didn't have a <serial> block, libvirtd crashes trying to read the original NULL serial string. Use _NULLABLE string comparisons to avoid the crash. A couple other properties needed the change too. --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fd0450f..f1e02e3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5871,28 +5871,28 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk, CHECK_EQ(transient, "transient", true); - if (disk->serial && STRNEQ(disk->serial, orig_disk->serial)) { + if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "serial"); return false; } - if (disk->wwn && STRNEQ(disk->wwn, orig_disk->wwn)) { + if (disk->wwn && STRNEQ_NULLABLE(disk->wwn, orig_disk->wwn)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "wwn"); return false; } - if (disk->vendor && STRNEQ(disk->vendor, orig_disk->vendor)) { + if (disk->vendor && STRNEQ_NULLABLE(disk->vendor, orig_disk->vendor)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "vendor"); return false; } - if (disk->product && STRNEQ(disk->product, orig_disk->product)) { + if (disk->product && STRNEQ_NULLABLE(disk->product, orig_disk->product)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "product"); -- 2.4.3
On 08/10/2015 07:33 PM, Cole Robinson wrote:
If you pass <disk><serial> XML to UpdateDevice, and the original device didn't have a <serial> block, libvirtd crashes trying to read the original NULL serial string.
Use _NULLABLE string comparisons to avoid the crash. A couple other properties needed the change too. --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Using STRNEQ_NULLABLE means you probably don't have to check the disk->* value either, right? ACK - with that adjustment... John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fd0450f..f1e02e3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5871,28 +5871,28 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk,
CHECK_EQ(transient, "transient", true);
- if (disk->serial && STRNEQ(disk->serial, orig_disk->serial)) { + if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "serial"); return false; }
- if (disk->wwn && STRNEQ(disk->wwn, orig_disk->wwn)) { + if (disk->wwn && STRNEQ_NULLABLE(disk->wwn, orig_disk->wwn)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "wwn"); return false; }
- if (disk->vendor && STRNEQ(disk->vendor, orig_disk->vendor)) { + if (disk->vendor && STRNEQ_NULLABLE(disk->vendor, orig_disk->vendor)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "vendor"); return false; }
- if (disk->product && STRNEQ(disk->product, orig_disk->product)) { + if (disk->product && STRNEQ_NULLABLE(disk->product, orig_disk->product)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "product");
On 08/11/2015 05:25 PM, John Ferlan wrote:
On 08/10/2015 07:33 PM, Cole Robinson wrote:
If you pass <disk><serial> XML to UpdateDevice, and the original device didn't have a <serial> block, libvirtd crashes trying to read the original NULL serial string.
Use _NULLABLE string comparisons to avoid the crash. A couple other properties needed the change too. --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Using STRNEQ_NULLABLE means you probably don't have to check the disk->* value either, right?
ACK - with that adjustment...
It would change the semantic a bit; if the orig disk has a <serial>, but not the XML passed to UpdateDevice, libvirt would now error where previously it wouldn't. It might make sense but I didn't want to change behavior - Cole
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fd0450f..f1e02e3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5871,28 +5871,28 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk,
CHECK_EQ(transient, "transient", true);
- if (disk->serial && STRNEQ(disk->serial, orig_disk->serial)) { + if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "serial"); return false; }
- if (disk->wwn && STRNEQ(disk->wwn, orig_disk->wwn)) { + if (disk->wwn && STRNEQ_NULLABLE(disk->wwn, orig_disk->wwn)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "wwn"); return false; }
- if (disk->vendor && STRNEQ(disk->vendor, orig_disk->vendor)) { + if (disk->vendor && STRNEQ_NULLABLE(disk->vendor, orig_disk->vendor)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "vendor"); return false; }
- if (disk->product && STRNEQ(disk->product, orig_disk->product)) { + if (disk->product && STRNEQ_NULLABLE(disk->product, orig_disk->product)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), "product");
On 08/11/2015 05:28 PM, Cole Robinson wrote:
On 08/11/2015 05:25 PM, John Ferlan wrote:
On 08/10/2015 07:33 PM, Cole Robinson wrote:
If you pass <disk><serial> XML to UpdateDevice, and the original device didn't have a <serial> block, libvirtd crashes trying to read the original NULL serial string.
Use _NULLABLE string comparisons to avoid the crash. A couple other properties needed the change too. --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Using STRNEQ_NULLABLE means you probably don't have to check the disk->* value either, right?
ACK - with that adjustment...
It would change the semantic a bit; if the orig disk has a <serial>, but not the XML passed to UpdateDevice, libvirt would now error where previously it wouldn't. It might make sense but I didn't want to change behavior
- Cole
OK - fair enough... right - time to quit for the day I guess ;-) ACK to what's on list John
On 08/11/2015 05:32 PM, John Ferlan wrote:
On 08/11/2015 05:28 PM, Cole Robinson wrote:
On 08/11/2015 05:25 PM, John Ferlan wrote:
On 08/10/2015 07:33 PM, Cole Robinson wrote:
If you pass <disk><serial> XML to UpdateDevice, and the original device didn't have a <serial> block, libvirtd crashes trying to read the original NULL serial string.
Use _NULLABLE string comparisons to avoid the crash. A couple other properties needed the change too. --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Using STRNEQ_NULLABLE means you probably don't have to check the disk->* value either, right?
ACK - with that adjustment...
It would change the semantic a bit; if the orig disk has a <serial>, but not the XML passed to UpdateDevice, libvirt would now error where previously it wouldn't. It might make sense but I didn't want to change behavior
- Cole
OK - fair enough... right - time to quit for the day I guess ;-)
ACK to what's on list
Thanks, pushed now - Cole
On Tue, Aug 11, 2015 at 05:37:30PM -0400, Cole Robinson wrote:
On 08/11/2015 05:32 PM, John Ferlan wrote:
On 08/11/2015 05:28 PM, Cole Robinson wrote:
On 08/11/2015 05:25 PM, John Ferlan wrote:
On 08/10/2015 07:33 PM, Cole Robinson wrote:
If you pass <disk><serial> XML to UpdateDevice, and the original device didn't have a <serial> block, libvirtd crashes trying to read the original NULL serial string.
Use _NULLABLE string comparisons to avoid the crash. A couple other properties needed the change too. --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Using STRNEQ_NULLABLE means you probably don't have to check the disk->* value either, right?
ACK - with that adjustment...
It would change the semantic a bit; if the orig disk has a <serial>, but not the XML passed to UpdateDevice, libvirt would now error where previously it wouldn't. It might make sense but I didn't want to change behavior
- Cole
OK - fair enough... right - time to quit for the day I guess ;-)
ACK to what's on list
Thanks, pushed now
Just a short info. Thanks for finding this out, sorry I meesed it up. And you still have to check for the disk->value because that's there on purpose. Only check changed values, no value means no check to be done, so you chose wisely :) Have a nice day, Martin
participants (3)
-
Cole Robinson -
John Ferlan -
Martin Kletzander