[libvirt] [PATCH 0/4] qemu: add comma escaping in qemu_command.c
The function virQEMUBuildBufferEscapeComma is used to escape commas in user provided fields for qemu command line processing in the four functions listed below. A corresponding test for each comma escaping instance has been added to tests/qemuxml2argvdata/name-escape.xml. This should complete the BiteSizedTask entry at https://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_mo.... Anya Harter (4): qemu: Escape commas for qemuBuildChrChardevStr qemu: Escape commas for qemuBuildChrChardevFileStr qemu: Escape commas for qemuBuildSmartcardCommandLine qemu: Escape commas for qemuBuildGrapicsSPICECommandLine src/qemu/qemu_command.c | 44 +++++++++++-------------- tests/qemuxml2argvdata/name-escape.args | 14 ++++++-- tests/qemuxml2argvdata/name-escape.xml | 18 ++++++++++ tests/qemuxml2argvtest.c | 10 +++++- 4 files changed, 58 insertions(+), 28 deletions(-) -- 2.17.1
Add comma escaping for dev->data.file.path in cases VIR_DOMAIN_CHR_TYPE_DEV and VIR_DOMAIN_CHR_TYPE_PIPE. Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 9 +++++---- tests/qemuxml2argvdata/name-escape.args | 4 ++++ tests/qemuxml2argvdata/name-escape.xml | 7 +++++++ tests/qemuxml2argvtest.c | 3 ++- 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bb956a77f4..b764008949 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4975,9 +4975,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, break; case VIR_DOMAIN_CHR_TYPE_DEV: - virBufferAsprintf(&buf, "%s,id=%s,path=%s", + virBufferAsprintf(&buf, "%s,id=%s,path=", STRPREFIX(alias, "parallel") ? "parport" : "tty", - charAlias, dev->data.file.path); + charAlias); + virQEMUBuildBufferEscapeComma(&buf, dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_FILE: @@ -4997,8 +4998,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, break; case VIR_DOMAIN_CHR_TYPE_PIPE: - virBufferAsprintf(&buf, "pipe,id=%s,path=%s", charAlias, - dev->data.file.path); + virBufferAsprintf(&buf, "pipe,id=%s,path=", charAlias); + virQEMUBuildBufferEscapeComma(&buf, dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_STDIO: diff --git a/tests/qemuxml2argvdata/name-escape.args b/tests/qemuxml2argvdata/name-escape.args index 5ff8c03db8..4b03068f95 100644 --- a/tests/qemuxml2argvdata/name-escape.args +++ b/tests/qemuxml2argvdata/name-escape.args @@ -23,6 +23,10 @@ bar=2/monitor.sock,server,nowait \ -no-acpi \ -boot c \ -usb \ +-chardev tty,id=charserial0,path=/dev/ttyS2,,foo \ +-device isa-serial,chardev=charserial0,id=serial0 \ +-chardev pipe,id=charchannel0,path=/tmp/guestfwd,,foo \ +-netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \ -vnc unix:/tmp/lib/domain--1-foo=1,,bar=2/vnc.sock \ -spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock \ -vga cirrus \ diff --git a/tests/qemuxml2argvdata/name-escape.xml b/tests/qemuxml2argvdata/name-escape.xml index 6b93d71798..3f5e1c9829 100644 --- a/tests/qemuxml2argvdata/name-escape.xml +++ b/tests/qemuxml2argvdata/name-escape.xml @@ -20,5 +20,12 @@ <graphics type='spice'> <listen type='socket'/> </graphics> + <serial type='dev'> + <source path='/dev/ttyS2,foo'/> + </serial> + <channel type='pipe'> + <source path='/tmp/guestfwd,foo'/> + <target type='guestfwd' address='10.0.2.1' port='4600'/> + </channel> </devices> </domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index f44cac9fef..c194ff59c9 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2760,7 +2760,8 @@ mymain(void) QEMU_CAPS_NAME_GUEST, QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_SPICE, - QEMU_CAPS_SPICE_UNIX); + QEMU_CAPS_SPICE_UNIX, + QEMU_CAPS_DEVICE_ISA_SERIAL); DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS); DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET); -- 2.17.1
On 06/18/2018 01:57 PM, Anya Harter wrote:
Add comma escaping for dev->data.file.path in cases VIR_DOMAIN_CHR_TYPE_DEV and VIR_DOMAIN_CHR_TYPE_PIPE.
Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 9 +++++---- tests/qemuxml2argvdata/name-escape.args | 4 ++++ tests/qemuxml2argvdata/name-escape.xml | 7 +++++++ tests/qemuxml2argvtest.c | 3 ++- 4 files changed, 18 insertions(+), 5 deletions(-)
Having tests is awesome! Thanks! Not sure why the bite size tasks omitted VIR_DOMAIN_CHR_TYPE_FILE too. If you want to investigate the FILE case that'd be good - just to make sure we aren't missing any! I'll still push this as is since it's fine, but if the FILE needs something it can be added afterwards. Reviewed-by: John Ferlan <jferlan@redhat.com> John
On 06/18/2018 07:37 PM, John Ferlan wrote:
On 06/18/2018 01:57 PM, Anya Harter wrote:
Add comma escaping for dev->data.file.path in cases VIR_DOMAIN_CHR_TYPE_DEV and VIR_DOMAIN_CHR_TYPE_PIPE.
Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 9 +++++---- tests/qemuxml2argvdata/name-escape.args | 4 ++++ tests/qemuxml2argvdata/name-escape.xml | 7 +++++++ tests/qemuxml2argvtest.c | 3 ++- 4 files changed, 18 insertions(+), 5 deletions(-)
Having tests is awesome! Thanks!
Not sure why the bite size tasks omitted VIR_DOMAIN_CHR_TYPE_FILE too. If you want to investigate the FILE case that'd be good - just to make sure we aren't missing any! I'll still push this as is since it's fine, but if the FILE needs something it can be added afterwards.
VIR_DOMAIN_CHR_TYPE_FILE calls the function qemuBuildChrChardevFileStr and passes the dev->data.file.path into the parameter named fileval which I escape in the second patch. Please let me know if I am missing something here. Thanks, Anya
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
On 06/19/2018 10:19 AM, Anya Harter wrote:
On 06/18/2018 07:37 PM, John Ferlan wrote:
On 06/18/2018 01:57 PM, Anya Harter wrote:
Add comma escaping for dev->data.file.path in cases VIR_DOMAIN_CHR_TYPE_DEV and VIR_DOMAIN_CHR_TYPE_PIPE.
Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 9 +++++---- tests/qemuxml2argvdata/name-escape.args | 4 ++++ tests/qemuxml2argvdata/name-escape.xml | 7 +++++++ tests/qemuxml2argvtest.c | 3 ++- 4 files changed, 18 insertions(+), 5 deletions(-)
Having tests is awesome! Thanks!
Not sure why the bite size tasks omitted VIR_DOMAIN_CHR_TYPE_FILE too. If you want to investigate the FILE case that'd be good - just to make sure we aren't missing any! I'll still push this as is since it's fine, but if the FILE needs something it can be added afterwards.
VIR_DOMAIN_CHR_TYPE_FILE calls the function qemuBuildChrChardevFileStr and passes the dev->data.file.path into the parameter named fileval which I escape in the second patch.
Please let me know if I am missing something here.
Nope I didn't dig far enough... John
Add comma escaping for fileval. Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 3 ++- tests/qemuxml2argvdata/name-escape.args | 2 ++ tests/qemuxml2argvdata/name-escape.xml | 4 ++++ tests/qemuxml2argvtest.c | 3 ++- 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b764008949..40e8f8fccf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4867,7 +4867,8 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager, virBufferAsprintf(buf, ",%s=%s,%s=on", filearg, fdpath, appendarg); VIR_FREE(fdpath); } else { - virBufferAsprintf(buf, ",%s=%s", filearg, fileval); + virBufferAsprintf(buf, ",%s=", filearg); + virQEMUBuildBufferEscapeComma(buf, fileval); if (appendval != VIR_TRISTATE_SWITCH_ABSENT) { virBufferAsprintf(buf, ",%s=%s", appendarg, virTristateSwitchTypeToString(appendval)); diff --git a/tests/qemuxml2argvdata/name-escape.args b/tests/qemuxml2argvdata/name-escape.args index 4b03068f95..35a13b2533 100644 --- a/tests/qemuxml2argvdata/name-escape.args +++ b/tests/qemuxml2argvdata/name-escape.args @@ -25,6 +25,8 @@ bar=2/monitor.sock,server,nowait \ -usb \ -chardev tty,id=charserial0,path=/dev/ttyS2,,foo \ -device isa-serial,chardev=charserial0,id=serial0 \ +-chardev file,id=charserial1,path=/tmp/serial.log,,foo,append=on \ +-device isa-serial,chardev=charserial1,id=serial1 \ -chardev pipe,id=charchannel0,path=/tmp/guestfwd,,foo \ -netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \ -vnc unix:/tmp/lib/domain--1-foo=1,,bar=2/vnc.sock \ diff --git a/tests/qemuxml2argvdata/name-escape.xml b/tests/qemuxml2argvdata/name-escape.xml index 3f5e1c9829..79c1b34458 100644 --- a/tests/qemuxml2argvdata/name-escape.xml +++ b/tests/qemuxml2argvdata/name-escape.xml @@ -23,6 +23,10 @@ <serial type='dev'> <source path='/dev/ttyS2,foo'/> </serial> + <serial type='file'> + <source path='/tmp/serial.log,foo' append='on'/> + <target port='0'/> + </serial> <channel type='pipe'> <source path='/tmp/guestfwd,foo'/> <target type='guestfwd' address='10.0.2.1' port='4600'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c194ff59c9..3e02fa576c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2761,7 +2761,8 @@ mymain(void) QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_SPICE, QEMU_CAPS_SPICE_UNIX, - QEMU_CAPS_DEVICE_ISA_SERIAL); + QEMU_CAPS_DEVICE_ISA_SERIAL, + QEMU_CAPS_CHARDEV_FILE_APPEND); DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS); DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET); -- 2.17.1
On 06/18/2018 01:57 PM, Anya Harter wrote:
Add comma escaping for fileval.
Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 3 ++- tests/qemuxml2argvdata/name-escape.args | 2 ++ tests/qemuxml2argvdata/name-escape.xml | 4 ++++ tests/qemuxml2argvtest.c | 3 ++- 4 files changed, 10 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Add comma escaping for smartcard->data.cert.file[i] and smartcard->data.cert.database. Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 21 ++++----------------- tests/qemuxml2argvdata/name-escape.args | 3 +++ tests/qemuxml2argvdata/name-escape.xml | 6 ++++++ tests/qemuxml2argvtest.c | 3 ++- 4 files changed, 15 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 40e8f8fccf..a9a5e200e9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8716,29 +8716,16 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager, virBufferAddLit(&opt, "ccid-card-emulated,backend=certificates"); for (i = 0; i < VIR_DOMAIN_SMARTCARD_NUM_CERTIFICATES; i++) { - if (strchr(smartcard->data.cert.file[i], ',')) { - virBufferFreeAndReset(&opt); - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid certificate name: %s"), - smartcard->data.cert.file[i]); - return -1; - } - virBufferAsprintf(&opt, ",cert%zu=%s", i + 1, - smartcard->data.cert.file[i]); + virBufferAsprintf(&opt, ",cert%zu=", i + 1); + virQEMUBuildBufferEscapeComma(&opt, smartcard->data.cert.file[i]); } if (smartcard->data.cert.database) { - if (strchr(smartcard->data.cert.database, ',')) { - virBufferFreeAndReset(&opt); - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid database name: %s"), - smartcard->data.cert.database); - return -1; - } database = smartcard->data.cert.database; } else { database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; } - virBufferAsprintf(&opt, ",db=%s", database); + virBufferAddLit(&opt, ",db="); + virQEMUBuildBufferEscapeComma(&opt, database); break; case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: diff --git a/tests/qemuxml2argvdata/name-escape.args b/tests/qemuxml2argvdata/name-escape.args index 35a13b2533..d3b908a7e6 100644 --- a/tests/qemuxml2argvdata/name-escape.args +++ b/tests/qemuxml2argvdata/name-escape.args @@ -22,7 +22,10 @@ bar=2/monitor.sock,server,nowait \ -no-shutdown \ -no-acpi \ -boot c \ +-device usb-ccid,id=ccid0,bus=usb.0,port=1 \ -usb \ +-device ccid-card-emulated,backend=certificates,cert1=cert1,,foo,cert2=cert2,\ +cert3=cert3,db=/etc/pki/nssdb,,foo,id=smartcard0,bus=ccid0.0 \ -chardev tty,id=charserial0,path=/dev/ttyS2,,foo \ -device isa-serial,chardev=charserial0,id=serial0 \ -chardev file,id=charserial1,path=/tmp/serial.log,,foo,append=on \ diff --git a/tests/qemuxml2argvdata/name-escape.xml b/tests/qemuxml2argvdata/name-escape.xml index 79c1b34458..9ca7be5968 100644 --- a/tests/qemuxml2argvdata/name-escape.xml +++ b/tests/qemuxml2argvdata/name-escape.xml @@ -31,5 +31,11 @@ <source path='/tmp/guestfwd,foo'/> <target type='guestfwd' address='10.0.2.1' port='4600'/> </channel> + <smartcard mode='host-certificates'> + <certificate>cert1,foo</certificate> + <certificate>cert2</certificate> + <certificate>cert3</certificate> + <database>/etc/pki/nssdb,foo</database> + </smartcard> </devices> </domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 3e02fa576c..7468537c68 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2762,7 +2762,8 @@ mymain(void) QEMU_CAPS_SPICE, QEMU_CAPS_SPICE_UNIX, QEMU_CAPS_DEVICE_ISA_SERIAL, - QEMU_CAPS_CHARDEV_FILE_APPEND); + QEMU_CAPS_CHARDEV_FILE_APPEND, + QEMU_CAPS_CCID_EMULATED); DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS); DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET); -- 2.17.1
On 06/18/2018 01:57 PM, Anya Harter wrote:
Add comma escaping for smartcard->data.cert.file[i] and smartcard->data.cert.database.
Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 21 ++++----------------- tests/qemuxml2argvdata/name-escape.args | 3 +++ tests/qemuxml2argvdata/name-escape.xml | 6 ++++++ tests/qemuxml2argvtest.c | 3 ++- 4 files changed, 15 insertions(+), 18 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John (and pushed)
Add comma escaping for cfg->spiceTLSx509certdir and graphics->data.spice.rendernode. Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 11 ++++++++--- tests/qemuxml2argvdata/name-escape.args | 5 +++-- tests/qemuxml2argvdata/name-escape.xml | 1 + tests/qemuxml2argvtest.c | 5 +++++ 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a9a5e200e9..36dccea9b2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7974,8 +7974,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, !cfg->spicePassword) virBufferAddLit(&opt, "disable-ticketing,"); - if (hasSecure) - virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir); + if (hasSecure) { + virBufferAddLit(&opt, "x509-dir="); + virQEMUBuildBufferEscapeComma(&opt, cfg->spiceTLSx509certdir); + virBufferAsprintf(&opt, ","); + } switch (graphics->data.spice.defaultMode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: @@ -8082,7 +8085,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, goto error; } - virBufferAsprintf(&opt, "rendernode=%s,", graphics->data.spice.rendernode); + virBufferAddLit(&opt, "rendernode="); + virQEMUBuildBufferEscapeComma(&opt, graphics->data.spice.rendernode); + virBufferAsprintf(&opt, ","); } } diff --git a/tests/qemuxml2argvdata/name-escape.args b/tests/qemuxml2argvdata/name-escape.args index d3b908a7e6..72ed2e8410 100644 --- a/tests/qemuxml2argvdata/name-escape.args +++ b/tests/qemuxml2argvdata/name-escape.args @@ -33,6 +33,7 @@ cert3=cert3,db=/etc/pki/nssdb,,foo,id=smartcard0,bus=ccid0.0 \ -chardev pipe,id=charchannel0,path=/tmp/guestfwd,,foo \ -netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \ -vnc unix:/tmp/lib/domain--1-foo=1,,bar=2/vnc.sock \ --spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock \ --vga cirrus \ +-spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock,gl=on,\ +rendernode=/dev/dri/foo,,bar \ +-device cirrus-vga,id=video0,bus=pci.0,addr=0x2 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/name-escape.xml b/tests/qemuxml2argvdata/name-escape.xml index 9ca7be5968..0580de1813 100644 --- a/tests/qemuxml2argvdata/name-escape.xml +++ b/tests/qemuxml2argvdata/name-escape.xml @@ -19,6 +19,7 @@ </graphics> <graphics type='spice'> <listen type='socket'/> + <gl enable='yes' rendernode='/dev/dri/foo,bar'/> </graphics> <serial type='dev'> <source path='/dev/ttyS2,foo'/> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7468537c68..ade21f5a10 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2761,6 +2761,11 @@ mymain(void) QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_SPICE, QEMU_CAPS_SPICE_UNIX, + QEMU_CAPS_DEVICE_VIRTIO_GPU, + QEMU_CAPS_VIRTIO_GPU_VIRGL, + QEMU_CAPS_SPICE_GL, + QEMU_CAPS_SPICE_RENDERNODE, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_DEVICE_ISA_SERIAL, QEMU_CAPS_CHARDEV_FILE_APPEND, QEMU_CAPS_CCID_EMULATED); -- 2.17.1
On 06/18/2018 01:57 PM, Anya Harter wrote:
Add comma escaping for cfg->spiceTLSx509certdir and graphics->data.spice.rendernode.
Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 11 ++++++++--- tests/qemuxml2argvdata/name-escape.args | 5 +++-- tests/qemuxml2argvdata/name-escape.xml | 1 + tests/qemuxml2argvtest.c | 5 +++++ 4 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a9a5e200e9..36dccea9b2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7974,8 +7974,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, !cfg->spicePassword) virBufferAddLit(&opt, "disable-ticketing,");
- if (hasSecure) - virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir); + if (hasSecure) { + virBufferAddLit(&opt, "x509-dir="); + virQEMUBuildBufferEscapeComma(&opt, cfg->spiceTLSx509certdir); + virBufferAsprintf(&opt, ",");
make syntax-check would have told you: src/qemu/qemu_command.c:7980: virBufferAsprintf(&opt, ","); src/qemu/qemu_command.c:8090: virBufferAsprintf(&opt, ","); So here and below, I changed to virBufferAddLit before pushing.
+ }
switch (graphics->data.spice.defaultMode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: @@ -8082,7 +8085,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, goto error; }
- virBufferAsprintf(&opt, "rendernode=%s,", graphics->data.spice.rendernode); + virBufferAddLit(&opt, "rendernode="); + virQEMUBuildBufferEscapeComma(&opt, graphics->data.spice.rendernode); + virBufferAsprintf(&opt, ","); } }
Reviewed-by: John Ferlan <jferlan@redhat.com> John
From the last time I passed through this when Sukrit posted patches, still to do are qemuBuildHostNetStr, qemuBuildDiskThrottling (for group name), and various qemuBuildNetworkDriveURI, qemuBuildNetworkDriveStr, and qemuGetDriveSourceString.
[...]
On 06/18/2018 07:52 PM, John Ferlan wrote:
On 06/18/2018 01:57 PM, Anya Harter wrote:
Add comma escaping for cfg->spiceTLSx509certdir and graphics->data.spice.rendernode.
Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 11 ++++++++--- tests/qemuxml2argvdata/name-escape.args | 5 +++-- tests/qemuxml2argvdata/name-escape.xml | 1 + tests/qemuxml2argvtest.c | 5 +++++ 4 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a9a5e200e9..36dccea9b2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7974,8 +7974,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, !cfg->spicePassword) virBufferAddLit(&opt, "disable-ticketing,");
- if (hasSecure) - virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir); + if (hasSecure) { + virBufferAddLit(&opt, "x509-dir="); + virQEMUBuildBufferEscapeComma(&opt, cfg->spiceTLSx509certdir); + virBufferAsprintf(&opt, ",");
make syntax-check would have told you:
src/qemu/qemu_command.c:7980: virBufferAsprintf(&opt, ","); src/qemu/qemu_command.c:8090: virBufferAsprintf(&opt, ",");
So here and below, I changed to virBufferAddLit before pushing.
+ }
switch (graphics->data.spice.defaultMode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: @@ -8082,7 +8085,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, goto error; }
- virBufferAsprintf(&opt, "rendernode=%s,", graphics->data.spice.rendernode); + virBufferAddLit(&opt, "rendernode="); + virQEMUBuildBufferEscapeComma(&opt, graphics->data.spice.rendernode); + virBufferAsprintf(&opt, ","); } }
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
From the last time I passed through this when Sukrit posted patches, still to do are qemuBuildHostNetStr, qemuBuildDiskThrottling (for group name), and various qemuBuildNetworkDriveURI, qemuBuildNetworkDriveStr, and qemuGetDriveSourceString.
Oh cool, I didn't realize you had found more examples! I looked at some of these with Anya before the patch series. NetworkDriveURI is a private subset of NetworkDriveStr, so the former doesn't need any direct changes AFAICT. qemuGetDriveSourceString is called outside qemu_command.c, for example passing the result to qemu monitor commands. Anyone know if comma escaping applies there too? Same with qemuBuildHostNetStr Thanks, Cole
On 06/19/2018 09:47 AM, Cole Robinson wrote:
On 06/18/2018 07:52 PM, John Ferlan wrote:
On 06/18/2018 01:57 PM, Anya Harter wrote:
Add comma escaping for cfg->spiceTLSx509certdir and graphics->data.spice.rendernode.
Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 11 ++++++++--- tests/qemuxml2argvdata/name-escape.args | 5 +++-- tests/qemuxml2argvdata/name-escape.xml | 1 + tests/qemuxml2argvtest.c | 5 +++++ 4 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a9a5e200e9..36dccea9b2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7974,8 +7974,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, !cfg->spicePassword) virBufferAddLit(&opt, "disable-ticketing,");
- if (hasSecure) - virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir); + if (hasSecure) { + virBufferAddLit(&opt, "x509-dir="); + virQEMUBuildBufferEscapeComma(&opt, cfg->spiceTLSx509certdir); + virBufferAsprintf(&opt, ",");
make syntax-check would have told you:
src/qemu/qemu_command.c:7980: virBufferAsprintf(&opt, ","); src/qemu/qemu_command.c:8090: virBufferAsprintf(&opt, ",");
So here and below, I changed to virBufferAddLit before pushing.
+ }
switch (graphics->data.spice.defaultMode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: @@ -8082,7 +8085,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, goto error; }
- virBufferAsprintf(&opt, "rendernode=%s,", graphics->data.spice.rendernode); + virBufferAddLit(&opt, "rendernode="); + virQEMUBuildBufferEscapeComma(&opt, graphics->data.spice.rendernode); + virBufferAsprintf(&opt, ","); } }
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
From the last time I passed through this when Sukrit posted patches, still to do are qemuBuildHostNetStr, qemuBuildDiskThrottling (for group name), and various qemuBuildNetworkDriveURI, qemuBuildNetworkDriveStr, and qemuGetDriveSourceString.
Oh cool, I didn't realize you had found more examples! I looked at some of these with Anya before the patch series.
NetworkDriveURI is a private subset of NetworkDriveStr, so the former doesn't need any direct changes AFAICT.
qemuGetDriveSourceString is called outside qemu_command.c, for example passing the result to qemu monitor commands. Anyone know if comma escaping applies there too? Same with qemuBuildHostNetStr
From what I can tell qemuGetDriveSourceString and qemuBuildHostNetStr usages outside of qemu_command.c should _not_ have comma escaping, which makes sense as the comma isn't used as a delimiter in those substrings. So the comma escaping should be done at the call sites of those functions in qemu_command.c
Thanks, Cole
On 06/19/2018 10:46 AM, Cole Robinson wrote:
On 06/19/2018 09:47 AM, Cole Robinson wrote:
On 06/18/2018 07:52 PM, John Ferlan wrote:
On 06/18/2018 01:57 PM, Anya Harter wrote:
Add comma escaping for cfg->spiceTLSx509certdir and graphics->data.spice.rendernode.
Signed-off-by: Anya Harter <aharter@redhat.com> --- src/qemu/qemu_command.c | 11 ++++++++--- tests/qemuxml2argvdata/name-escape.args | 5 +++-- tests/qemuxml2argvdata/name-escape.xml | 1 + tests/qemuxml2argvtest.c | 5 +++++ 4 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a9a5e200e9..36dccea9b2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7974,8 +7974,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, !cfg->spicePassword) virBufferAddLit(&opt, "disable-ticketing,");
- if (hasSecure) - virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir); + if (hasSecure) { + virBufferAddLit(&opt, "x509-dir="); + virQEMUBuildBufferEscapeComma(&opt, cfg->spiceTLSx509certdir); + virBufferAsprintf(&opt, ",");
make syntax-check would have told you:
src/qemu/qemu_command.c:7980: virBufferAsprintf(&opt, ","); src/qemu/qemu_command.c:8090: virBufferAsprintf(&opt, ",");
So here and below, I changed to virBufferAddLit before pushing.
+ }
switch (graphics->data.spice.defaultMode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: @@ -8082,7 +8085,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, goto error; }
- virBufferAsprintf(&opt, "rendernode=%s,", graphics->data.spice.rendernode); + virBufferAddLit(&opt, "rendernode="); + virQEMUBuildBufferEscapeComma(&opt, graphics->data.spice.rendernode); + virBufferAsprintf(&opt, ","); } }
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
From the last time I passed through this when Sukrit posted patches, still to do are qemuBuildHostNetStr, qemuBuildDiskThrottling (for group name), and various qemuBuildNetworkDriveURI, qemuBuildNetworkDriveStr, and qemuGetDriveSourceString.
Oh cool, I didn't realize you had found more examples! I looked at some of these with Anya before the patch series.
NetworkDriveURI is a private subset of NetworkDriveStr, so the former doesn't need any direct changes AFAICT.
qemuGetDriveSourceString is called outside qemu_command.c, for example passing the result to qemu monitor commands. Anyone know if comma escaping applies there too? Same with qemuBuildHostNetStr
From what I can tell qemuGetDriveSourceString and qemuBuildHostNetStr usages outside of qemu_command.c should _not_ have comma escaping, which makes sense as the comma isn't used as a delimiter in those substrings. So the comma escaping should be done at the call sites of those functions in qemu_command.c
By my estimation the BiteSizedTask entry https://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_mo... is complete. The four bullet points have been addressed as follows: * Building the source drive string in qemuBuildNetworkDriveURI, qemuBuildNetworkDriveStr, and qemuGetDriveSourceString (src->hosts->socket, NBD exportname=src->path, Sheepdog src->path processing, rbd src->path & configFile processing) Since some of these functions are called from outside the building of the command line, escaping within the functions could lead to problems elsewhere. Thus, the approach is to escape the output of the functions from their callers. All calls to qemuBuildNetworkDriveURI are within qemuBuildNetworkDriveStr. One call to qemuBuildNetworkDriveStr is within qemuGetDriveSourceString, and the other is within qemuBuildSCSIiSCSIHostdevDrvStr. The only call to qemuGetDriveSourceString is within qemuBuildDriveSourceStr and the output is escaped later within this function. This escaping handles all but the one call to qemuBuildNetworkDriveStr. This case is handled in this patch: https://www.redhat.com/archives/libvir-list/2018-June/msg01462.html * Validate whether TYPE_FILE needs to follow TYPE_DEV and TYPE_PIPE handling in qemuBuildChrChardevStr Addressed in this email: https://www.redhat.com/archives/libvir-list/2018-June/msg01384.html * socket.address processing in qemuBuildHostNetStr (localaddr for UDP too) Cole believes that these entities cannot have commas in them anyways, so escaping any commas would just delay the inevitable * group_name processing in qemuBuildDiskThrottling Handled in this patch: https://www.redhat.com/archives/libvir-list/2018-June/msg01409.html Please let me know if there is any more work to be done on this task or if it is ready to be deleted from the page. Thanks so much, Anya
participants (3)
-
Anya Harter -
Cole Robinson -
John Ferlan