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

Re: [libvirt] [virt-tools-list] virt-manager/libvirt backwards compatibility problem?



On 07/27/2011 01:32 PM, Guido Günther wrote:
On Wed, Jul 27, 2011 at 10:53:01AM -0600, Eric Blake wrote:
[adding libvir-list]

On 07/27/2011 10:28 AM, Whit Blauvelt wrote:
Looks like my real problem may be not incorporating a Debian/Ubuntu patch
before building 0.9.x, since netcat differs:

https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/517478

Reading that bug report, it looks like upstream libvirt.git should
be patched to add a runtime test on whether a remote nc supports the
-q option, rather than blindly assuming that 'nc -q' exists on the
remote side.
This is the patch we're currently using in Debian for that (based on a
version originally forwarded from Ubuntu):

	http://anonscm.debian.org/gitweb/?p=pkg-libvirt/libvirt.git;a=blob;f=debian/patches/Autodetect-if-the-remote-nc-command-supports-the-q-o.patch

I'd be happy to push that since this is the distro specific patch I have
to adjust the most ;)


Pasting from that URL gave awkward results below; can you address my comments below, then post a v2 as a proper patch against libvirt.git?

 +++ b/src/rpc/virnetsocket.c

  22 @@ -596,9 +596,30 @@ int virNetSocketNewConnectSSH(const char *nodename,

  23      if (noTTY)

  24          virCommandAddArgList(cmd, "-T", "-o", "BatchMode=yes",

  25                               "-e", "none", NULL);

  26 -    virCommandAddArgList(cmd, nodename,

  27 -                         netcat ? netcat : "nc",

  28 -                         "-U", path, NULL);

  29 +

  30 +    virCommandAddArgList(cmd, nodename, "sh -c", NULL);

Why is "sh -c" passed as a single argument, separate from the argument passed to that shell? Yes, ssh will join all arguments, then resplit the combined argument according to shell rules (I hate the fact that ssh duplicates shell word parsing), but we should either be providing a single argument to ssh with the entire shell script, or breaking this into one argument per word to be joined by ssh (three total), instead of doing half and half (two words in one argument, the third word [hairy script] in another).

  31 +    /*

  32 +     * This ugly thing is a shell script to detect availability of

  33 +     * the -q option for 'nc': debian and suse based distros need this

  34 +     * flag to ensure the remote nc will exit on EOF, so it will go away

  35 +     * when we close the VNC tunnel. If it doesn't go away, subsequent

  36 +     * VNC connection attempts will hang.

s/VNC tunnel/connection tunnel/
s/VNC connection attempts/connection attempts/

This code is not a VNC tunnel.


  37 +     *

  38 +     * Fedora's 'nc' doesn't have this option, and apparently defaults

  39 +     * to the desired behavior.

  40 +     */

  41 +    virCommandAddArgFormat(cmd, "'%s -q 2>&1 | grep -q \"requires an argument\";"

grep -q is not portable to Solaris, even though it is required by POSIX. Instead, use grep with stdout and stderr redirected to /dev/null.


  42 +                           "if [ $? -eq 0 ] ; then"

Why split the nc probe from the if?  We could use the more compact:

"if %s -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then"


  43 +                           "   CMD=\"%s -q 0 -U %s\";"

I don't like this part. It is not safe to pass %s as the pathname through an additional round of shell parsing, since if the pathname has anything that might be construed as shell metacharacters, the parse will be destroyed. To some extent, that is already a pre-existing bug (slightly mitigated by the fact that 'path' is under libvirt's control, and should not be containing arbitrary characters unless you passed odd directory choices to ./configure). But we aren't even evaluating the 'netcat' variable for sanity - and that is an arbitrary string given completely by user input - sounds like we need an independent patch to reject a user-provided netcat that would be misparsed by ssh and any extra shell expansions. Without that, then this patch would need the burden of adding an extra level of escaping to match an extra level of shell parsing.


  44 +                           "else"

  45 +                           "   CMD=\"%s -U %s\";"

  46 +                           "fi;"

  47 +                           "eval \"$CMD\";'",

Yuck. We should avoid eval at all costs, since that adds a third(!) level of shell parsing on top of the two we're already suffering from (ssh and sh -c). I'd much rather see:

sh -c 'CMD=<either empty or -q 0>; nc $CMD -U path'

than

sh -c 'CMD=<either nc -q 0 -U path or nc -U path>; eval "$CMD"'

By the way, you can avoid one level of shell parsing by using this printf:

"sh -c 'CMD=..; \"$1\" $CMD -U \"$2\"' sh %s %s", netcat, path

that is, supply 'netcat' and 'path' to ssh at the same level of quoting they have always been used, with the shell script referring only to positional parameters rather than doing yet another round of parsing on those parameters. But if we already have to plug the hole of user-provided 'netcat' being safe for ssh re-parsing, I don't know that we're doing much better by going through those extra hoops.

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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