[libvirt] [PATCH 0/4] qemu: escape smbios strings passed on commandline
See patch 3 for the core change. Peter Krempa (4): qemu: command: Fix up coding style of smbios commandine formatters qemu: command: Don't bother reporting errors in smbios formatters qemu: command: escape smbios entry strings schema: smbios: allow any strings docs/schemas/domaincommon.rng | 4 +- src/qemu/qemu_command.c | 129 ++++++++++++++++++++++++------------------ 2 files changed, 76 insertions(+), 57 deletions(-) -- 2.10.0
--- src/qemu/qemu_command.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0804133..13c2cde 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5694,7 +5694,8 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager, } -static char *qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def) +static char * +qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5726,8 +5727,10 @@ static char *qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def) return NULL; } -static char *qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def, - bool skip_uuid) + +static char * +qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def, + bool skip_uuid) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5772,7 +5775,9 @@ static char *qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def, return NULL; } -static char *qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr def) + +static char * +qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr def) { virBuffer buf = VIR_BUFFER_INITIALIZER; -- 2.10.0
qemuBuildSmbiosBiosStr and qemuBuildSmbiosSystemStr return NULL if there's noting to format on the commandline. Reporting errors from buffer creation doesn't make sense since it would be ignored. --- src/qemu/qemu_command.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 13c2cde..11ae363 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5717,14 +5717,7 @@ qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def) if (def->release) virBufferAsprintf(&buf, ",release=%s", def->release); - if (virBufferCheckError(&buf) < 0) - goto error; - return virBufferContentAndReset(&buf); - - error: - virBufferFreeAndReset(&buf); - return NULL; } @@ -5765,14 +5758,7 @@ qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def, if (def->family) virBufferAsprintf(&buf, ",family=%s", def->family); - if (virBufferCheckError(&buf) < 0) - goto error; - return virBufferContentAndReset(&buf); - - error: - virBufferFreeAndReset(&buf); - return NULL; } -- 2.10.0
On 10/10/2016 11:51 AM, Peter Krempa wrote:
qemuBuildSmbiosBiosStr and qemuBuildSmbiosSystemStr return NULL if there's noting to format on the commandline. Reporting errors from
nothing
buffer creation doesn't make sense since it would be ignored.
Introduced by 54c0237ccb, so it's been this way a long time...
--- src/qemu/qemu_command.c | 14 -------------- 1 file changed, 14 deletions(-)
I suppose since it seems the only legitimate error you'll hit is ENOMEM and if you ignore it here, some shortly to be run code is sure to run into it, then no big deal... Although, one could argue the callers should check/return on error, but they'd probably lose that argument. ACK for what's here John
On Thu, Oct 13, 2016 at 17:11:54 -0400, John Ferlan wrote:
On 10/10/2016 11:51 AM, Peter Krempa wrote:
qemuBuildSmbiosBiosStr and qemuBuildSmbiosSystemStr return NULL if there's noting to format on the commandline. Reporting errors from
nothing
buffer creation doesn't make sense since it would be ignored.
Introduced by 54c0237ccb, so it's been this way a long time...
--- src/qemu/qemu_command.c | 14 -------------- 1 file changed, 14 deletions(-)
I suppose since it seems the only legitimate error you'll hit is ENOMEM and if you ignore it here, some shortly to be run code is sure to run into it, then no big deal...
Although, one could argue the callers should check/return on error, but they'd probably lose that argument.
Not really :). I thought the same when writing the patch as it's fully possible although extremely unlikely that we'd start a VM with invalid configuration. Given that we were doing it like this for quite some while and it would require a more invasive refactoring of the code I just decided to drop the code altogether.
ACK for what's here
Thanks, I'll go with this version currently, since it keeps the semantics present for a rather long time. Peter
We pass free-form strings from the users to qemu, thus we need escape commas since they are passed to qemu monitor. Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535 --- src/qemu/qemu_command.c | 102 +++++++++++++++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 36 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 11ae363..fffeeef 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5705,17 +5705,25 @@ qemuBuildSmbiosBiosStr(virSysinfoBIOSDefPtr def) virBufferAddLit(&buf, "type=0"); /* 0:Vendor */ - if (def->vendor) - virBufferAsprintf(&buf, ",vendor=%s", def->vendor); + if (def->vendor) { + virBufferAddLit(&buf, ",vendor="); + virQEMUBuildBufferEscapeComma(&buf, def->vendor); + } /* 0:BIOS Version */ - if (def->version) - virBufferAsprintf(&buf, ",version=%s", def->version); + if (def->version) { + virBufferAddLit(&buf, ",version="); + virQEMUBuildBufferEscapeComma(&buf, def->version); + } /* 0:BIOS Release Date */ - if (def->date) - virBufferAsprintf(&buf, ",date=%s", def->date); + if (def->date) { + virBufferAddLit(&buf, ",date="); + virQEMUBuildBufferEscapeComma(&buf, def->date); + } /* 0:System BIOS Major Release and 0:System BIOS Minor Release */ - if (def->release) - virBufferAsprintf(&buf, ",release=%s", def->release); + if (def->release) { + virBufferAddLit(&buf, ",release="); + virQEMUBuildBufferEscapeComma(&buf, def->release); + } return virBufferContentAndReset(&buf); } @@ -5736,27 +5744,40 @@ qemuBuildSmbiosSystemStr(virSysinfoSystemDefPtr def, virBufferAddLit(&buf, "type=1"); /* 1:Manufacturer */ - if (def->manufacturer) - virBufferAsprintf(&buf, ",manufacturer=%s", - def->manufacturer); + if (def->manufacturer) { + virBufferAddLit(&buf, ",manufacturer="); + virQEMUBuildBufferEscapeComma(&buf, def->manufacturer); + } /* 1:Product Name */ - if (def->product) - virBufferAsprintf(&buf, ",product=%s", def->product); + if (def->product) { + virBufferAddLit(&buf, ",product="); + virQEMUBuildBufferEscapeComma(&buf, def->product); + } /* 1:Version */ - if (def->version) - virBufferAsprintf(&buf, ",version=%s", def->version); + if (def->version) { + virBufferAddLit(&buf, ",version="); + virQEMUBuildBufferEscapeComma(&buf, def->version); + } /* 1:Serial Number */ - if (def->serial) - virBufferAsprintf(&buf, ",serial=%s", def->serial); + if (def->serial) { + virBufferAddLit(&buf, ",serial="); + virQEMUBuildBufferEscapeComma(&buf, def->serial); + } /* 1:UUID */ - if (def->uuid && !skip_uuid) - virBufferAsprintf(&buf, ",uuid=%s", def->uuid); + if (def->uuid && !skip_uuid) { + virBufferAddLit(&buf, ",uuid="); + virQEMUBuildBufferEscapeComma(&buf, def->uuid); + } /* 1:SKU Number */ - if (def->sku) - virBufferAsprintf(&buf, ",sku=%s", def->sku); + if (def->sku) { + virBufferAddLit(&buf, ",sku="); + virQEMUBuildBufferEscapeComma(&buf, def->sku); + } /* 1:Family */ - if (def->family) - virBufferAsprintf(&buf, ",family=%s", def->family); + if (def->family) { + virBufferAddLit(&buf, ",family="); + virQEMUBuildBufferEscapeComma(&buf, def->family); + } return virBufferContentAndReset(&buf); } @@ -5773,24 +5794,33 @@ qemuBuildSmbiosBaseBoardStr(virSysinfoBaseBoardDefPtr def) virBufferAddLit(&buf, "type=2"); /* 2:Manufacturer */ - if (def->manufacturer) - virBufferAsprintf(&buf, ",manufacturer=%s", - def->manufacturer); + virBufferAddLit(&buf, ",manufacturer="); + virQEMUBuildBufferEscapeComma(&buf, def->manufacturer); /* 2:Product Name */ - if (def->product) - virBufferAsprintf(&buf, ",product=%s", def->product); + if (def->product) { + virBufferAddLit(&buf, ",product="); + virQEMUBuildBufferEscapeComma(&buf, def->product); + } /* 2:Version */ - if (def->version) - virBufferAsprintf(&buf, ",version=%s", def->version); + if (def->version) { + virBufferAddLit(&buf, ",version="); + virQEMUBuildBufferEscapeComma(&buf, def->version); + } /* 2:Serial Number */ - if (def->serial) - virBufferAsprintf(&buf, ",serial=%s", def->serial); + if (def->serial) { + virBufferAddLit(&buf, ",serial="); + virQEMUBuildBufferEscapeComma(&buf, def->serial); + } /* 2:Asset Tag */ - if (def->asset) - virBufferAsprintf(&buf, ",asset=%s", def->asset); + if (def->asset) { + virBufferAddLit(&buf, ",asset="); + virQEMUBuildBufferEscapeComma(&buf, def->asset); + } /* 2:Location */ - if (def->location) - virBufferAsprintf(&buf, ",location=%s", def->location); + if (def->location) { + virBufferAddLit(&buf, ",location="); + virQEMUBuildBufferEscapeComma(&buf, def->location); + } if (virBufferCheckError(&buf) < 0) goto error; -- 2.10.0
On 10/10/2016 11:51 AM, Peter Krempa wrote:
We pass free-form strings from the users to qemu, thus we need escape commas since they are passed to qemu monitor.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535 --- src/qemu/qemu_command.c | 102 +++++++++++++++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 36 deletions(-)
ACK (oy vei!) John
The smbios docs allow any string to be passed and libvirt does not really do any validation on them. Allow passing any string. Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535 --- docs/schemas/domaincommon.rng | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6eeb4e9..f1609f9 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4517,9 +4517,7 @@ </define> <define name="sysinfo-value"> - <data type="string"> - <param name='pattern'>[a-zA-Z0-9/\-_\. \(\)]+</param> - </data> + <data type="string"/> </define> <define name="acpiTable"> -- 2.10.0
On 10/10/2016 11:51 AM, Peter Krempa wrote:
The smbios docs allow any string to be passed and libvirt does not really do any validation on them. Allow passing any string.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535 --- docs/schemas/domaincommon.rng | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
ACK, although couldn't the <ref name="sysinfo-value"/>'s each be replaced by <text/>, too? John
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6eeb4e9..f1609f9 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4517,9 +4517,7 @@ </define>
<define name="sysinfo-value"> - <data type="string"> - <param name='pattern'>[a-zA-Z0-9/\-_\. \(\)]+</param> - </data> + <data type="string"/> </define>
<define name="acpiTable">
On Thu, Oct 13, 2016 at 17:23:33 -0400, John Ferlan wrote:
On 10/10/2016 11:51 AM, Peter Krempa wrote:
The smbios docs allow any string to be passed and libvirt does not really do any validation on them. Allow passing any string.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1373535 --- docs/schemas/domaincommon.rng | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
ACK, although couldn't the <ref name="sysinfo-value"/>'s each be replaced by <text/>, too?
It could probably but I did not want to remove it if we ever revisit it. On the other hand in such case it will most likely be replaced with specific entries.
participants (2)
-
John Ferlan -
Peter Krempa