Re: [PATCH] command: shell-quote when logging commands

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]