[libvirt] [PATCH 3/3] Use virBufferEscapeShell in virNetSocketNewConnectSSH
to escape the netcat command since it's passed to the shell. Adjust expected test case output accordingly. --- src/rpc/virnetsocket.c | 25 ++++++++++++++++++++----- tests/virnetsockettest.c | 10 +++++----- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index ea653da..0105e45 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -612,7 +612,10 @@ int virNetSocketNewConnectSSH(const char *nodename, const char *path, virNetSocketPtr *retsock) { + char *quoted; virCommandPtr cmd; + virBuffer buf = VIR_BUFFER_INITIALIZER; + *retsock = NULL; cmd = virCommandNew(binary ? binary : "ssh"); @@ -636,6 +639,19 @@ int virNetSocketNewConnectSSH(const char *nodename, virCommandAddArgList(cmd, "-o", "StrictHostKeyChecking=no", NULL); virCommandAddArgList(cmd, nodename, "sh", "-c", NULL); + + virBufferEscapeShell(&buf, netcat ? netcat : "nc"); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + quoted = virBufferContentAndReset(&buf); + if (quoted == NULL) { + virReportOOMError(); + return -1; + } + /* * This ugly thing is a shell script to detect availability of * the -q option for 'nc': debian and suse based distros need this @@ -647,14 +663,13 @@ int virNetSocketNewConnectSSH(const char *nodename, * behavior. */ virCommandAddArgFormat(cmd, - "'if %s -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then" + "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then" " ARG=-q0;" "fi;" - "%s $ARG -U %s'", - netcat ? netcat : "nc", - netcat ? netcat : "nc", - path); + "'%s' $ARG -U %s'", + quoted, quoted, path); + VIR_FREE(quoted); return virNetSocketNewConnectCommand(cmd, retsock); } diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index b3a2705..c063e74 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -496,7 +496,7 @@ mymain(void) struct testSSHData sshData1 = { .nodename = "somehost", .path = "/tmp/socket", - .expectOut = "somehost sh -c 'if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n", + .expectOut = "somehost sh -c 'if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;'nc' $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 1", 1, testSocketSSH, &sshData1) < 0) ret = -1; @@ -509,7 +509,7 @@ mymain(void) .noTTY = true, .noVerify = false, .path = "/tmp/socket", - .expectOut = "-p 9000 -l fred -T -o BatchMode=yes -e none somehost sh -c 'if netcat -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;netcat $ARG -U /tmp/socket'\n", + .expectOut = "-p 9000 -l fred -T -o BatchMode=yes -e none somehost sh -c 'if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;'netcat' $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 2", 1, testSocketSSH, &sshData2) < 0) ret = -1; @@ -522,7 +522,7 @@ mymain(void) .noTTY = false, .noVerify = true, .path = "/tmp/socket", - .expectOut = "-p 9000 -l fred -o StrictHostKeyChecking=no somehost sh -c 'if netcat -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;netcat $ARG -U /tmp/socket'\n", + .expectOut = "-p 9000 -l fred -o StrictHostKeyChecking=no somehost sh -c 'if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;'netcat' $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 3", 1, testSocketSSH, &sshData3) < 0) ret = -1; @@ -538,7 +538,7 @@ mymain(void) struct testSSHData sshData5 = { .nodename = "crashyhost", .path = "/tmp/socket", - .expectOut = "crashyhost sh -c 'if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n", + .expectOut = "crashyhost sh -c 'if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;'nc' $ARG -U /tmp/socket'\n", .dieEarly = true, }; @@ -550,7 +550,7 @@ mymain(void) .path = "/tmp/socket", .keyfile = "/root/.ssh/example_key", .noVerify = true, - .expectOut = "-i /root/.ssh/example_key -o StrictHostKeyChecking=no example.com sh -c 'if nc -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n", + .expectOut = "-i /root/.ssh/example_key -o StrictHostKeyChecking=no example.com sh -c 'if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then ARG=-q0;fi;'nc' $ARG -U /tmp/socket'\n", }; if (virtTestRun("SSH test 6", 1, testSocketSSH, &sshData6) < 0) ret = -1; -- 1.7.6.3
On 10/12/2011 04:40 PM, Guido Günther wrote:
to escape the netcat command since it's passed to the shell. Adjust expected test case output accordingly. --- src/rpc/virnetsocket.c | 25 ++++++++++++++++++++----- tests/virnetsockettest.c | 10 +++++----- 2 files changed, 25 insertions(+), 10 deletions(-)
ACK with nits fixed. Still to go - it would be nice to follow up with a 4/3 that converts virsh.c cmdEcho to use virBufferEscapeShell.
@@ -636,6 +639,19 @@ int virNetSocketNewConnectSSH(const char *nodename, virCommandAddArgList(cmd, "-o", "StrictHostKeyChecking=no", NULL);
virCommandAddArgList(cmd, nodename, "sh", "-c", NULL); + + virBufferEscapeShell(&buf, netcat ? netcat : "nc"); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + quoted = virBufferContentAndReset(&buf); + if (quoted == NULL) {
You are guaranteed that quoted is not NULL, by virtue of the fact that you already filtered for virBufferError just beforehand. Drop this if statement.
+++ b/tests/virnetsockettest.c @@ -496,7 +496,7 @@ mymain(void) struct testSSHData sshData1 = { .nodename = "somehost", .path = "/tmp/socket", - .expectOut = "somehost sh -c 'if nc -q 2>&1 | grep \"requires an argument\">/dev/null 2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n", + .expectOut = "somehost sh -c 'if 'nc' -q 2>&1 | grep \"requires an argument\">/dev/null 2>&1; then ARG=-q0;fi;'nc' $ARG -U /tmp/socket'\n", };
Rebase this on top of my comments for 1/3 (no long lines). Also, add one more test where $NC is something that actually proves things work with shell meta-characters, perhaps by passing 'nc -4' as the value that eventually supplies the %s for $NC, so that .expectOut will include: sh -c 'if ''nc -4'' -q 2>&1 .... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Oct 12, 2011 at 05:22:02PM -0600, Eric Blake wrote:
On 10/12/2011 04:40 PM, Guido Günther wrote:
to escape the netcat command since it's passed to the shell. Adjust expected test case output accordingly. --- src/rpc/virnetsocket.c | 25 ++++++++++++++++++++----- tests/virnetsockettest.c | 10 +++++----- 2 files changed, 25 insertions(+), 10 deletions(-)
ACK with nits fixed. Still to go - it would be nice to follow up with a 4/3 that converts virsh.c cmdEcho to use virBufferEscapeShell.
@@ -636,6 +639,19 @@ int virNetSocketNewConnectSSH(const char *nodename, virCommandAddArgList(cmd, "-o", "StrictHostKeyChecking=no", NULL);
virCommandAddArgList(cmd, nodename, "sh", "-c", NULL); + + virBufferEscapeShell(&buf, netcat ? netcat : "nc"); + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + quoted = virBufferContentAndReset(&buf); + if (quoted == NULL) {
You are guaranteed that quoted is not NULL, by virtue of the fact that you already filtered for virBufferError just beforehand. Drop this if statement.
Done.
+++ b/tests/virnetsockettest.c @@ -496,7 +496,7 @@ mymain(void) struct testSSHData sshData1 = { .nodename = "somehost", .path = "/tmp/socket", - .expectOut = "somehost sh -c 'if nc -q 2>&1 | grep \"requires an argument\">/dev/null 2>&1; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n", + .expectOut = "somehost sh -c 'if 'nc' -q 2>&1 | grep \"requires an argument\">/dev/null 2>&1; then ARG=-q0;fi;'nc' $ARG -U /tmp/socket'\n", };
Rebase this on top of my comments for 1/3 (no long lines). Also, add one more test where $NC is something that actually proves things work with shell meta-characters, perhaps by passing 'nc -4' as the value that eventually supplies the %s for $NC, so that .expectOut will include: sh -c 'if ''nc -4'' -q 2>&1 ....
Done too. Cheers, -- Guido
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/13/2011 03:02 PM, Guido Günther wrote:
On Wed, Oct 12, 2011 at 05:22:02PM -0600, Eric Blake wrote:
On 10/12/2011 04:40 PM, Guido Günther wrote:
to escape the netcat command since it's passed to the shell. Adjust expected test case output accordingly. ---
Rebase this on top of my comments for 1/3 (no long lines). Also, add one more test where $NC is something that actually proves things work with shell meta-characters, perhaps by passing 'nc -4' as the value that eventually supplies the %s for $NC, so that .expectOut will include: sh -c 'if ''nc -4'' -q 2>&1 ....
Done too.
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
--- tools/virsh.c | 25 +++++-------------------- 1 files changed, 5 insertions(+), 20 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index bcf0603..fe1a224 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12340,27 +12340,12 @@ cmdEcho (vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd) arg = opt->data; if (count) virBufferAddChar(&buf, ' '); - /* Add outer '' only if arg included shell metacharacters. */ - if (shell && - (strpbrk(arg, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~") || !*arg)) { - virBufferAddChar(&buf, '\''); - close_quote = true; - } - if (xml) { + if (xml) virBufferEscapeString(&buf, "%s", arg); - } else { - if (shell && (q = strchr(arg, '\''))) { - do { - virBufferAdd(&buf, arg, q - arg); - virBufferAddLit(&buf, "'\\''"); - arg = q + 1; - q = strchr(arg, '\''); - } while (q); - } - virBufferAdd(&buf, arg, strlen(arg)); - } - if (close_quote) - virBufferAddChar(&buf, '\''); + else if (shell) + virBufferEscapeShell(&buf, arg); + else + virBufferAdd(&buf, arg, -1); count++; } -- 1.7.6.3
On Thu, Oct 13, 2011 at 11:03:05PM +0200, Guido Günther wrote:
--- tools/virsh.c | 25 +++++-------------------- 1 files changed, 5 insertions(+), 20 deletions(-)
I just noticed that this breaks the testsuite for two reasons: cmdEcho emits '' in case of an *arg == 0 and it also allows for --shell --xml which I didn't think is useful at first but it actually is. I'll come up with a new version. Cheers, -- Guido
diff --git a/tools/virsh.c b/tools/virsh.c index bcf0603..fe1a224 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12340,27 +12340,12 @@ cmdEcho (vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd) arg = opt->data; if (count) virBufferAddChar(&buf, ' '); - /* Add outer '' only if arg included shell metacharacters. */ - if (shell && - (strpbrk(arg, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~") || !*arg)) { - virBufferAddChar(&buf, '\''); - close_quote = true; - } - if (xml) { + if (xml) virBufferEscapeString(&buf, "%s", arg); - } else { - if (shell && (q = strchr(arg, '\''))) { - do { - virBufferAdd(&buf, arg, q - arg); - virBufferAddLit(&buf, "'\\''"); - arg = q + 1; - q = strchr(arg, '\''); - } while (q); - } - virBufferAdd(&buf, arg, strlen(arg)); - } - if (close_quote) - virBufferAddChar(&buf, '\''); + else if (shell) + virBufferEscapeShell(&buf, arg); + else + virBufferAdd(&buf, arg, -1); count++; }
-- 1.7.6.3
participants (2)
-
Eric Blake -
Guido Günther