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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org