On Tue, Aug 28, 2012 at 11:16:13AM -0700, Eric Blake wrote:
> Without this patch, logged command executions can be ambiguous if
> the command contained any shell metacharacters. This has caused
> more than one person to attempt to patch clients to add unnecessary
> quoting, without realizing that the command itself was run with
> correct args, and only the logged output was ambiguous.
>
> * src/util/command.c (virCommandToString): Add shell escapes.
> * tests/commandtest.c (test16): Test new behavior.
> * tests/commanddata/test16.log: Update expected output.
> * tests/qemuxml2argvdata/qemuxml2argv-*.args: Likewise.
> * tests/networkxml2argvdata/*.argv: Likewise.
> ---
> src/util/command.c | 25 ++++++++++++++++------
> tests/commanddata/test16.log | 2 +-
> tests/commandtest.c | 6 ++++--
> .../nat-network-dns-txt-record.argv | 2 +-
> .../qemuxml2argv-disk-drive-network-rbd-auth.args | 7 +++---
> .../qemuxml2argv-disk-drive-network-rbd.args | 7 +++---
> .../qemuxml2argv-graphics-vnc.args | 2 +-
> tests/qemuxml2argvdata/qemuxml2argv-qemu-ns.args | 2 +-
> tests/qemuxml2argvdata/qemuxml2argv-smbios.args | 6 +++---
> 9 files changed, 38 insertions(+), 21 deletions(-)
>
> diff --git a/src/util/command.c b/src/util/command.c
> index 49ec178..418b198 100644
> --- a/src/util/command.c
> +++ b/src/util/command.c
> @@ -1614,9 +1614,10 @@ virCommandWriteArgLog(virCommandPtr cmd, int logfd)
> * virCommandToString:
> * @cmd: the command to convert
> *
> - * Call after adding all arguments and environment settings, but before
> - * Run/RunAsync, to return a string representation of the environment and
> - * arguments of cmd. If virCommandRun cannot succeed (because of an
> + * Call after adding all arguments and environment settings, but
> + * before Run/RunAsync, to return a string representation of the
> + * environment and arguments of cmd, suitably quoted for pasting into
> + * a shell. If virCommandRun cannot succeed (because of an
> * out-of-memory condition while building cmd), NULL will be returned.
> * Caller is responsible for freeing the resulting string.
> */
> @@ -1639,13 +1640,25 @@ virCommandToString(virCommandPtr cmd)
> }
>
> for (i = 0; i < cmd->nenv; i++) {
> - virBufferAdd(&buf, cmd->env[i], strlen(cmd->env[i]));
> + /* In shell, a='b c' has a different meaning than 'a=b c', so
> + * we must determine where the '=' lives. */
> + char *eq = strchr(cmd->env[i], '=');
> +
> + if (!eq) {
> + virBufferFreeAndReset(&buf);
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("invalid use of command API"));
> + return NULL;
> + }
> + eq++;
> + virBufferAdd(&buf, cmd->env[i], eq - cmd->env[i]);
> + virBufferEscapeShell(&buf, eq);
> virBufferAddChar(&buf, ' ');
> }
> - virBufferAdd(&buf, cmd->args[0], strlen(cmd->args[0]));
> + virBufferEscapeShell(&buf, cmd->args[0]);
> for (i = 1; i < cmd->nargs; i++) {
> virBufferAddChar(&buf, ' ');
> - virBufferAdd(&buf, cmd->args[i], strlen(cmd->args[i]));
> + virBufferEscapeShell(&buf, cmd->args[i]);
> }
>
> if (virBufferError(&buf)) {
> diff --git a/tests/commanddata/test16.log b/tests/commanddata/test16.log
> index 7088165..119dd29 100644
> --- a/tests/commanddata/test16.log
> +++ b/tests/commanddata/test16.log
> @@ -1 +1 @@
> -A=B true C
> +A=B C=D E true F G H
> diff --git a/tests/commandtest.c b/tests/commandtest.c
> index b1c7523..c005153 100644
> --- a/tests/commandtest.c
> +++ b/tests/commandtest.c
> @@ -607,12 +607,14 @@ static int test16(const void *unused ATTRIBUTE_UNUSED)
> {
> virCommandPtr cmd = virCommandNew("true");
> char *outactual = NULL;
> - const char *outexpect = "A=B true C";
> + const char *outexpect = "A=B C='D E' true F 'G H'";
> int ret = -1;
> int fd = -1;
>
> virCommandAddEnvPair(cmd, "A", "B");
> - virCommandAddArg(cmd, "C");
> + virCommandAddEnvPair(cmd, "C", "D E");
> + virCommandAddArg(cmd, "F");
> + virCommandAddArg(cmd, "G H");
>
> if ((outactual = virCommandToString(cmd)) == NULL) {
> virErrorPtr err = virGetLastError();
> diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv
> index 1b31871..2a6c799 100644
> --- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv
> +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv
> @@ -1,6 +1,6 @@
> @DNSMASQ@ --strict-order --bind-interfaces \
> --local=// --domain-needed --filterwin2k --conf-file= \
> ---except-interface lo --txt-record=example,example value \
> +--except-interface lo '--txt-record=example,example value' \
> --listen-address 192.168.122.1 --listen-address 192.168.123.1 \
> --listen-address 2001:db8:ac10:fe01::1 \
> --listen-address 2001:db8:ac10:fd01::1 --listen-address 10.24.10.1 \
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
> index b323e91..02a9869 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth.args
> @@ -2,9 +2,10 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
> /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
> unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
> file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
> -file=rbd:pool/image:\
> +'file=rbd:pool/image:\
> id=myname:\
> key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\
> auth_supported=cephx\;none:\
> -mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
> -if=virtio,format=raw -net none -serial none -parallel none -usb
> +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;\
> +mon3.example.org\:6322,\
> +if=virtio,format=raw' -net none -serial none -parallel none -usb
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
> index 69cf7c7..61c8f7d 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd.args
> @@ -2,6 +2,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \
> /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
> unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -drive \
> file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
> -file=rbd:pool/image:auth_supported=none:\
> -mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
> -if=virtio,format=raw -net none -serial none -parallel none -usb
> +'file=rbd:pool/image:auth_supported=none:\
> +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;\
> +mon3.example.org\:6322,\
> +if=virtio,format=raw' -net none -serial none -parallel none -usb
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc.args
> index 2af1540..af99225 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc.args
> @@ -1,4 +1,4 @@
> LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
> /usr/bin/qemu -S -M pc -m 214 -smp 1 -monitor unix:/tmp/test-monitor,server,\
> nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none \
> --parallel none -usb -vnc [2001:1:2:3:4:5:1234:1234]:3
> +-parallel none -usb -vnc '[2001:1:2:3:4:5:1234:1234]:3'
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-qemu-ns.args b/tests/qemuxml2argvdata/qemuxml2argv-qemu-ns.args
> index 19450a1..88bdd13 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-qemu-ns.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-qemu-ns.args
> @@ -1,4 +1,4 @@
> -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test NS=ns BAR= \
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test NS=ns BAR='' \
> /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor \
> unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda \
> /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -unknown \
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args
> index 3f6cb81..ac28bad 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args
> @@ -1,7 +1,7 @@
> LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \
> -pc -m 214 -smp 1 -smbios type=0,vendor=LENOVO,version=6FET82WW (3.12 ) -smbios \
> -type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,\
> +pc -m 214 -smp 1 -smbios 'type=0,vendor=LENOVO,version=6FET82WW (3.12 )' \
> +-smbios 'type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,\
> serial=32dfcb37-5af1-552b-357c-be8c3aa38310,\
> -uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809,sku=1234567890,family=Red Hat \
> +uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809,sku=1234567890,family=Red Hat' \
> -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda \
> /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb
> --
> 1.7.11.4
ACK, sounds right, but I would rather push it after the release,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list