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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list