Re: [libvirt] [PATCH 3/3] Use virBufferEscapeShell in virNetSocketNewConnectSSH
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 21 | grep \requires an argument\/dev/null 21; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n, +.expectOut = somehost sh -c 'if 'nc' -q 21 | grep \requires an argument\/dev/null 21; 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 21 Done too. Cheers, -- Guido -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org From 48650440f98ea4f975d71f97c5c050a1a4653690 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Guido=20G=C3=BCnther?= a...@sigxcpu.org Date: Thu, 13 Oct 2011 21:49:01 +0200 Subject: [PATCH 3/4] 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 | 18 +++--- tests/virnetsockettest.c | 34 -- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 2a9bca0..e4eff49 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); @@ -639,6 +642,14 @@ int virNetSocketNewConnectSSH(const char *nodename, netcat = nc; virCommandAddArgList(cmd, nodename, sh, -c, NULL); + +virBufferEscapeShell(buf, netcat); +if (virBufferError(buf)) { +virBufferFreeAndReset(buf); +virReportOOMError(); +return -1; +} +quoted = virBufferContentAndReset(buf); /* * This ugly thing is a shell script to detect availability of * the -q option for 'nc': debian and suse based distros need this @@ -650,14 +661,15 @@ int virNetSocketNewConnectSSH(const char *nodename, * behavior. */ virCommandAddArgFormat(cmd, - 'if %s -q 21 | grep \requires an argument\ /dev/null 21; then + 'if '%s' -q 21 | grep \requires an argument\ /dev/null 21; then ARG=-q0; else ARG=; fi; - %s $ARG -U %s', - netcat, netcat, 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 75cc9c0..6320ce0 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -496,12 +496,12 @@ mymain(void) struct testSSHData sshData1 = { .nodename = somehost, .path = /tmp/socket, -.expectOut = somehost sh -c 'if nc -q 21 | grep \requires an argument\ /dev/null 21; then +.expectOut = somehost sh -c 'if 'nc' -q 21 | grep \requires an argument\ /dev/null 21; then ARG=-q0; else ARG=; fi; - nc $ARG -U /tmp/socket'\n, + 'nc' $ARG -U /tmp/socket'\n,
Re: [libvirt] [PATCH 3/3] Use virBufferEscapeShell in virNetSocketNewConnectSSH
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 21 Done too. ACK. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[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 21 | grep \requires an argument\ /dev/null 21; then + 'if '%s' -q 21 | grep \requires an argument\ /dev/null 21; 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 21 | grep \requires an argument\ /dev/null 21; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n, +.expectOut = somehost sh -c 'if 'nc' -q 21 | grep \requires an argument\ /dev/null 21; 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 21 | grep \requires an argument\ /dev/null 21; 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 21 | grep \requires an argument\ /dev/null 21; 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 21 | grep \requires an argument\ /dev/null 21; then ARG=-q0;fi;netcat $ARG -U /tmp/socket'\n, +.expectOut = -p 9000 -l fred -o StrictHostKeyChecking=no somehost sh -c 'if 'netcat' -q 21 | grep \requires an argument\ /dev/null 21; 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 21 | grep \requires an argument\ /dev/null 21; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n, +.expectOut = crashyhost sh -c 'if 'nc' -q 21 | grep \requires an argument\ /dev/null 21; 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 21 | grep \requires an argument\ /dev/null 21; 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 21 | grep \requires an argument\ /dev/null 21; 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 -- libvir-list mailing
Re: [libvirt] [PATCH 3/3] Use virBufferEscapeShell in virNetSocketNewConnectSSH
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 21 | grep \requires an argument\/dev/null 21; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n, +.expectOut = somehost sh -c 'if 'nc' -q 21 | grep \requires an argument\/dev/null 21; 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 21 -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list